Dockerise testing for dev#1093
Conversation
Scott-Simmons
force-pushed
the
dockerise-testing-for-dev
branch
2 times, most recently
from
469f111 to
6773ae4
Compare
Add image for testing tsfresh with multiple python versions. This can be a good final sanity check e.g. before a release.
Targets include 1. Building dockerised testing environment 2. Testing tsfresh within testing environment 3. Testing tsfresh locally with tox with available python versions 4. Testing tsfresh locally with current active python version 5. Cleaning build artifacts 6. Formatting. This is so that changes can be made before pushing the changes up the remote to trigger the formatting checks. TODO: pre-commit hooks should probs be added too.
Change docs to reflect the new testing invokations.
Scott-Simmons
force-pushed
the
dockerise-testing-for-dev
branch
from
6773ae4 to
27f02b9
Compare
|
@nils-braun let me know If this is worth adding, hoping that it can helpful for the release process going forward |
nils-braun
left a comment
nils-braun
left a comment
There was a problem hiding this comment.
Thanks @Scott-Simmons - similarly to your last PR this is of very high quality. I do have a few suggestions around the single-line makefile commands but I am in general happy with this. I do not think all users will require it, but I do think that it can be a very good UX improvement for some users. Well done!
| TEST_DOCKERFILE := Dockerfile.testing | ||
| TEST_CONTAINER := tsfresh-test-container | ||
| # >= 3.9.2 ---> https://github.com/dask/distributed/issues/7956 | ||
| PYTHON_VERSIONS := "3.7.12 3.8.12 3.9.12 3.10.12 3.11.0" |
There was a problem hiding this comment.
Do we really need to specify also the patch version number? I do not want to create the need to update this file whenever there is a new python version coming out.
There was a problem hiding this comment.
I see all I need to do is specify major.minor like 3.10 and pyenv will interpret this as latest.
|
|
||
| # Tests `PYTHON_VERSIONS`, provided they are also | ||
| # specified in setup.cfg `envlist` | ||
| test-all-testenv: clean build-testenv |
There was a problem hiding this comment.
Why do you need to call clean?
There was a problem hiding this comment.
More sensible make layout which cleans after the tests are invoked in docker: 3ddad59
Calls to clean are required for local testing if test-all-testenv is run beforehand. Because those files will be owned by root and generated, it will brick the testing for local pytest and tox. see here:
However on reflection it makes more sense to move clean into executing after the docker tests run.
| # current context (e.g. global or local version of | ||
| # python set by pyenv, or the python version in | ||
| # the active virtualenv). | ||
| test-local: clean |
There was a problem hiding this comment.
Same question: why do you need to clean?
There was a problem hiding this comment.
Moved to cleaning within the docker testing target 3ddad59
| .. code:: | ||
|
|
||
| pytest | ||
| make test-local |
There was a problem hiding this comment.
With this, users will need to reinstall the pip packages for testing on very run of the test. I know, it should be quick as all packages are already there, but it does take some extra time nevertheless.
I would suggest to not add a makefile command for the local testing but to state that the users need to run pytest. The makefile command just replaced two commands and the installation command is explained on the page already anyways
There was a problem hiding this comment.
Removed all the one-line makefile commands 3ddad59
| clean: | ||
| rm -rf .tox build/ dist/ *.egg-info | ||
|
|
||
| install-formatters: |
There was a problem hiding this comment.
Why is this command needed and the one below? We have pre-commit for exactly this, or?
There was a problem hiding this comment.
Removed these targets 3ddad59
I was not aware that the precommit check runs locally with each commit, but I see that it is in the docs so we do not need this as you pointed out.
|
|
||
|
|
||
| `tox -r` will execute tests for the Python versions specified in `setup.cfg <https://github.com/blue-yonder/tsfresh/blob/1297c8ca5bd6f8f23b4de50e3f052fb4ec1307f8/setup.cfg>`_ using the `envlist` variable. For example, if `envlist` is set to `py37, py38`, the test suite will run for Python 3.7 and 3.8 on the local development platform, assuming the binaries for those versions are available locally. The exact Python microversions (e.g. `3.7.1` vs `3.7.2`) depend on what is installed on the local development machine. | ||
| This will execute tests for the Python versions specified in `setup.cfg <https://github.com/blue-yonder/tsfresh/blob/1297c8ca5bd6f8f23b4de50e3f052fb4ec1307f8/setup.cfg>`_ using the `envlist` variable. For example, if `envlist` is set to `py37, py38`, the test suite will run for Python 3.7 and 3.8 on the local development platform, assuming the binaries for those versions are available locally. The exact Python microversions (e.g. `3.7.1` vs `3.7.2`) depend on what is installed on the local development machine. |
There was a problem hiding this comment.
Sorry, did not spot this in the last PR: can you make sure the link points to the latest HEAD/main version of the file?
There was a problem hiding this comment.
Responded to feedback here 3ddad59
This might result in dead links in the future e.g. if we change the file and not a great way to trace them. If we pin to a specific commit even if the file changes, it will be more traceable as to what happened, instead of a 404.
|
|
||
| tox -r | ||
| make test-all-local | ||
|
|
There was a problem hiding this comment.
Similar to above: I am not a big fan of replacing Single-Line commands with a makefile command. This adds another layer of indirection "advanced" users can be annoyed about (they know what tox is, so they want to potentially change options etc) and "new" users copy-paste what is in here anyways, so it does not matter.
There was a problem hiding this comment.
Reverted the single line makefile commands + updated the docs 3ddad59
That makes sense. I was thinking that makefile is a useful place to act as a single point of entry for people new to the project to understand how to run it.
But since the docs fulfill that purpose, understandable why that is not desired. Will remove the syntactic sugar.
One advantage is that by encapsulating in a makefile, suppose tox -r evolves to tox -r -other_flag -another_flag etc... then using a makefile means the user only needs to think about make foobar instead of the flags. Without needing to update the docs. But this is probably not super crucial to have for the tsfresh project.
There was a problem hiding this comment.
I do not know for tox, but many tools allow to set default options on the pyproject or setup.cfg or their own config file. This is typically better, because then the tool can do its own "schema" migration. For example, we so this for pytest in the setup.cfg.
- Remove the python PATCH version from makefile - Move calls to `make clean` to be part of the docker test invokation instead of within local test commands - Removed the one line makefile commands (the docs have enough info on the development process so will not bother using `make` for these. - Updated hyperlink to point to main instead of specific commit sha
Scott-Simmons
left a comment
Scott-Simmons
left a comment
There was a problem hiding this comment.
Hey @nils-braun, have responded to the feedback. Let me know if there is anything else
2 participants
Related: #994.
Easiest to go commit by commit. Uses 3 "flows"
(1) Test locally on specific python version Is useful for quick iteration.
(2) Test locally on the discoverable python versions Is a useful sanity check for ensuring new development works on multiple versions of python.
(3) Test in dedicated testing environment with guaranteed availability of python versions Is a good final sanity check. Takes longer, but creates a reproducible environment to test off and is a good 'source of truth'.
1 Test locally on specific python version
Testing time for one version is ~1 min. Note this includes the speedup from cached packages
2 Test locally on the discoverable python versions
Testing time across versions 3.7-3.11 is ~4 mins, but note that this speedup includes the speedup from cached packages.
Also note that python 3.7 fails on my machine due to some idiosyncratic issue related to my setup
py37/lib/python3.7/site-packages/pip/_vendor/typing_extensions.py... getting same error as here but not using vscode. Not an issue for me personally but shows an example of why testing within an isolated testing environment could be a good way to ensure reproducible version targeting.3 Test in dedicated testing environment with guaranteed availability of python versions
Build time for testenv ~5 mins but this is a once-off build for the testenv.
Testing time across versions 3.7-->3.11 is ~6 mins.