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

Include tests in sdist #3667

Merged
merged 7 commits into from
Jan 28, 2020
Merged

Conversation

diego-plan9
Copy link
Member

@diego-plan9 diego-plan9 commented Jan 2, 2020

Summary

Add entries to the MANIFEST.in so the tests (test/ .py and other resources) are included in the .tar.gz source distribution, tuning a bit the existing globs in the process. For the non-python files, I went with specifying them in separate entries as the amount of items seems still reasonable, and to minimize the risk of including extraneous files (jupyter notebooks temporary checkpoints come to mind).

Details and comments

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This is a weird pattern if we want to include the tests as part of our installed package. Using a manifest like this doesn't actually accomplish anything useful. All this does is mean that we include the test/ directory in the sdist tarball, but because it's not included in any of the installed namespaces it gets ignored when you install from sdist. This also means that built wheels won't include it either. If the goal here is to include the tests in what we install so that we have the option to run tests from an installed qiskit we should just move the tests directory under the qiskit namespace (ie git mv -r test/qiskit/python/* qiskit/test/.).

@diego-plan9
Copy link
Member Author

If the goal here is to include the tests in what we install [...]

No, it is not: the PR is about distribution, not installation.

All this does is mean that we include the test/ directory in the sdist tarball

Correct, and intended! There is value in bundling both the code and the tests in a single pip-retrievable file. Actually, I think you summed up the rationale quite nicely at #268 (comment) - I was basically typing something similar myself:

This is useful for packagers because when they download the tarball they'll be able to verify their packages work by running the tests. (instead of having to also clone the repo to get the tests)

With this PR, we can facilitate those cases without entering into the installation domain - it is not meant to close #1389 entirely (only the "bundling" part, for some specific definition of bundling).

@diego-plan9
Copy link
Member Author

Complementing the previous comment, please note that default behavior of python setup.py build sdist is to include the test files in the resulting .tar.gz:

https://docs.python.org/3/distutils/sourcedist.html#specifying-the-files-to-distribute

[...] the sdist command puts a minimal default set into the source distribution:

  • anything that looks like a test script: test/test*.py (currently, the Distutils don’t do anything with test scripts except include them in source distributions, but in the future there will be a standard for testing Python module distributions)

In our case, the tests don't get picked up by default only because the default matching is non-recursive - this PR arguably sticks to the standard spirit and merely extends it to include the subdirectories.

@1ucian0
Copy link
Member

1ucian0 commented Jan 23, 2020

It seems to me that #1389 is the wrong justification for this PR, since I have the impression that we are even fully sure that we want a qiskit.test(). However, I see the value in including the tests in the tarball (for running the test at packing time distributions like Debian, for example). About the right way to fulfill that goal, @diego-plan9's reasoning about doing it via manifest seems convincing to me. What do you think @mtreinish ?

@mtreinish
Copy link
Member

If the sole goal here is to just include it in the sdist only for packaging without the intent of enabling users who install from running tests then this is fine. Just update the PR summary (and I'd say commit message too but it looks like there aren't any commit messages) to reflect that and remove the #1389 reference and I'll be fine with this.

recursive-include qiskit/test/mock/backends *.json

# Include the tests files.
recursive-include test *.py
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this and L10-L13, you could replace this with just

graft test

and probably also a exclude to make sure we don't pull in pyc files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is less risky to include rather than exclude - from the description:

For the non-python files, I went with specifying them in separate entries as the amount of items seems still reasonable, and to minimize the risk of including extraneous files (jupyter notebooks temporary checkpoints come to mind).

We can reevaluate as things evolve.

@diego-plan9
Copy link
Member Author

Ok! Tweaked the description, as it seems I was unable to convey the relation with the issue.

@1ucian0 1ucian0 added this to the 0.12 milestone Jan 28, 2020
@mergify mergify bot merged commit 4a1f9e7 into Qiskit:master Jan 28, 2020
@diego-plan9 diego-plan9 deleted the feature/sdist-tests branch January 29, 2020 08:32
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* Add test Python files to MANIFEST.in

* Add other test resources and tweaks

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

3 participants