Use Tox for easily testing with multiple Python interpreters #372
Conversation
|
💖 Thanks for opening your first pull request here! 💖 |
|
I've changed the base branch of this pull request from
Keep in mind that after we've completed the port to Python 3, we'll end supporting Python 2. Right now whipper is unusable under Python 3. So, at the end, this would be used only to test whipper under different Python 3 versions. Let's hear what's the opinion of @Freso about this one. |
now the minimum version is the same one as the one used in travis
Signed-off-by: Pau Ruiz i Safont <unduthegun@gmail.com>
There was a problem hiding this comment.
Overall, I like the idea of using tox (or something else) to help with testing. Thanks for your work on this!
One thing I find lacking is explanations for your choices in what you've done. Other than a commit title, your commits have no additional information. Feel free to write short stories in them so it's possible to tell in 5 years what you were thinking as you made the changes. Most of my comments on/questions to the actual code changes below should have been/be answered by consulting the relevant commit message.
You should also really split up your commits more, please. E.g., 1f85298 both moves requirements.txt's information into setup.py, as expected from its title: "Incorporate dependencies in setup.py"—but it also does a number of changes to .travis.yml and seemingly unrelated changes to README.md (also some obviously related changes, those are fine). This one commit should like be at least 2 separate commits.
Also, c58630e is an empty commit(?) and 84f1077 needs to get signed-off.
(I hope this doesn't scare you off! 🙈)
| @@ -1,30 +1,31 @@ | |||
| dist: xenial | |||
There was a problem hiding this comment.
Why? Up until now we've generally tried to support as old versions of things as possible, and I didn't see a change in that mentioned anywhere, so switching to a newer distro version should probably be explained/argued for in more detail.
There was a problem hiding this comment.
I updated to Xenial because the version of gobject-introspection packed in Trusty was incompatible with the version of PyGObject that pip pulls with the current restrictions.
I could make it go back to manually install the os-packaged versions, but then we lose the poer of having a single source for dependency definition. I could look into how to define versions of libraries to install depending on a library version in the OS.
Since that is not trivial I preferred to update from trusty to xenial, as trusty is going out of support on April. I also didn't occur to me this was also testing the package in a specifically old version on purpose, I though it was only used to run the unit tests. Making the tests to pass more important than the environment this happened on.
| 'PyGObject', | ||
| 'requests'] | ||
| TEST_DEPS = ['Twisted'] | ||
| LINT_DEPS = ['flake8'] |
There was a problem hiding this comment.
What's the point of putting these in "constants" like this rather than just adding them directly to the _requires? It puts the data further away from where it's used, and AFAICT it's not needed for reuse anywhere else either.
There was a problem hiding this comment.
I think it's a tradeoff of locality against structural clarity. More complex rules of versions can be added later on, and it seem they might be needed in some cases. n the end it's a matter of preference.
| mutagen | ||
| pycdio>0.20 | ||
| PyGObject | ||
| requests |
There was a problem hiding this comment.
It might be an idea to replace the with a single line -e .? See https://caremad.io/posts/2013/07/setup-vs-requirement/ for some thoughts.
There was a problem hiding this comment.
That would be good for not breaking compatibility with existing users. I do not like the idea of keeping a requirements.txt that is not used for doing a deployment with a particular version of each dependency.
In the article "application" refers to a web-app, where the environment can be controlled and the versions of the dependencies can be frozen. For terminal applications this might not be the case since users might end up having an environment where several applications share dependencies.
Progress is happening in the tooling to avoid using shared environments (pipx) but this is not true for most users.
I'll change it to -e . unless you tell me otherwise
|
|
||
| `pip install -r requirements.txt` | ||
| `python2 -m pip install -e .` |
There was a problem hiding this comment.
Why the -m invocation instead of calling pip directly?
There was a problem hiding this comment.
sometime pip is not on the user's path, or it might be python3's pip.
This means that novice users cipypasting the instruction will have a lesser chance of encountering situations where things subtly break, at least in my opinion. (I can change if you want me to)
| script: | ||
| - if [ ! "$FLAKE8" = true ]; then python -m unittest discover; fi | ||
| - if [ "$FLAKE8" = true ]; then flake8 --benchmark --statistics; fi | ||
| script: tox |
There was a problem hiding this comment.
This approach fails to take advantage of Travis' parallel building and that the whole of the test will either fail or pass, rather than (currently) code style check and unit tests pass or fail independently of each other.
There was a problem hiding this comment.
I agree. I don't think that linting takes so much time that the shaving of time is substantial, it saves resources too as the installation of dependencies only happens once.
In any case, once testing for several python interpreters is enabled in this file these will happen on parallel.
I can look on how to run linting and unittesting in parallel
|
|
||
| ```bash | ||
| python2 -m whipper -h | ||
| python2 -m pip install --user -e . |
There was a problem hiding this comment.
You're changing the meaning and purpose of this section. Is this intentional? Also, you're not installing whipper here (as the description preceding the command claims), only its dependencies.
There was a problem hiding this comment.
It's intentional, with the changes centered on building whipper as a package it makes sense to try to use it as much as possible to depend more on the dependencies being installed by pip.
The command does install the package on live mode. This means that any change in the code is instantly picked up as part of the package. This is done through symlinks, instead of the usual copying of files that happens when installing files.
This is something that would not be used in the long run, as tox would replace this workflow: it can be change to run a specific test with a specific python interpreter. I would need to check how tests are organized to make this happen, though.
| - sudo make install | ||
| - cd .. | ||
| - popd | ||
| # |
There was a problem hiding this comment.
I don't know why it was added either
| envlist = | ||
| py{27,34,35,36,37,py,py3}-unit, | ||
| py{27,34,35,36,37,py,py3}-lint, | ||
| skip_missing_interpreters = True |
There was a problem hiding this comment.
We're not supporting Python 3 (yet) or PyPy (yet?), so there's no reason to run or care for tests for those. Wouldn't it be better to just not list them rather than ignore them if non-existent?
There was a problem hiding this comment.
On travis this are not run because only the environment is set up for python 2.7.
For local dev the tests never get tun on pypy nor py3 because the package is dfefined as a python 2.7 only package.
I'll remove the line, I don't think it hurts knowing this will be useful for the porting effort.
| - cd .. | ||
|
|
||
| # Installing | ||
| - sudo python setup.py install |
There was a problem hiding this comment.
We probably still want to test whether installing works at all. We don't have a lot of functionality tests as it is (do we have any?), only unit tests, and this seems like a fairly basic one to have…
There was a problem hiding this comment.
This command is run by tox as part of the process it uses to run commands.
It generate a source distribution of the package, a virtual environment and then installs the package onto the virtual environment.
Excerpt from a tox command:
$ tox -e py27-lint
GLOB sdist-make: /home/pau/code/tui/whipper/setup.py
py27-lint create: /home/pau/code/tui/whipper/.tox/py27-lint
py27-lint inst: /home/pau/code/tui/whipper/.tox/.tmp/package/1/whipper-0.7.4.dev50+gcb3515c.zip
py27-lint installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,certifi==2018.11.29,chardet==3.0.4,configparser==3.7.1,entrypoints==0.3,enum34==1.1.6,flake8==3.7.5,functools32==3.2.3.post2,idna==2.8,mccabe==0.6.1,musicbrainzngs==0.6,mutagen==1.42.0,pycairo==1.18.0,pycdio==2.0.0,pycodestyle==2.5.0,pyflakes==2.1.0,PyGObject==3.30.4,requests==2.21.0,typing==3.6.6,urllib3==1.24.1,whipper==0.7.4.dev50+gcb3515c
|
Is this pull request still alive and relevant (#411)? |
|
Not really. #433 didn’t do what this PR set out to do (namely, use As for whether it’s still “alive and relevant"… 🤷♂️ I think moving to |
|
@undu Thanks for the pull request, as it seems to be stale I'm closing it. If I'm wrong feel free to reopen it or to submit a new one. |
Tox makes it very easy to test packages in customized virtual environments.
I thought it would be useful to be able to test the package in preparation for python 3 compatibility: #78
Right now there are 2 tests failing: one fails because some url parameters are out of order and the other one I need to investigate further, seems like the environment cannot access cdrdao
Let me know if you like this approach so I can continue working on this. In the end if the developers don't use it, there's not much point in doing this.