search

zopefoundation / DateTime

This package provides a DateTime data type, as known from Zope. Unless you need to communicate with Zope APIs, you're probably better off using Python's built-in datetime module.
Other
19 stars 25 forks source link

Don't install tests #27

Closed scop closed 3 years ago

scop commented 3 years ago

They're not needed at runtime.

jamadden commented 3 years ago

I'm not sure that won't mess up downstream distributors. I've seen distros that install the package then run the tests before finally completing their final package (RPM). They do this to detect packaging problems, I think.

d-maurer commented 3 years ago

Please keep the tests in the distribution. They might rarely be required but they won't hurt. When I suspect a distribution to cause problems, the first thing is to run their tests.

dataflake commented 3 years ago

I cannot see any compelling reason to change the egg, either. -1 on this change.

scop commented 3 years ago

This does not exclude tests from the sdist, so downstream distributors as well as anyone building from source is still able to run them.

I'm aware there are different opinions on this, and won't start parroting mine but here are a few data points:

scop commented 3 years ago

Elaborated a bit more on "why not" at https://gist.github.com/scop/5910ee85a1352988554e8b8d5d675e65

dataflake commented 3 years ago

I like your writeup. I'm not super firm on setuptools details, so you're saying this PR won't change anything for the source distribution tarball on PyPI and only affect the wheel? If that is the case I wonder if the use cases @jamadden and @d-maurer mention are covered well enough by the source distribution.

I personally don't use tests in installed packages at all, only in the source tree, so I don't need the wheel to contain them.

scop commented 3 years ago

so you're saying this PR won't change anything for the source distribution tarball on PyPI and only affect the wheel?

That's the intent here. I did look into the generated bdist_wheel and sdist results locally to verify before filing the PR, and it looked good.

d-maurer commented 3 years ago

Ville Skyttä wrote at 2020-10-24 00:06 -0700:

... I'm aware there are different opinions on this, and won't start parroting mine but here are a few data points:

  • They do "hurt" by piling up and being waste of space and network bandwidth for the vast majority of users. For example for the full dependency set of Home Assistant,du reports 75MB of that fluff here.

Nowadays, 75 MB are not really impressive. What is its percentage relative to the complete (i.e. test and non-test) code?

  • Test code often requires test dependencies, which likely (well hopefully) are not installed for regular installs. (Haven't checked if it's the case here.)

I often analyse problems and therefore the general testing tools (mainly dm.zopepatches.ztest, a zope.testrunner wrapper) are always installed in my environments. Typically, nothing more was required for the tests I have performed recently.

For DateTime this gives:

/home/dieter/tmp/Zope/src /home/dieter/tmp
Running tests at level 1
Running zope.testrunner.layer.UnitTests tests:
  Set up zope.testrunner.layer.UnitTests in 0.000 seconds.
  Running:
................................................
  Ran 48 tests with 0 failures, 0 errors and 0 skipped in 0.076 seconds.
Tearing down left over layers:
  Tear down zope.testrunner.layer.UnitTests in 0.000 seconds.
  • Test code is often referred to as usage examples, but it often is not really written like something that is a good example of the package's use. (Haven't checked if it's the case here.)

Well, I prefer non optimal examples over no examples at all.

  • In my experience, the majority of package don't install tests (which I happen to think is great), so one cannot really generally rely on them being installed.

I checked with my Zope installation: 132 distributions installed 66 test packages

Fortunately, I had not yet to analyse problems apparently related to disbtributions without installed tests.

d-maurer commented 3 years ago

Jens Vagelpohl wrote at 2020-10-24 01:52 -0700:

I like your writeup. I'm not super firm on setuptools details, so you're saying this PR won't change anything for the source distribution tarball on PyPI and only affect the wheel? If that is the case I wonder if the use cases @jamadden and @d-maurer mention are covered well enough by the source distribution.

I am not convinced (--> details in other reply).

In my own distributions, I always include tests AND documentation. And in several cases, the test availability has been important to understand problems reported by users.

It is very rare that I met a package which called for my attention and which did not contain tests (and I hope that stays this way). But documentation has been missing significantly more often. In my view, an installation should contains both the tests and its documentation.

dataflake commented 3 years ago

@d-maurer I have a feeling you keep talking about your own packages and how you handle problems there. Don't forget that this is about zopefoundation packages.

