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

don't install tests and benchmarks #1003

Conversation

jcgruenhage
Copy link

@jcgruenhage jcgruenhage commented Sep 14, 2022

jsonschema currently installed the whole jsonschema subfolder of this
repository, which includes tests and benchmarks. From my testing, those don't
need to be installed, so here's a patch to not install them.


📚 Documentation preview 📚: https://python-jsonschema--1003.org.readthedocs.build/en/1003/

@codecov-commenter
Copy link

Codecov Report

Base: 98.18% // Head: 28.60% // Decreases project coverage by -69.57% ⚠️

Coverage data is based on head (22b0046) compared to base (1ada5d0).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1003       +/-   ##
===========================================
- Coverage   98.18%   28.60%   -69.58%     
===========================================
  Files          20       10       -10     
  Lines        3466     1552     -1914     
  Branches      570      392      -178     
===========================================
- Hits         3403      444     -2959     
- Misses         47     1099     +1052     
+ Partials       16        9        -7     
Impacted Files Coverage Δ
jsonschema/cli.py 0.00% <0.00%> (-95.63%) ⬇️
jsonschema/_legacy_validators.py 11.73% <0.00%> (-87.16%) ⬇️
jsonschema/_validators.py 15.90% <0.00%> (-83.72%) ⬇️
jsonschema/_utils.py 21.55% <0.00%> (-76.65%) ⬇️
jsonschema/validators.py 30.44% <0.00%> (-66.35%) ⬇️
jsonschema/exceptions.py 38.36% <0.00%> (-59.75%) ⬇️
jsonschema/__init__.py 35.00% <0.00%> (-55.00%) ⬇️
jsonschema/_format.py 53.27% <0.00%> (-41.05%) ⬇️
jsonschema/_types.py 63.33% <0.00%> (-36.67%) ⬇️
jsonschema/tests/test_exceptions.py
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Julian
Copy link
Member

Julian commented Sep 14, 2022

Hi, thanks! An argument could possibly be made for the benchmarks, so probably happy to exclude those, but generally tests should be installed with (any) package, as users may be interested in running the tests against an installed version.

@jcgruenhage
Copy link
Author

I don't really agree on that. Running tests of a library that's installed as a dependency somewhere down the stack isn't something that's commonly done by users. If a user wants to do so, they can still choose to include those files, but defaulting to having them not included does make more sense IMO. Installing the tests is not a common practice, and also not the default for the tooling that you're using. hatch defaults to having the tests in a separate folder in the repository root, which causes the tests not to be shipped to users by default.

@jcgruenhage
Copy link
Author

The test coverage delta is still something that I hadn't forseen and is something that means that this can't be merged here as is.

@Julian
Copy link
Member

Julian commented Sep 14, 2022

Installing the tests is not a common practice

Sure it is. See e.g. http://pythonchb.github.io/PythonTopics/where_to_put_tests.html which is just the first Google result, but there have essentially been two camps here for ~10 years at least, and plenty of packages do both.

and also not the default for the tooling that you're using.

Hatch is a packaging tool, it doesn't really have an opinion on this, though its author of course has to make a choice one way or the other for hatch itself as a package with tests.

Of course if you're packaging for a distro you're welcome to prune the tests if you feel you'd like to, since I agree if some end user gets jsonschema installed they're unlikely to care about it or its tests, but for normal uses (developers who've installed jsonschema while developing some Python package) this is both common and a good practice in my opinion -- we're talking tiny files-- so I'm going to thank you for the PR but decline.

@Julian Julian closed this Sep 14, 2022
@jcgruenhage jcgruenhage deleted the dont-install-tests-and-benchmarks branch September 14, 2022 13: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.

None yet

3 participants