Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tests #104

Merged
merged 10 commits into from
Aug 30, 2016
Merged

Update tests #104

merged 10 commits into from
Aug 30, 2016

Conversation

medecau
Copy link
Contributor

@medecau medecau commented Aug 1, 2016

This pull-request:

  • Drops test support for python 2.6.
    • too many errors related to pylint.
    • decided to keep pylint and drop python 2.6.
  • Adds requests version variations to tox config similar to travis.
    • tox -l for a list of all environments included
    • detox to run tests in parallel
  • Moves test dependencies into tox.
  • Adds a new environment for code style conformance validation.
    • pycodestyle
    • fixed some code style errors
  • Changes travis configuration to use tox.
  • Changes travis configuration to use containers.

@john-kurkowski
Copy link
Owner

Thank you for your contribution. Can I ask, what prompted this? How is this better?

@@ -1,16 +1,27 @@
sudo: false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Travis's default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True.
According to the docs this is the default for all repos activated since 2015. And was already the default behaviour for tldextract.

Should I remove it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, you're right, explicit > implicit

medecau added 3 commits August 2, 2016 08:39
*-current environment is for the latest requests from pypi
removed lingering python 2.6 dependency definitions
multiple versions of python are tested with the latest requests lib
python 3.5 is tested on a few requests variations
@john-kurkowski
Copy link
Owner

Sorry for the huge delay in response.

Can I ask, what prompted this? How is this better?

Bump. I'm still wondering what prompted the major changes in this PR.

I've been happy with the test setup and developer ergonomics of the project. Aren't the instructions in the README's Contribute section pretty standard for Python developers? I.e. the use of requirements.txt? A single command to run the test suite for the current Python version?

Do developers need to run all environment permutations all the time? Is Tox the new hotness?

@medecau
Copy link
Contributor Author

medecau commented Aug 28, 2016

What prompted this?

There wasn’t a single cause for the changes or I can no longer remember what it was.
But I made the changes. I think they are good and stand by them.

How is this better?

Before remote tests were more exhaustive than local tests. i.e.: local tests could pass while remote tests failed. With these changes the steps for local tests are the same as for remote tests.
Devs are able to test on all relevant environments before pushing to remote. And during development it is possible to run only one set of tests for the specific environment the changes affect.
When Python 3.6 is released it will be added to tox.ini along with one line to .travis.yml.
There is an environment solely for code style. In the future you can add environments for doc style and security.

Do developers need to run all environment permutations all the time?

No. Developers can run tests against only one environment or a small set of environments. e.g.: tox -e py35-requests-2.1.0
More info on tox docs.

Is Tox the new hotness?

Probably. Depends on the definition of ‘new’ and ‘hotness’. I’ll have to leave this one up to you.
Here are a few commits of tox being added to some influential projects:

@medecau
Copy link
Contributor Author

medecau commented Aug 28, 2016

After posting the previous comment I noticed it included an appeal to authority.

Let me try with my personal experience. Anecdotal as it may be.

I've been using tox for about the same time as I've been using py.test. A few years now. I don’t consider it new. I find it extremely usefull. Even if it may be hot I find it to be usefully hot. Using tox on a project gives me the guarantee that whatever changes I make do not brake the project at any level that tox is able to test. That is, the project builds, it installs, dependencies do not brake, and tests pass. I have not explored running the test on different OSs. For now I leave that to travis-ci.

I was also going to list a bunch of tox features and guarantees that tox provides but decided against it because the tox wiki does a much better job than I could possibly do. See tox docs.

I also remembered that a similar clarification was requested on a different project. See transitions issue #123

- pip install -r requirements.txt
- pip install --upgrade --force-reinstall requests==$REQUESTS_VERSION
script: py.test
- TOXENV=py27-requests-current
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: does Travis have any syntax for syncing this list of envs to run with tox.ini? Too bad it has to be in 2 places. But it's not too bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could run tox without any TOXENV vars but that would collapse all the environments into a single one. Tox would then run all permutations but it would be harder to which tests failed from the travis interface.

By defining the different environments it forces travis to display the results for the different environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the above means in usability is that currently if a test fails in one of the environments travis will display the failed environment from the UI. e.g.: https://travis-ci.org/john-kurkowski/tldextract/builds/93708810

If no TOXENV vars are defined then tox will run all permutations under a single build, hiding the details of which environment failed deep in the logs. Tox makes it easy to see which envs failed but not as nice as travis web UI.

@john-kurkowski
Copy link
Owner

Thank you very much for the explanation. I like seeing the other examples and discussions. From this PR's description alone, I wasn't sure what to make of the increased reliance on Tox.

Before remote tests were more exhaustive than local tests.

I hadn't realized that. The way this PR smooths that over is nice.

And no argument against PEP-8 compliance from me. I was looking for that anyway.

So, this new direction sounds good. I'll bite.

@john-kurkowski
Copy link
Owner

I'm trying to check out your branch locally, and try the new test suite, as if I were a newb. The README's Contribute section doesn't seem to apply any more. There's no requirements.txt. I get tox: command not found. Can you update README.md?

Hopefully that section can remain as succinct as it is. It doesn't need to be nearly as detailed as Django's addition of tox.

@medecau
Copy link
Contributor Author

medecau commented Aug 29, 2016

You may have received a notification for an update on this PR. Please ignore that.

I went back to the README and am trying to figure out the steps for contribution.
The current steps for installation are not the steps for contribution.

For contribution I would simply mention pull-requests as the way to go. And request that contributors validate that all tests pass before publishing.

@john-kurkowski
Copy link
Owner

Oh, I'm not asking you to add more info to that section than what's already there. Just to update the incorrect info.

In particular, I'm trying to run your PR locally, but the instructions in that section don't work any more.

@john-kurkowski
Copy link
Owner

john-kurkowski commented Aug 30, 2016

Whew, very simple instructions. Tried it out locally. Works great. Thank you for your contribution and for seeing this through!

@john-kurkowski john-kurkowski merged commit fa2b093 into john-kurkowski:master Aug 30, 2016
@medecau
Copy link
Contributor Author

medecau commented Aug 30, 2016

You are welcome!

Thanks for taking the time to review the pull-request.

@medecau medecau deleted the update-tests branch August 30, 2016 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants