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

test command prevents other commands from running #850

Closed
scop opened this Issue Nov 17, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@scop
Contributor

scop commented Nov 17, 2016

The test command seems to prevent other commands from running. Looks like this issue was introduced in 18.4; in 18.3.2 it doesn't occur. Using https://github.com/pypa/sampleproject:

Setup:

$ pyvenv-3.5 venv
$ source venv/bin/activate

With setuptools 18.3.2, both "setup.by build test" and "setup.py test build" run the specified commands in the expected order:

$ pip install setuptools==18.3.2
Requirement already satisfied (use --upgrade to upgrade): setuptools==18.3.2 in ./venv/lib/python3.5/site-packages
$ python setup.py build test
running build
[...]
running test
[...]
$ python setup.py test build
running test
[...]
running build
[...]

...but with 18.4 (and up to 28.8.0), "setup.py build test" runs both build and test, but "setup.py test build" does not run build at all.

$ pip install setuptools==18.4
[...]
Successfully installed setuptools-18.4
$ python setup.py build test
running build
[...]
running test
[...]
$ python setup.py test build
running test
[...]
# note, "running build" does not appear

While fiddling with the build and test commands this way may be a bit silly, the issue exists with other commands as well. Initially I thought this was a flaw in the flake8 command and reported this there, see https://gitlab.com/pycqa/flake8/issues/260. With flake8, the test command behaves differently/worse than with build (with which "build test" works); "flake8 test" runs flake8 but not test, and "test flake8" runs test, not flake8.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Nov 17, 2016

For reference, https://gitlab.com/pycqa/flake8/blob/master/src/flake8/main/setuptools_command.py is the source for the setuptools flake8 command. There may still be a bug there.

@jaraco

This comment has been minimized.

Member

jaraco commented Dec 6, 2016

Here are the differences between 18.3.2 and 18.4. It's not obvious to me how these changes are causing the swallowing of other commands, but given that the changes are largely related to the handling of command-line arguments, it seems entirely plausible that there's an error in there. Those changes were indicated as relating to #446, which suggests that perhaps the tests weren't even being run at all before.

@scop, would you be willing to dive in and identify the underlying cause and maybe even put together a patch with possibly a test?

@scop

This comment has been minimized.

Contributor

scop commented Dec 7, 2016

I did a quick bisect session, and the issue was introduced in commit 3491076. Another finding is that the issue does not seem to happen in --dry-run mode (not even with the current master). Anyway, I found unittest.main does a sys.exit by default, so I think that's clearly why test build ends at test. #873 addresses that. It doesn't affect the original flake8 issue I ran into though, I'll look into it separately next.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 7, 2016

@scop I suspect Flake8 has a similar issue (using sys.exit)

@scop

This comment has been minimized.

Contributor

scop commented Dec 7, 2016

@sigmavirus24 similar yes, it raises a SystemExit if it found any issues. But it does not do that (nor exit otherwise) if no flake8 issues are found. I guess what this means or if anything should be done to it boils down to expected behavior for chained setuptools commands; if running e.g. flake8 test and flake8 fails for some reason, should test be run at all? If not, I think flake8 is ok as is. If yes, some changes in flake8 are needed. Thoughts? Anyway I didn't find a clear way to communicate failure to setuptools from setuptools plugins.

@scop

This comment has been minimized.

Contributor

scop commented Dec 7, 2016

@sigmavirus24 scratch that, the SystemExit in flake8 is always raised, even if there are no issues found (unless force-skipped with exit_zero). So I think something should be done to flake8 in any case; exactly what depends on my question on the expected behavior...

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 7, 2016

Yeah, I don't know what the right behaviour is for setuptools commands honest, or how we communicate failure to setuptools. I blame distutils for that though, mainly because setuptools commands are just extensions of distutils commands and most of the logic still resides there.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Dec 7, 2016

So looking at https://github.com/python/cpython/blob/master/Lib/distutils/cmd.py I don't see a way to communicate failure.

@jaraco jaraco closed this in 53b47e1 Dec 8, 2016

jaraco added a commit that referenced this issue Dec 8, 2016

Merge pull request #873 from scop/issue-850
Tell unittest.main not to exit, fixes #850.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment