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

python buildPythonPackage: check and run correct test command #11715

Closed
wants to merge 4 commits into from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Dec 14, 2015

The default command for running tests is python setup.py test. Many
packages use a test runner that needs to be invoked, e.g. nosetests or
py.test. It would be nice if buildPythonPackage automatically
determines which command to run.

We can check in the buildInputs whether nose or pytest is
included, and if so, do either 1) or 2)

  1. Run the appropriate command, nosetests or py.test.
  2. Apply a patch to enable python setup.py test. For
    [py.test](http://pytest.org/latest/goodpractises.html#integrating-with-
    setuptools-python-setup-py-test-pytest-runner) and
    nose

Option 2) could get complicated/messy and therefore I'm in favor of
option 1).

This commit implements option 1.


Reference to #1819


Furthermore, comments in buildPythonPackage are moved out of the arguments, and explicit calls to the test runners that are not necessary anymore are removed.

The default command for running tests is `python setup.py test`.  Many
packages use a test runner that needs to be invoked, e.g. `nosetests` or
`py.test`. It would be nice if `buildPythonPackage` automatically
determines which command to run.

We can check in the `buildInputs` whether `nose` or `pytest` is
included, and if so, do either 1) or 2)

1. Run the appropriate command, `nosetests` or `py.test`.
2. Apply a patch to enable `python setup.py test`. For
[py.test](http://pytest.org/latest/goodpractises.html#integrating-with-
setuptools-python-setup-py-test-pytest-runner) and
[nose](http://nose.readthedocs.org/en/latest/api/commands.html)

Option 2) could get complicated/messy and therefore I'm in favor of
option 1).

This commit implements option 1.

---
Reference to NixOS#1819
If I am correct any future modifications to the comments wouldn't
require a rebuild.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @domenkozar, @civodul and @offlinehacker to be potential reviewers

@FRidh
Copy link
Member Author

FRidh commented Dec 14, 2015

closes #11590 and we can mark one item on the list

configurePhase = attrs.configurePhase or ''
runHook preConfigure

# patch python interpreter to write null timestamps when compiling python files
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this comment removed?

EDIT oh nevermind, you're putting it outside of bash, which seems like a good thing

Copy link
Member Author

Choose a reason for hiding this comment

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

its moved a couple of lines up.

nosetests
else
${python.interpreter} nix_run_setup.py test
fi
Copy link
Member

Choose a reason for hiding this comment

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

I know I sometimes use py.test and call it within the python setup.py test after some set-up have been done (something like being sure that database schema are deployed). Would not that change break such behavior ? I have no idea how much this kind of setup is used.

@FRidh
Copy link
Member Author

FRidh commented Dec 14, 2015

@lancelotsix indeed, I was just about to write a note on that.
If you do invoke your test runner through python setup.py test and do something special before the test runner runs, then that is not done.
It's a good example, and it would indeed also be ugly if we have to set the checkPhase manually to python setup.py test. But how common is this? Actually, in your example I think the setup should be done by the test in the proper phase.

@lsix
Copy link
Member

lsix commented Dec 14, 2015

That's my underlying question : how common is that ? I do it this way in some projects because running the entire migration history takes a lot of time when my cleanup phases can only make sure the tables are empty.

Anyway, this not even that much a good example because unless I test against sqlite, I need a sql server running with proper credential set-up, which makes tests non reproducible since the test phase do not declare a dependency to a running service.

I also use either the wrapper in the setup.py file or the setup.cfg file to add options when calling py.test. This can be used to activate things such as pytest-pep8. But once again, I am not sure how important this is from the nixpkgs perspective. That's an open question ;)

@FRidh
Copy link
Member Author

FRidh commented Dec 14, 2015

Indeed, so we either

  1. try and find out how common this is before merging,
  2. or we find out when we encounter it and it causes problems

The latter we might be able to find out by doing a full rebuild.

Most packages that have pytest as buildInput don't set the checkPhase but do succeed building.
This can mean I think two things:

  1. in their setup.py they call their test suite properly.
  2. the wrong directory is called, no tests are found, and the checkPhase succeeds.

I just randomly checked the source of a couple of packages, and quite often I've encountered also the
test_suite argument to setup.

So actually I'm thinking now of closing this PR and keeping it the way it was.

cc @domenkozar @garbas

@FRidh FRidh closed this Dec 15, 2015
@FRidh FRidh deleted the python-interpreter branch March 4, 2017 07:42
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.

4 participants