robintw / Py6S

A Python interface to the 6S Radiative Transfer Model
GNU Lesser General Public License v3.0
191 stars 105 forks source link

Testing with SixS.test() doesn't run full test suite #19

Closed dmwelch closed 8 years ago

dmwelch commented 8 years ago

Pip installed Py6S and ran:

>>> from Py6S import *
>>> SixS.test()
6S wrapper script by Robin Wilson
Using 6S located at /Users/dwelch/virtualenvs/atmocorrect/bin/sixs
Running 6S using a set of test parameters
The results are:
Expected result: 619.158000
Actual result: 619.158000
#### Results agree, Py6S is working correctly
0

but this IS NOT a test suite (or even a real test, since it doesn't run anything but the default values!). I have two users with different environments/versions, etc., both with "passing" reports but vastly different results for their outputs when being used "in the wild".

One hallmark of software design is that it's better to have no tests than tests that mislead users with inadequate/invalid coverage. What happens is developers start depending on the 'bad' tests and introduce bugs that only pass the 'bad' tests.

robintw commented 8 years ago

Sorry for the frustration.

This isn't the full test suite, it's just a simple test to ensure that Py6S can actually find the 6S executable properly and can run it and get a sensible result - as that is the biggest problem that people have when installing Py6S. It might be better to rename this method, but I can't really think of a better name than test.

The full test suite is in the source repository under test, and can be run with nosetests from the root of the repository. Continuous integration is used to run the test suite on every commit, and there is pretty good coverage (see http://server.rtwilson.com:8080/job/Py6S/TOXENV=py34/93/cobertura/ for instance), so I don't think there is a problem with lack of testing. If you fancy submitting a pull request to add something like a full=True parameter to the SixS.test method that runs the full test suite, then I'd be more than happy to merge it.

If you've got a specific issue with the output of Py6S being different on two machines then please feel free to open a separate issue about that, and I'll see if I can help you work out what's going wrong (the output of s.produce_debug_report() would be very useful as part of the debugging process).

dmwelch commented 8 years ago

Hi Robin,

I think this should be reopened until setup.py includes the tests/ directory in the site_packages/Py6S dir - I have a coworker who mistakenly thought nose tests were all passing when really none were found.

Take a look at how nipype does their testing, I think this is how you want to do it: https://github.com/nipy/nipype/blob/master/nipype/__init__.py

Cheers!

robintw commented 8 years ago

I'm not sure how best to go about doing that. I've added the tests folder to MANIFEST.in so that it is included in the distribution, but Python compiles things to .egg files these days (at least, Python 3 does) so you can't actually cd to the directory that the tests are in - so I'm not sure how users could run the tests easily after they've installed via pip.

I've also added some configuration to setup.py to make it possible to run python setup.py test in the source folder (eg. before running python setup.py install) to run all of the tests.

I've had a brief look at how nipype does things, and it seems to be very complex and - to be honest - I don't really understand most of it.

If you're able to submit some pull requests that make it possible to run the tests easily once the .egg files have been install this then I'd be happy to merge them, but unfortunately I don't have the time at the moment to investigate the details of how to get this to work. From what I've read online, it sounds like it will be almost impossible to do this with .egg installs - but I'm happy to be proved wrong.

dmwelch commented 8 years ago

It looks like wheel should be used over egg (see PEP 0427: https://www.python.org/dev/peps/pep-0427/#comparison-to-egg).

This is my first stab at packaging, but I found this guide (http://python-packaging-user-guide.readthedocs.org/en/latest/) and I'll take a stab at it later. :smile: