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

Moving unit tests into dedicated unit_tests/ directory #2227

Merged
merged 2 commits into from
Sep 2, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 31, 2016

Also moving two _testing.py modules and updating the relevant imports.

As pre-work, removed all unit test imports (i.e. unit tests that imported from another unit test).

See https://gist.github.com/dhermes/a36abffe9d921755d5566c2cb3930be2 for the script I used to generate the 2nd commit. (No code was manually edited, all done by the script.)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 31, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Aug 31, 2016

@jonparrott What changes do I need to make to setup.py / MANIFEST.in to make sure Debian package maintainers are happy but also unit_tests/ doesn't end up installed in site-packages?

@theacodes
Copy link
Contributor

@dhermes just add:

graft unit_tests

to MANIFEST.in.

When doing an sdist, setuptools will include that directory in the tar.gz. sdists still use setup.py, so even though the folder is there, it won't be included unless find_packages finds it, which it shouldn't. If it does, use the second argument to ignore it: find_pacakages('.', exclude=('unit_tests')). Note the bdists (including wheels) won't include this folder. This is intentional.

Also, you probably could've kept the situations where tests import other tests by making unit_tests it's own python package (that's just not installed).

@dhermes
Copy link
Contributor Author

dhermes commented Aug 31, 2016

Also, you probably could've kept the situations where tests import other tests by making unit_tests it's own python package (that's just not installed).

That was mostly for good testing hygiene

@theacodes
Copy link
Contributor

SGTM.

@theacodes
Copy link
Contributor

This LGTM.

@tseaver
Copy link
Contributor

tseaver commented Aug 31, 2016

I really dislike moving the tests out of line: I believe they should be present in the installed software, on the "fly what you test, test what you fly" principle.

@theacodes
Copy link
Contributor

@tseaver I understand your concern, but it seems that several other major Python packages have tests in a separate directory. We're still providing them in the source distribution.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 31, 2016

@tseaver The "responsible" thing to do is to move to the best-in-class and actively maintained test-runner, rather than staying on the dead nose project.

I'm not entirely sure what "fly what you test, test what you fly" has to do with the location of the tests, or sure why it would matter where the tests exist, rather it should matter that they exist.

I don't have any interest in writing a test-runner framework. Provided we're using someone else's, we should do what they support. I can modify my script to go with the second option, which is a dedicated test/ dir (without an __init__.py) in each subpackage.

WDYT?

@tseaver
Copy link
Contributor

tseaver commented Sep 1, 2016

The "fly what you test" implies shipping the tests with all the distributions, which means not moving them out of the package directory.

I'm -0 about switching to py.test: I can live with that a lot more easily than moving the tests out-of-line.

@theacodes
Copy link
Contributor

@tseaver I can't think of a case where it makes a lot of sense for a binary distribution (wheel) to include tests, especially if we explicitly structure tests to be able to run against the installed library. The clear separation of the tests package from the installed package makes this more obvious - the tester doesn't have to wonder what's being tested.

That is to say, if there is a test at a/test_a.py that tests a.a, it's ambiguous (especially with nose) if the tests are exercising ./a/a.py or {env}/lib/python{x}.{y}/site-packages/a/a.py. By moving the tests to their own package, it's always explicit that the package-under-test is in site-packages.

@daspecster
Copy link
Contributor

Sorry, I'm also confused about how the "Fly as you test, test as you fly" principle applies in this case?
ISTM that means having and adding systems tests as we build which to me would be our CI?

That is to say, if there is a test at a/test_a.py that tests a.a, it's ambiguous (especially with nose) if the tests are exercising ./a/a.py or {env}/lib/python{x}.{y}/site-packages/a/a.py.

In all honesty, I've been burned a couple times by exactly what @jonparrott pointed out there, so I'm game for trying out solutions.

@tseaver
Copy link
Contributor

tseaver commented Sep 1, 2016

I want users who have installed a wheel to be able to test it without the sdist, e.g.:

  $ /path/to/virtualenv/bin/pip install google-cloud
  $ /path/to/virtualenv/bin/nosetests google.cloud

(or the py.test equvalent). This allows them to verify the installation is correct on their platform. Today, this works (assuming you can navigate borken dependencies yourself):

$ /path/to/virtualenv/bin/pip install googleapis-common-proto==1.2.0 # borken
$ /path/to/virtualenv/bin/pip install nose unittests2 gcloud
$ /path/to/virtualenv/bin/nosetests gcloud
$ /tmp/issue_2227/bin/nosetests gcloud
.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................SSSSSSSSSS....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 2207 tests in 0.546s

OK (SKIP=10)

@theacodes
Copy link
Contributor

@tseaver basically no other package does this. If that is a strong desire for the user then we should make it easy for them to use the tests in our source tree against an installed version (which is what all the other major packages I linked do), which is what I'm recommending we do.

@tseaver
Copy link
Contributor

tseaver commented Sep 2, 2016

@jonparrott AYKM? I can point to hundreds which ship the tests inline in the source package.

@jgeewax
Copy link
Contributor

jgeewax commented Sep 2, 2016

Putting my preferences aside, it looks to me like we have two possible scenarios in the future:

  1. Some standard body says "thou shalt put your tests in site-packages"
  2. Some standard body says "thou shalt not put your tests in site-packages"

If we presume (1) and (2) happens instead, following the standard would mean that we break people who assume they can still type nosetests google.cloud. If we presume (2) and (1) happens instead, we add a feature to meet the standard...

Considering that we both have examples to point to, and a vote today among this group would end up with (2, don't ship tests), and it's the more future-proof option, it seems like we should just not put tests in site-packages and put it down as work to do should a standard be ratified.

Cool?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 2, 2016

@tseaver I'm going with consensus here and merging. We can revisit / we can always put these back. I just don't want to block the other work I'm doing to move to py.test / namespace changes.

@dhermes dhermes merged commit 192023d into googleapis:master Sep 2, 2016
@dhermes dhermes deleted the move-unit-tests branch September 2, 2016 17:43
@dhermes dhermes mentioned this pull request Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants