Opened 4 years ago

Closed 3 years ago

#7228 enhancement closed fixed (fixed)

Add tox configuration file

Reported by: Adi Roiban Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/add-tox-file-7228-4
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban, glyph

Description

Add a tox configuration file so developers can use tox to run all the automated tests and checks

Attachments (4)

add-tox-file-7228.patch (1.1 KB) - added by Chris Wolfe 4 years ago.
add-tox-file-7228-2.patch (1.1 KB) - added by Chris Wolfe 4 years ago.
include py33
add-tox-file-7228-3.patch (1.8 KB) - added by Chris Wolfe 3 years ago.
7228-adi-1.diff (1.8 KB) - added by Adi Roiban 3 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 4 years ago by Chris Wolfe

I'd think that a fair amount of the configuration could be lifted from what is being used by twisted/Klein:

https://github.com/twisted/klein/blob/master/tox.ini

comment:2 Changed 4 years ago by Chris Wolfe

Closed duplicate ticket #7677

Changed 4 years ago by Chris Wolfe

Attachment: add-tox-file-7228.patch added

comment:3 Changed 4 years ago by Chris Wolfe

A few questions:

1) What is the goal of the tox file? Are we wanting to make it easier for both users and buildbots? Or is the file mainly intended for one or the other? 2) Should the tox file be set up to have environments to exhaustively test all of the different configurations that a buildbot might provide or should it only contain testenvs for a simple base configuration with zope.interface. 3) How can/should we test the tox file?

I've attached a tox file that i have been using to run tests - it is by no means exhaustive (and not meant to close the ticket), but it might help in getting things started.

Last edited 4 years ago by Chris Wolfe (previous) (diff)

Changed 4 years ago by Chris Wolfe

Attachment: add-tox-file-7228-2.patch added

include py33

comment:4 Changed 4 years ago by Chris Wolfe

And - I left out the py33 env. I've included it with patch add-tox-file-7228-2.patch

comment:5 Changed 4 years ago by Adi Roiban

From my point of view, the main benefit of tox is to allow non-commiters to run as much test as possible on their local machine, before sending a patch.

This should include py2.7, py3.3, with and without modules, pyflakes, pydoctor, twistedcher...etc.

It would we nice to use the same tox environments/targets to execute task on a buildslave... but for a start I don't think this is possible, as twisted builders are highly customized to cope with old errors in pyflakes and twistedcheker.

comment:6 Changed 4 years ago by Chris Wolfe

The more I think about it, the more I dislike how I've written the tox.ini patch file that I attached to the ticket (add-tox-file-7228.patch and add-tox-file-7228-2.patch).

Tox is intended to run tests in completely isolated environments. My file basically just runs all of the tests inside of the toxinidir, i.e. the project's root directory. This has the unfortunate effect of leaving test artifacts throughout project.

I'm working on a similar issue with another project so I should have something better that I can upload soon.

Last edited 4 years ago by Chris Wolfe (previous) (diff)

comment:7 Changed 4 years ago by Chris Wolfe

Owner: changed from Adi Roiban to Chris Wolfe

Sorry I left this in limbo and forgot about it. I'll start working on it again now.

Changed 3 years ago by Chris Wolfe

Attachment: add-tox-file-7228-3.patch added

comment:8 Changed 3 years ago by Chris Wolfe

Keywords: review added
Owner: Chris Wolfe deleted

This isn't ready for merge, but I'd like to gather a bit of feedback regarding the patch in its current state.

add-tox-file-7228-3.patch is setup to mimic several of the build configurations that the buildbot matrix provides.It runs tests for python2.6, 2.7, and 3.3, provided each interpreter is installed. It also runs twistedchecker, pyflakes, and builds both the api and narrative docs.

This would definitely need new documentation before merging.

  1. Does it look like any builders are missing that *should* be present?
  2. Would it be helpful to have environments with optional dependencies explicitly installed; i.e. testenvs where the tls, dev, conch... options are installed?

Thanks!

comment:9 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/add-tox-file-7228

(In [43972]) Branching to add-tox-file-7228.

comment:10 Changed 3 years ago by Adi Roiban

thanks for working on this.

I think that we should use '.[dev]' for installing dependencies.... but .dev is not yet ready for Python3

for without_modules and gc_select I think that is fine to use a single python version... gc_select is so slow that I don't know if anyone will run in on its computer.

Python 3.3 test runner also accept input arguments.

Instead of using .tox maybe we can use build folder, as it is already in the ignore list.


It needs documentation but is a big step forward

Now I can do this to run the same tests on all environment:

