tests/data don't install

[tashrifbillah]

Has there been an issue that tests/data don't get installed in nrrd/tests/ ? I am experiencing this with:
pip install pynrrd
pip install .
python setup.py install

However, the python files inside tests/ get installed. I tried putting on __init__.py file in tests/data, yet no luck.

[addisonElliott]

I just reread the setup.py script and I agree that the data folder is not being copied over.

The next question is whether we want to copy data over on pip install or just require the user to clone the GitHub repo if they want to test things. I'm partial to the latter option, however we shouldn't bother even including the test files if the data isn't present.

Here is the line of code that is relevant I believe: https://github.com/mhe/pynrrd/blob/master/setup.py#L27

This would need to be tests/data/* I believe

[tashrifbillah]

I noticed the line of code. Before submitting the issue, I tried your proposed change, didn't work either.

I am also partial to the latter option. It does make sense to not copy nrrd/tests, if data files are not present there. I think this could be done by omitting package_data in setup.py. However, before going there, can you try the options one more time:

1. Include __init__.py in data/
2. Use tests/data/* as the second item in the list assigned to package data
3. Another option I would not like (don't see a package copying test files like this) is to use the following in setup.py:
   data_files=[('share/doc/conversion/data',glob(pjoin('tests','data','*')))],

Just to bring to your attention, apparently DIPY also has this problem. There is a comment in setup.py in this regard.

By the way, we might have to mention this in the README.md. Also, I am a little skeptical about python -m unittest discover absolute/path/to/cloned/pynrrd/nrrd/tests finding the right test files when user has nrrd installed in his site-packages already.

[tashrifbillah]

Okay, it looks like I found the solution. As I noted before, we need both:

1. __init__.py in data/
2. tests/data/* as the second item in the list assigned to package_data

Feel free to test my solution at your convenient time. I can do a PR.

[addisonElliott]

Okay, that makes sense. I am confused though, we just need to remove line 27 in order to exclude the tests in the package data right? Wouldn't that just remove the tests altogether and then if the user wants to test it, we just require they clone the repo?

[tashrifbillah]

Not sure if you had the chance to look at my latest comment. We are not removing anything, rather modifying as follows:

package_data={'nrrd': ['tests/', 'tests/data/']}

Upon using the above modification, we no longer need to clone the repo. Rather, python -m unittest discover absolute/path/to/cloned/pynrrd/nrrd/tests works:

(base) [tb571@pnl-z840-2 Downloads]$ python -m unittest discover ~/Documents/miniconda3/lib/python3.6/site-packages/nrrd/tests/
................/home/tb571/Documents/miniconda3/lib/python3.6/traceback.py:216: ResourceWarning: unclosed file <_io.BufferedReader name='/home/tb571/Documents/miniconda3/lib/python3.6/site-packages/nrrd/tests/data/BallBinary30x30x30.raw'>
  tb.tb_frame.clear()
./home/tb571/Documents/miniconda3/lib/python3.6/site-packages/nrrd/reader.py:409: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead
  data = np.fromstring(decompressed_data[byte_skip:], dtype)
....../home/tb571/Documents/miniconda3/lib/python3.6/site-packages/nrrd/reader.py:409: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead
  data = np.fromstring(decompressed_data[byte_skip:], dtype)
.........................
----------------------------------------------------------------------
Ran 48 tests in 0.160s

OK

The user might have to to pip install pynrrd --upgrade. But yes, of course removing line 27 would remove the tests altogether.

[addisonElliott]

Yeah, I read your latest comment and it all makes sense. My confusion was whether we think the best approach is to include the data in the tests or to remove the tests altogether.

You said this originally so I was a bit confused about it:

I am also partial to the latter option. It does make sense to not copy nrrd/tests, if data files are not present there.

Personally, I don't know if we should include the tests at all. However, I don't know what common practice is for the bigger Python libraries like Numpy, Scipy, scikit-image, etc.

My concern with including the tests is that the tests will not be used by a vast majority of the people downloading the library and so that is just a waste of space. On the other hand, the tests and data add up to a whopping 200kB.

What are your thoughts? I could go either way with this so if you submit a PR, I'll accept it with either approach.

P.S. Yes, I think users would have to upgrade their pynrrd version in order to get the tests. Not that big of a deal, plus most people probably don't care if they have tests. Just the select few, like you

[tashrifbillah]

On a different note, is this line required when running tests from cloned repo?

[addisonElliott]

It is for me. Otherwise I'm unable to import nrrd. Do you think differently?

[tashrifbillah]

Agreed, just checking to make sure. It should be also required when working on cloned repository other than the installed one.

[tashrifbillah]

Let's remove this line:

https://github.com/mhe/pynrrd/blob/master/setup.py#L26

and write in README.md:

Run the following command in the base directory to run the tests:

git clone https://github.com/mhe/pynrrd.git
python -m unittest discover -v nrrd/tests

If this is the fix we agreed upon, I shall submit a patch.

[addisonElliott]

I agree with removing the tests from the package.

As for updating the README, there are two potential ways of doing it:

1. Remove the "Tests" header and include an excerpt in the "Install from source (recommended for contributing to pynrrd)" section like:

The tests can be run via the following command in the base directory...
blah blah blah

2. Or, include a disclaimer in the "Tests" header like:

Note: Tests can only be run from a cloned `pynrrd` repository. The PyPi package does not include the tests.

I'm partial to the first option but I'd accept either method. The reason why I'm not a fan of putting the python test command in the same code block as cloning the repository is primarily so the user understands what is going on. Plus, not everyone will immediately want to run the tests after they clone the repository.