desihub / specter

A toolkit for simulating multi-object spectrographs
Other
8 stars 7 forks source link

Coverage #30

Closed sbailey closed 8 years ago

sbailey commented 8 years ago

Improves coverage tests, and fixes one bug found in the process (GaussHermitePSF didn't define wmin_all and wmax_all).

weaverba137 commented 8 years ago

@sbailey, I can tell that you're floundering here a bit on these tests.

The original error message has nothing to do with coverage or desiutil. And by removing desiutil, you've removed the coverage check. The desiutil imports are what make python setup.py test --coverage possible.

This doesn't run the coverage check. It reports the results of the coverage check to coveralls.io:

after_success:
    - if [[ $SETUP_CMD == 'test --coverage' ]]; then coveralls; fi

I can't tell yet what is happening, but it is some kind import error. Perhaps tests are starting at the wrong level of the directory hierarchy.

sbailey commented 8 years ago

Thanks for jumping in. I had just realized that my original failures weren't due to desiutil after I got rid of desiutil and the tests were still failing (I think I got coverage back in there though). Odd thing is that the tests pass without a problem on my laptop. I'll try to add some debugging statements about the environment and pwd so expect some more emails from github/specter...

sbailey commented 8 years ago

The previous versions of astropy=1.1 numpy=1.9 scipy=0.14 used to work for the Travis tests, but don't work now. From the log, it was automatically upgrading numpy=1.10.4 but perhaps leaving scipy alone. Explicitly changing to astropy=1.1 numpy=1.10, scipy=0.17 seems to work. It is unclear what this was necessary to change now — perhaps some of my expanded coverage tests touched on the incompatible part of the previous set of installs and caused an import error. If an import error in a test really causes:

AttributeError: 'module' object has no attribute 'test'

then that is just downright unhelpfully evil.

I'm considering removing the specific version requirements on numpy/scipy/astropy and just letting conda provide a reasonably recent self-consistent set.

Now in the process of trying to fix this, I have of course broken coveralls, which was the original point of this branch (or at least improving coverage was).

weaverba137 commented 8 years ago

The code that raises that exception is pretty evil. It basically does a series of imports, checks which fail, and uses that to determine the path to the test suite. So "real" import error could throw that off.

You'll need to look at https://github.com/desihub/desiutil/blob/master/py/desiutil/setup.py to see how coverage checks are run. Coverage checks are not part of the default test command provided by setuptools/distutils.

sbailey commented 8 years ago

Travis tests + coveralls work again, with coverage up to 82.5%. The main ding against the coverage is the deprecated gausshermite2 PSF (we use the normal gausshermite PSF). This accidentally fixed #20 (remove desiutil dependency) along the way.