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

Exclude tests from wheel #249

Closed
wants to merge 1 commit into from

Conversation

penguinpee
Copy link

It's generally not desired to install tests together with the module.
This change does just that and keeps everything else unchanged. Tests
are still included in the sdist.

It's generally not desired to install tests together with the module.
This change does just that and keeps everything else unchanged. Tests
are still included in the sdist.
@satra satra requested a review from jwodder August 20, 2024 12:37
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.89%. Comparing base (6fc16b8) to head (37f7505).

❗ There is a different number of reports uploaded between BASE (6fc16b8) and HEAD (37f7505). Click for more details.

HEAD has 121 uploads less than BASE
Flag BASE (6fc16b8) HEAD (37f7505)
unittests 151 30
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   97.75%   91.89%   -5.87%     
==========================================
  Files          16       16              
  Lines        1739     1739              
==========================================
- Hits         1700     1598     -102     
- Misses         39      141     +102     
Flag Coverage Δ
unittests 91.89% <ø> (-5.87%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jwodder jwodder left a comment

Choose a reason for hiding this comment

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

Personally, I think that, if we're going to exclude tests from the wheel (which I support), we should go all the way and move tests/ outside of dandischema/ — but @yarikoptic doesn't like either of these practices.

@yarikoptic
Copy link
Member

yarikoptic commented Aug 20, 2024

It's generally not desired to install tests together with the module.

why? Does it solve any problem?

it kinda introduces some though:

  • becomes difficult to test software as installed: need to dig out tests somewhere, match version etc.
  • edit: if moved outside of sources, not so easy to test a component of software I just changed unless tests hierarchy replicates original code hierarchy

And so far I personally did not spot any problem being solved, or convenience being introduced, so not sure if a worthwhile effort.

@penguinpee
Copy link
Author

Personally, I think that, if we're going to exclude tests from the wheel (which I support), we should go all the way and move tests/ outside of dandischema/ — but @yarikoptic doesn't like either of these practices.

Well, I looked at that option as well. But that's complicated by the fact that there are tests in dandischema/digests/tests as well.

why? Does it solve any problem?

  1. For starters it makes the wheel smaller. It might not be significant in this case, but I've packaged projects with hundreds of tests + test data.
  2. They are not required as part of the installed package. I ask myself: Does it make sense to do from dandischema import tests? Moreover, if you were using automatic discovery tests would be excluded by default. Users don't need them.
  3. In Fedora, we can optionally run an import test on the installed package. That's helpful in discovering if any dependencies have been missed (e.g. not declared). For dandischema, in it's current configuration, that throws an error (or false positive) on pytest1.
  • becomes difficult to test software as installed: need to dig out tests somewhere, match version etc.
  • edit: if moved outside of sources, not so easy to test a component of software I just changed unless tests hierarchy replicates original code hierarchy

I think both points very much depend on how you run tests and how your dev environment is set up.

In the end, it's a suggestion (based on common practice). But the decision is entirely up to you. I am / we are happy to carry it as a downstream only patch.

Footnotes

  1. Sure, I can exclude importing *tests*, but, again, those aren't required for using dandischema.

@yarikoptic
Copy link
Member

3. In Fedora, we can optionally run an import test on the installed package. That's helpful in discovering if any dependencies have been missed (e.g. not declared). For dandischema, in it's current configuration, that throws an error (or false positive) on pytest1.

We arrived at the motivation/rationale, which IMHO boils down to: an installed component (dandischema.tests and alike) to import requiring a dependency which is typically not installed/not needed for a user installation.
ok, let me sit on this for a bit (autopkgtest for Debian below might add weight).

FWIW, for Debian packages (I am yet to package this one though), I usually enable not just import test but running all discovered tests during package build time. Then pytest should be installed etc for that purpose. And I do then install the tests but do not add pytest into installation dependencies. Nevertheless a person could use autopkgtest (if package has needed config) (see https://people.debian.org/~eriberto/README.package-tests.html for more) to run those tests for that version of package and thus testing it against its dependencies (not necessarily the versions etc present during package build time).

@yarikoptic
Copy link
Member

FWIW, even though I now see rationale, @penguinpee, if you could share some pointers to support the common practice and generally "claims", I would appreciate that!

@penguinpee
Copy link
Author

I think the most compelling argument for not including tests can be made by looking at what setuptools does when using automatic discovery. I already linked to the documentation, but here it is again, explicitly:

https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#flat-layout

You have FlatLayoutPackageFinder.DEFAULT_EXCLUDE and FlatLayoutModuleFinder.DEFAULT_EXCLUDE for excluding packages and modules respectively. Both list test and spelling variations thereof. Above the lists it states:

To avoid confusion, file and folder names that are used by popular tools (or that correspond to well-known conventions, such as distributing documentation alongside the project code) are automatically filtered out in the case of flat-layout

(emphasis mine)

Note that automatic discovery only works when packages or py_modules are not provided. In case of using packages = find: or find_namespace: and a flat layout with tests/, consider what would happen if you were not to filter out tests. It would get installed as a top level module. That's a packaging error, but not uncommon. It's very likely (one of) the rational(s) for filtering it out. I think it's unfortunate that find_packages() and find_namespace_packages() do not use the same filters.

@jwodder
Copy link
Member

jwodder commented Aug 20, 2024

@penguinpee setuptools' automatic discovery only filters out top-level tests/ trees, while tests/ inside a package (like dandischema has) is still included.

@penguinpee
Copy link
Author

A less compelling argument might be PyPA's sampleproject which uses automatic discovery with a src-layout. By placing tests/ outside src/ it will be automatically excluded from the wheel.

@penguinpee
Copy link
Author

@penguinpee setuptools' automatic discovery only filters out top-level tests/ trees, while tests/ inside a package (like dandischema has) is still included.

You are correct. I misunderstood that part. It indeed only filters on the top-level names.

@yarikoptic
Copy link
Member

yarikoptic commented Aug 21, 2024

some pointers to support the common practice and generally

@jwodder , IIRC you had/know of some kind of index of pip packages, am I right? I wonder if there is a listing of contents of source packages/distribution to get some stats on how common is to ship tests within sources, within package_source, within "binary" builds. I can only look at examples of some most popular/core packages, e.g.

numpy - within `numpy` package, and shipped in source distribution, (edit) and shipped in .whl
❯ find -iname tests
./numpy/_core/tests
./numpy/_pyinstaller/tests
./numpy/compat/tests
./numpy/distutils/tests
./numpy/f2py/tests
./numpy/fft/tests
./numpy/lib/tests
./numpy/linalg/tests
./numpy/ma/tests
./numpy/matrixlib/tests
./numpy/polynomial/tests
./numpy/random/tests
./numpy/testing/tests
./numpy/tests
./numpy/typing/tests
❯ tar -tzvf <(curl --silent https://files.pythonhosted.org/packages/54/a4/f8188c4f3e07f7737683588210c073478abcb542048cf4ab6fedad0b458a/numpy-2.1.0.tar.gz) | grep 'tests/test_.*\.py' | head
-rw-r--r-- 0/0            5032 2024-07-08 08:34 numpy-2.1.0/numpy/_core/src/common/pythoncapi-compat/tests/test_pythoncapi_compat.py
-rw-r--r-- 0/0           27472 2024-07-08 08:34 numpy-2.1.0/numpy/_core/src/common/pythoncapi-compat/tests/test_upgrade_pythoncapi.py
-rw-r--r-- 0/0            2881 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test__exceptions.py
-rw-r--r-- 0/0            2221 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_abc.py
-rw-r--r-- 0/0           22932 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_api.py
-rw-r--r-- 0/0            2824 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_argparse.py
-rw-r--r-- 0/0            3062 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_array_api_info.py
-rw-r--r-- 0/0           34509 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_array_coercion.py
-rw-r--r-- 0/0            7767 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_array_interface.py
-rw-r--r-- 0/0            3264 2024-08-18 13:20 numpy-2.1.0/numpy/_core/tests/test_arraymethod.py
❯ unzip -l numpy-2.1.0-pp310-pypy310_pp73-win_amd64.whl | grep 'tests/test_.*\.py' | head
     2853  2024-08-18 17:32   numpy/distutils/tests/test_build_ext.py
    29586  2024-08-18 17:32   numpy/distutils/tests/test_ccompiler_opt.py
     6523  2024-08-18 17:32   numpy/distutils/tests/test_ccompiler_opt_conf.py
     7612  2024-08-18 17:32   numpy/distutils/tests/test_exec_command.py
     1320  2024-08-18 17:32   numpy/distutils/tests/test_fcompiler.py
     2191  2024-08-18 17:32   numpy/distutils/tests/test_fcompiler_gnu.py
     1088  2024-08-18 17:32   numpy/distutils/tests/test_fcompiler_intel.py
     1124  2024-08-18 17:32   numpy/distutils/tests/test_fcompiler_nagfor.py
     1147  2024-08-18 17:32   numpy/distutils/tests/test_from_template.py
      902  2024-08-18 17:32   numpy/distutils/tests/test_log.py

@jwodder
Copy link
Member

jwodder commented Aug 21, 2024

you had/know of some kind of index of pip packages, am I right?

Not one that covers sdists.

I wonder if there is a listing of contents of source packages/distribution to get some stats on how common is to ship tests within sources, within package_source, within "binary" builds.

If you really want to investigate this, I suggest asking on https://discuss.python.org/c/packaging/14.

@penguinpee
Copy link
Author

I didn't mean to send you on a wild goose chase. If you prefer including the tests in the wheel, by all means do. As I stated before, we can carry this PR as a downstream only patch. Matter of fact we already do.

Quite often when I see tests shipped in the wheel it turns out to be an oversight. Sometimes it is a genuine mistake, e.g. when they are shipped as a separate module.

I have also come across projects that ship them neither in the wheel nor in the sdist. In such cases we usually fall back to acquiring the sources from the upstream forge, because we do want to run those tests as part of the build if possible within reasonable effort.

@yarikoptic
Copy link
Member

Thank you @penguinpee ! For now I would just prefer to keep current practice among our projects, unless it would be prohibitive (e.g. large test data requirements etc) which would warrant adding separation of tests from the main code sources. Who knows -- I might be convinced in the opposite later on, so I will keep in mind this case of "import tests".
Cheers!

@yarikoptic yarikoptic closed this Aug 23, 2024
yarikoptic added a commit to yarikoptic/fscacher that referenced this pull request Aug 23, 2024
…test

also removed unnecessary now dev installation of joblib

# extras test vs tox.ini:

Most recent debate on tox.ini vs extra_depends is at
con/solidation#43 (comment)

Although I appreciate the convenience of "tox" as an entry point for testing, I
increasingly find no support for it to be "the" location for testing depends
listing. Moreover I keep running into cases of needing to fish out test
dependencies somewhere else (tox.ini, nox etc) which then can differ across
projects, and also require me to adopt some avoidable runtime etc overhead from
running those extra test shims whenever pytest is just good enough.

My arguments for generally adopting an approach of specifying test
dependencies in setup.{cfg,py} or other "generic" locations as optional are:

- pytest, and its extentions are used (imported) inside the tests.  I, in 99%
  of cases, do consider "tests" to be the part of the source code.  I would not
  consider them part of the source code whenever there is an outside test
  battery which is developed/maintained independently of the source code.

  As such, I think that dependencies for tests, as part of the source code,
  should be listed alongside with dependencies for the build/installation/run
  time dependencies.  Some distributions even do "import test" across entire
  source code distribution and thus tend to decide or to request exclusion from
  source distributions (IMHO the wrong step in most of the cases).
  Ref: dandi/dandi-schema#249

- It is unfortunate that there is no "standard" convention on how/where to
  specify such test requirements, so I think it is ok to adopt [test] as
  the general convention among our projects.

- tox.ini looses NOTHING from using "extras".

- tox.ini is the correct location to describe environments and dependncies
  for external to source code tools/modules, i.e those not imported explicitly
  anywhere in the source code.

- with description of test dependencies alongside with the main dependencies
  would benefit downstream distribution (debian, conda, gentoo, etc)
  packagers since they do not need to fish around for what other dependencies
  to install/recommend/suggest for the package.

- I do appreciate the fact that test dependencies alone are not sufficient to
  run the tests, but invocation of the pytest is standardized enough to just
  invoke it against the source code. (given dependencies are installed)

  That is e.g. how dh-python helper in Debian operates -- if pytest dependency
  announced, just run pytest automagically.

# joblib:

joblib dev listing was added in

    commit 6375816
    Author: John T. Wodder II <[email protected]>
    Date:   Fri Mar 19 08:42:47 2021 -0400

likely to use

    ❯ git describe --contains 457d2c8a926105fd156b1dfccfaae3e500d22451
    1.1.0~13
    ❯ git show 457d2c8a926105fd156b1dfccfaae3e500d22451
    commit 457d2c8a926105fd156b1dfccfaae3e500d22451
    Author: John T. Wodder II <[email protected]>
    Date:   Thu Mar 18 06:07:06 2021 -0400

        Use inspect.signature() instead of inspect.getfullargspec() (#1165)

        This fixes a bug with the ignore parameter when the cached function is decorated

        Co-authored-by: Loïc Estève <[email protected]>

and we have already joblib ~= 1.1 so should be all set.
yarikoptic added a commit to yarikoptic/fscacher that referenced this pull request Aug 23, 2024
…test

Most recent debate on tox.ini vs extra_depends is at
con/solidation#43 (comment)

Although I appreciate the convenience of "tox" as an entry point for testing, I
increasingly find no support for it to be "the" location for testing depends
listing. Moreover I keep running into cases of needing to fish out test
dependencies somewhere else (tox.ini, nox etc) which then can differ across
projects, and also require me to adopt some avoidable runtime etc overhead from
running those extra test shims whenever pytest is just good enough.

My arguments for generally adopting an approach of specifying test
dependencies in setup.{cfg,py} or other "generic" locations as optional are:

- pytest, and its extentions are used (imported) inside the tests.  I, in 99%
  of cases, do consider "tests" to be the part of the source code.  I would not
  consider them part of the source code whenever there is an outside test
  battery which is developed/maintained independently of the source code.

  As such, I think that dependencies for tests, as part of the source code,
  should be listed alongside with dependencies for the build/installation/run
  time dependencies.  Some distributions even do "import test" across entire
  source code distribution and thus tend to decide or to request exclusion from
  source distributions (IMHO the wrong step in most of the cases).
  Ref: dandi/dandi-schema#249

- It is unfortunate that there is no "standard" convention on how/where to
  specify such test requirements, so I think it is ok to adopt [test] as
  the general convention among our projects.

- tox.ini looses NOTHING from using "extras".

- tox.ini is the correct location to describe environments and dependncies
  for external to source code tools/modules, i.e those not imported explicitly
  anywhere in the source code.

- with description of test dependencies alongside with the main dependencies
  would benefit downstream distribution (debian, conda, gentoo, etc)
  packagers since they do not need to fish around for what other dependencies
  to install/recommend/suggest for the package.

- I do appreciate the fact that test dependencies alone are not sufficient to
  run the tests, but invocation of the pytest is standardized enough to just
  invoke it against the source code. (given dependencies are installed)

  That is e.g. how dh-python helper in Debian operates -- if pytest dependency
  announced, just run pytest automagically.
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