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 sdists #310

Merged
merged 2 commits into from
Sep 1, 2019
Merged

Include tests in sdists #310

merged 2 commits into from
Sep 1, 2019

Conversation

toddrme2178
Copy link
Contributor

This lets downstream users and packagers check their installation.

This lets downstream users and packagers check their installation.
@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #310 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
- Coverage    99.2%   99.18%   -0.03%     
==========================================
  Files          70       71       +1     
  Lines        6701     6771      +70     
==========================================
+ Hits         6648     6716      +68     
- Misses         53       55       +2
Impacted Files Coverage Δ
jupytext/cell_reader.py 96.57% <0%> (-0.41%) ⬇️
jupytext/magics.py 100% <0%> (ø) ⬆️
tests/test_contentsmanager.py 100% <0%> (ø) ⬆️
tests/test_read_simple_python.py 100% <0%> (ø) ⬆️
jupytext/jupytext.py 100% <0%> (ø) ⬆️
jupytext/languages.py 100% <0%> (ø) ⬆️
tests/test_execute.py 100% <0%> (ø) ⬆️
jupytext/cli.py 95.92% <0%> (ø) ⬆️
tests/test_jupytext_read.py 100% <0%> (ø)
jupytext/stringparser.py 95.12% <0%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5422de5...6fa126e. Read the comment docs.

@mwouts
Copy link
Owner

mwouts commented Aug 14, 2019

Hello @toddrme2178 , thanks for suggesting this. Actually I must confess that I don't have much experience with the MANIFEST.in files, are you aware of a good reference on those? Is it a common practice in Python to package the tests with the package? (in the case of Jupytext, that could make the package much larger).

@jhermann, I see that we stopped deploying the tests folder in version 1.0.1 following your advice at #184. How does that relates to @toddrme2178 's suggested change here? Thanks!

@toddrme2178
Copy link
Contributor Author

@mwouts It is pretty standard to include tests in sdists (less so in wheels), unless they are so huge that they make the package hard to download. The approach used here will include the files in the sdist but not install them.

Here is the list of commands for a MANIFEST.in file.

@jhermann
Copy link

#184 is about pip install (and wheels), not setup.py sdist. Different life-cycle phases.

@mwouts
Copy link
Owner

mwouts commented Aug 15, 2019

I see! Thank you both for the additional explanations. Sure, I'll integrate this in the next release (most probably in the first weeks of September).

@mwouts mwouts added this to the 1.2.2 milestone Aug 30, 2019
@mwouts
Copy link
Owner

mwouts commented Sep 1, 2019

Hello @toddrme2178 , I just gave a try to this, and I have two additional questions:

  • Can I exclude the directory tests/__pycache__ that exists locally on my computer? I tried recursive-exclude and global-exclude but I could not get them to do what I wanted.
  • Adding the tests increases the size of the tar.gz generated by python setup.py sdist bdist_wheel from 157Ko to 2596Ko. I think this is too much. Would it be fine for you if we exclude one large notebook from the test base? Again, I'd like to know how to do that in the MANIFEST.in file... Thanks!

@jhermann
Copy link

jhermann commented Sep 1, 2019

Use prune, at the very end of the manifest.

@mwouts
Copy link
Owner

mwouts commented Sep 1, 2019

Thank you @jhermann.

I have been able to ignore the files in the __pycache__ folder with

prune tests\__pycache__

(sorry I'm doing this on Windows, so it really has to be a \...)

However I have still not find a way to exclude the large notebook. I've tried adding

prune tests\notebooks\ipynb_py\World population.ipynb

but that did not work; also I tried with ? or * in place of the white space, I even tried renaming the notebook to remove the white space, etc. Do you see anything else that could help me exclude that file?

@mwouts
Copy link
Owner

mwouts commented Sep 1, 2019

Finally I was able to exclude the large notebook with

global-exclude *population.ipynb

Thank you @toddrme2178 and @jhermann !

@mwouts mwouts merged commit 7329013 into mwouts:master Sep 1, 2019
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