Divmod

Code review comment for lp:~nejucomo/divmod.org/fix-nevow-setup

fix-nevow-setup
Merge into trunk

Revision history for this message

nejucomo (nejucomo) wrote on 2013-07-27:

> There's no problem with the tests living inside of nevow. The *code* has to
> live in setup.py to avoid regressing, but there's no problem with importing
> setup.py as a module to run some functions in it. Why does that seem
> ridiculous to you?

Perhaps "ridiculous" is too strong. It depends on the expected use case of unittests. Importing "setup" is a problem if the unittests are intended to run from the installed code.

I tend to prefer unittests which install with the application code, such that at any time one may run the test suite for an installed package as a sanity check. For example: "pip install $PKG; trial $PKG"

If, OTOH, we decide unittests can only be run from a revision control checkout (what about unpacked sdists?), then this would not be an issue, except in so far as future maintainers don't realize this policy and violate it.

A middle ground is to have "application" unittests which are installed with the package and can be run on the installed package, and have separate "packaging" unittests.

I think I prefer this "middle ground" approach. Unless someone raises an issue with it, next chance I'll take this approach:

Add a new single python module called something like "nevow_setup_unittests.py" which does not get installed, and place the test code in there, then perhaps tweak "runtests" to pass that to trial. (I'm assuming "runtests" is only for development and is unrelated to the installation.)

I prefer this separate module approach versus a conditional import inside Nevow *application* unittest code because it seems to easy to import the *wrong* setup.py.

I'm curious what other packages have done to address this issue. I know that Tahoe-LAFS, for example has an extremely complex setup.py and importing it has all manner of side effects. I wouldn't be surprised if it modifies sys.path outside of setuptools behavior.

The approach there has been to test all packaging code with test code which lives outside the repository and is currently messily spread about some buildbot configs.

I'd like to see the packaging test code which is specific to a revision live within the repository, yet be separate from application unittest code.

> There's no problem with the tests living inside of nevow.  The *code* has to
> live in setup.py to avoid regressing, but there's no problem with importing
> setup.py as a module to run some functions in it.  Why does that seem
> ridiculous to you?

Perhaps "ridiculous" is too strong.  It depends on the expected use case of unittests.  Importing "setup" is a problem if the unittests are intended to run from the installed code.

A middle ground is to have "application" unittests which are installed with the package and can be run on the installed package, and have separate "packaging" unittests.

I think I prefer this "middle ground" approach.  Unless someone raises an issue with it, next chance I'll take this approach:

Add a new single python module called something like "nevow_setup_unittests.py" which does not get installed, and place the test code in there, then perhaps tweak "runtests" to pass that to trial.  (I'm assuming "runtests" is only for development and is unrelated to the installation.)

I prefer this separate module approach versus a conditional import inside Nevow *application* unittest code because it seems to easy to import the *wrong* setup.py.

I'm curious what other packages have done to address this issue.  I know that Tahoe-LAFS, for example has an extremely complex setup.py and importing it has all manner of side effects.  I wouldn't be surprised if it modifies sys.path outside of setuptools behavior.

The approach there has been to test all packaging code with test code which lives outside the repository and is currently messily spread about some buildbot configs.

I'd like to see the packaging test code which is specific to a revision live within the repository, yet be separate from application unittest code.

« Back to merge proposal