tox -e py26,py27,py33 twisted.test.test_tcp.FactoryTestCase

I have attached my version of the tox file which uses .[dev] and also allow sending arguments to py33

Thanks!

Changed 3 years ago by Adi Roiban

Attachment: 7228-adi-1.diff added

comment:11 Changed 3 years ago by Glyph

Author: adiroibanadiroiban, glyph
Branch: branches/add-tox-file-7228branches/add-tox-file-7228-2

(In [44005]) Branching to add-tox-file-7228-2.

comment:12 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Chris Wolfe

Hi adiroiban, herrwolfe. Thanks for your work on this.

  1. I'm seeing lots of failures. Particularly on twisted.python.test.test_release.SphinxBuilderTests. Am I just seeing these failures here for the first time because the buildbots don't ever install sphinx, and it's broken for real?
  2. I think that the patch got mangled somewhat when adiroiban updated it, see the defaselect_gc and narrative_docsult environments referenced in the basepython configuration. This causes InterpreterNotFound on several environments.
  3. select_gc is a misnomer, because the select reactor is never specified to trial. For some time now we have used a better reactor by default.
  4. The without_modules environment should actually not have the modules installed in its environment, not just specify the --without-module argument to trial. Similarly, most of these environments should not have development dependencies installed. (Maybe we can work around the 1st feedback point by starting off with not installing them into any of the environments where tests are actually run, and adding that on later.)
  5. Is there a ticket for fixing .[dev] for py3? If not, please file one and mention this. If possible, let's fix that first so we don't need duplicate configuration for dependencies. (It seems to me the whole point of using tox at all is to centralize our dependency declarations so they can be automatically dealt with.)

Thanks again. I'm looking forward to seeing this updated.

comment:13 Changed 3 years ago by Chris Wolfe

Thank you for your review, Glyph.

I've opened ticket #7807 to address the issue with python3 and extras_require options.

comment:14 Changed 3 years ago by Adi Roiban

my patch is broken... I should have just attach my version of tox.ini which I use for 2.6 2.7 and 3.3 for reference.

herrwolfe sorry for stepping over your toes.

comment:15 in reply to:  12 Changed 3 years ago by Chris Wolfe

adiroiban - no worries! Thanks for your comments

glyph - thanks for your comments, I have a few follow up questions:

  1. I'm seeing lots of failures. Particularly on twisted.python.test.test_release.SphinxBuilderTests. Am I just seeing these failures here for the first time because the buildbots don't ever install sphinx, and it's broken for real?

test_build and test_main look to be failing for me as well. I'll file tickets for them.

  1. select_gc is a misnomer, because the select reactor is never specified to trial. For some time now we have used a better reactor by default.
  2. The without_modules environment should actually not have the modules installed in its environment, not just specify the --without-module argument to trial.

Regarding points 3 and 4, it seems like neither of these options would provide much utility to someone running tox. Should they just be left out of the file?

Similarly, most of these environments should not have development dependencies installed. (Maybe we can work around the 1st feedback point by starting off with not installing them into any of the environments where tests are actually run, and adding that on later.)

I'll continue working on a version of the file where the default env only has zope.interface installed. It seems like there should be several other test envs that have dependencies that enable certain tests to run; for example sphinx. To run tests that need sphinx an env like, py27-with-sphinx should exist.

The only concern I have with this solution is that there are a relatively large number of dependencies that are optional but are needed to be able to run tests; the adbapi tests are an example.

comment:16 Changed 3 years ago by Chris Wolfe

Actually, it doesn't look like the sphinx release tests are being skipped on the builders where sphinx is installed, nor are those tests failing. Example: https://buildbot.twistedmatrix.com/builders/py-select-gc/builds/4124/steps/trial/logs/stdio. I'm not sure why they are failing locally.

comment:17 Changed 3 years ago by Adi Roiban

I am ok with renaming the select_gc tox builder or skiling

I don't use it much as it is slow... but I do use it to track unhandled errors and it is of great use... but is not hard to add -gc argument to trial


I found the without_module test important and would like to see it in tox.

For example, while working on tests using pycryto, since my venv has pycrypto I would see that my test will pass but then the without_modules buildbot builder will fail and to fix that I had to add an ugly skip.

without dependencies installed is hard to work on conch ssh


I think that is important to have as many modules installed as possible so that you can run as many tests as possible.

for example while working on conch ssh I don't run the py2.6 tests as I failed to compile pycrypto for 2.6 on my Ubuntu 14.04 system

comment:18 Changed 3 years ago by hawkowl

Author: adiroiban, glyphhawkowl, adiroiban, glyph
Branch: branches/add-tox-file-7228-2branches/add-tox-file-7228-3

