search

desihub / specsim

Quick simulations of spectrograph response
2 stars 9 forks source link

toplevel pytest not working #130

Closed sbailey closed 5 months ago

sbailey commented 1 year ago

We're trying to move desi-related repos from the deprecated "python setup.py test" to "pytest". For the specsim package, "pytest specsim/tests" works, but if you just run "pytest" at the top level it tries to pickup things in non-test directories like ah_bootstrap.py and specsim/atmosphere.py .

Identify why pytest is picking up these non-test files, and try to reorganize so that "pytest" at the top level works.

Context: although running "pytest specsim/tests" is fine, it is a pain to have to remember which repos require which subdirectories to be specified and it would be preferable for all our repos to work with "pytest" alone without caveats, just like they used to all work with "python setup.py test".

@weaverba137 this is an example of what we discussed on Monday, where top-level "pytest" doesn't work correctly.

weaverba137 commented 1 year ago

This is a special case in a couple of ways, because specsim has historically followed the astropy-affiliated package pattern. That pattern includes built-in pytest configuration that is not included in other DESI packages.

In the case of specsim/atmosphere.py, the astropy-affiliated configuration enables doctest_plus, which runs tests on any examples in the package. There's an example on Line 28. Now I happen to think that testing examples is a good thing, but it's not something we've normally enforced in DESI.

I'm a little less clear on 'non-test directories like ah_bootstrap.py', because ah_bootstrap.py is a file, not a directory. It would be useful to see an actual test result showing every unexpected file or directory. In any case, the Astropy community no longer supports astropy_helpers, and has even deprecated their own package template in favor of much simpler packaging guidelines (which are actually more compatible with DESI's standard pattern).

The developers of this package need to make a definitive decision: fully update this package to the latest astropy-affiliated package patterns, or abandon astropy-affiliated status altogether and move towards a DESI-pattern package. In the latter case, a wider sample of the DESI collaboration would be able to provide support for this package.

weaverba137 commented 5 months ago

I believe this is now addressed by #136. Note however that in the course of updating the package infrastructure, I discovered a previously undeclared dependency on desimodel, which also brings in a dependency on desiutil. If this is not the case, please reopen.