I'm happy to be proven wrong, but I haven't met anyone who attempts to diagnose issues by running tests right in the installed package. Everyone I know runs package tests from a code checkout. Besides, with CI enabled on most of our repositories, the case where a package is released with failing tests seems super rare.

d-maurer commented 3 years ago

Jens Vagelpohl wrote at 2020-10-26 02:50 -0700:

@d-maurer I have a feeling you keep talking about your own packages and how you handle problems there. Don't forget that this is about zopefoundation packages.

I am aware of this.

I'm happy to be proven wrong, but I haven't met anyone who attempts to diagnose issues by running tests right in the installed package.

Concrete example for zopefoundation/Missing: A client had strange problems in an old installation (based on a buildout setup). Initial analysis indicated that this might be related to Missing. Running the Missing tests directly in this installation revealed that it did indeed no longer work. The instance's evolution had caused a breakage for Missing.

Proving the Missing disfunctionality (in the given context) would have been significantly more complex when the tests had not been included.

Keep in mind that most zopefoundation packages have significant dependencies on other packages. With different versions of those packages, they may fail - even though all tests pass in a reference system. Therefore, it is very helpful to have the tests readily available to be run in the setup at hand.

dataflake commented 3 years ago

Concrete example for zopefoundation/Missing: A client had strange problems in an old installation (based on a buildout setup). Initial analysis indicated that this might be related to Missing. Running the Missing tests directly in this installation revealed that it did indeed no longer work. The instance's evolution had caused a breakage for Missing. Proving the Missing disfunctionality (in the given context) would have been significantly more complex when the tests had not been included.

The process I have used (and I know many people are using) is to temporarily extend that buildout configuration to grab the offending package from GitHib with mr.developer and re-run the buildout. Is that too complex?

d-maurer commented 3 years ago

Jens Vagelpohl wrote at 2020-10-26 05:00 -0700:

... The process I have used (and I know many people are using) is to temporarily extend that buildout configuration to grab the offending package from GitHib with mr.developer and re-run the buildout. Is that too complex?

Significantly more complex then ztest -s Missing.

Let's see the concrete example:

dataflake commented 3 years ago

Significantly more complex then ztest -s Missing.

This must be a reference to your own tool. No one else knows/has that.

Let's see the concrete example: * I need to change the buildout configuration using tools ("mr.developer") , I do not regularly use and which therefore require reading the corresponding documentation

Please don't get me wrong, but that's a lame excuse. mr.developer is used everywhere in the zopefoundation packages. One quick look and you know how it is used.

  • I must locate the precise version/commit/tag corresponding to Missing version currently used in the buildout.

Just view the contents of the produced instance script in the bin folder of your buildout and find the line with the egg in question. 1 second job.

jamadden commented 3 years ago

A can attest to the value of having the tests package installed. For gevent, I've also asked bug reporters to run tests in their installation and report the results. It's a simple python -m gevent.tests.test__SOMETHING. I don't control how their package is installed so I can't make any assumptions nor ask them to re-run buildout.

Overall I'm -1 on excluding the tests subpackage from installation. I don't see the benefit; the "downsides" seem very insignificant.

dataflake commented 3 years ago

OK, I think in summary there's a vocal majority for keeping them in. @d-maurer I apologize for being so critical, I was just trying to weed out reasons that appear to be based only on a strong wish to continue using practices that may be unique to your own development style but not widely shared.

d-maurer commented 3 years ago

Jens Vagelpohl wrote at 2020-10-26 05:55 -0700:

Significantly more complex then ztest -s Missing.

This must be a reference to your on tool. No one else knows/has that.

It is almost zope.testrunner. Many buildouts have a test section which allows for bin/test -s <package>.

  • I need to change the buildout configuration using tools ("mr.developer") , I do not regularly use and which therefore require reading the corresponding documentation

Please don't get me wrong, but that's a lame excuse. mr.developer is used everywhere in the zopefoundation packages. One quick look and you know how it is used.

When I look (currently at five.formlib), I see extensions=mr.developer. But that is not enough: I must also know that I have to define a sources section and what exactly to put there.

  • I must locate the precise version/commit/tag corresponding to Missing version currently used in the buildout.

Just view the contents of the produced instance script in the bin folder of your buildout and find the line with the egg in question. 1 second job.

This gives me the version number of the used package. But I would need the exact reference for mr.developer.