(In [45501]) Branching to add-tox-file-7228-3.

comment:19 Changed 3 years ago by hawkowl

Keywords: review added
Owner: Chris Wolfe deleted

I've done some work on this, because not having tox for pyflakes or whatever makes me mad.

  • There's no select_gc ones. You can use posargs if you want to do it.
  • Tests on Py27, Py33, Py34.
  • 'nomodules' actually has no modules installed, rather than just pretending.
  • Introduces a .coveragerc because we should have one.
  • Fixes up some sphinx things that are broken.
  • Doesn't install it, so it's rather quick.

Builders spun, please review.

comment:20 Changed 3 years ago by hawkowl

Results:

ERROR:   py27-tests: commands failed
ERROR:   py27-nomodules: commands failed
  py33-tests: commands succeeded
  py33-nomodules: commands succeeded
  py34-tests: commands succeeded
  py34-nomodules: commands succeeded
ERROR:   py27-coverage: commands failed
  py33-coverage: commands succeeded
  py34-coverage: commands succeeded
ERROR:   pyflakes: commands failed
ERROR:   twistedchecker: commands failed
  apidocs: commands succeeded
  narrativedocs: commands succeeded

The py27 failures are because of a words test that requires stable dict ordering. Tox randomises this, and this failure is visible in the PyPy builders too.

comment:21 Changed 3 years ago by Adi Roiban

Thanks for working on this!

Is there a ticket for the tests which fail on PyPy?

Can we turn off tox randomization ?

Maybe we can include a note about tox support in the README - Unit test section. What do you think?

For narrativedocs maybe we should run the with Fail on warning so that the build will fail if there are any errors/warnings.

Is there any use to add the twistedchecker env ? It will fail in 99% of the cases as the diff feature is not working :(

This branch also adds the coverage.rc so maybe the README file should also talk about the coverage support.

Leaving this in the review queue so that other dev can check it.

Thanks again!

comment:22 Changed 3 years ago by hawkowl

This should be semi-fixed with #8015.

comment:23 Changed 3 years ago by hawkowl

Branch: branches/add-tox-file-7228-3branches/add-tox-file-7228-4

(In [45650]) Branching to add-tox-file-7228-4.

comment:24 Changed 3 years ago by hawkowl

The randomisation is now no longer an issue.

comment:25 Changed 3 years ago by Chris Wolfe

Keywords: review removed
Owner: set to hawkowl

Hi Hawkowl,

Thanks for your work on this! I've hand-tested each of the environments and commands and they all seem to work as expected, both with their default and positional arguments.

  1. Are the changes to the logging docs related to this ticket or are they just meant to be a quick fix? It is a nit, but if they aren't related to tox, I think they belong in a separate ticket.
  2. Why isn't PyPy included?
  3. I'm not sure if having tox qualifies as being a feature, but I think other devs would be happy to see it in the release notes :-). This could mean changing the newsfile to a feature or adding text to this one (though I'm not sure if misc tickets allow for text).
  4. It looks like you've done nomodules correctly (where I failed :-)). No development dependencies are installed in the nomodules environments.

Adiroiban: I might be wrong, but it looks like sphinx-build is using the -W option that turns errors into failures.

narrativedocs: sphinx-build -aW -b html -d {toxinidir}/docs/_build docs {toxinidir}/docs/_build/

Thanks again for all of you work! Please address the question above and resubmit for review.

comment:26 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi herrwolfe, thanks for the review.

  1. Without them, the docs tox command fails; but I will fix that in another ticket.
  2. PyPy is not a supported platform -- so in this case, it would only cause confusion to contributors, as the pypy test would always be red. We could look into this later, probably.
  3. I'd rather not have it in the NEWS file... process improvement is a bit different. We should announce it on the ML and change the development docs to suit, though.

Please review.

comment:27 Changed 3 years ago by Chris Wolfe

Keywords: review removed

Hi Hawkowl,

  1. Ok, thanks!
  2. Whoops, sorry about that, I had thought that pypy was a supported platform. :-)
  3. That makes sense - with tox not really having an effect on consumers of Twisted, it doesn't really need a mention in the release notes.

This looks good to me. Please land if you're ok with my review. If not, please put it up again to get another opinion.

Thanks for your work on this! herrwolfe

comment:28 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [45685]) Merge add-tox-file-7228-4: Add a tox.ini configuration file

Authors: herrwolfe, adiroiban, hawkowl Reviewers: glyph, herrwolfe, adiroiban Fixes: #7228

Note: See TracTickets for help on using tickets.