Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT/TST: update tests to use pytest #477

Merged
merged 14 commits into from
Mar 28, 2019
Merged

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Mar 17, 2019

This PR switches from nose to pytest as requested in #389, but has not tried to refactor any of the tests. Many tests could probably benefit from use of the parametrize decorator, for instance, although that could be left to a follow-up PR.

Some common code from test_matlab_compatibility.py and test_matlab_compatibility_cwt.py and code checking availability of futures was factored out into a common file _pytest.py, but otherwise the actual changes to the tests are pretty minimal.

There are a few remaining things to be resolved:

1.) Do we need to remove the ability to run tests via pywt.test() or is there a way to maintain that without requiring nose? It is still present currently, but uses nose indirectly via NumPy and there are a couple of failures now when running the test this way because skips related to optional dependency pymatbridge were not respected.

2.) I'm not sure yet if code coverage is working correctly after these changes.

3.) I haven't tested running the pymatbridge-based tests via PYWT_XSLOW yet.

closes #389

@rgommers
Copy link
Member

This PR switches from nose to pytest as requested in #389, but has not tried to refactor any of the tests.

Nice. Good idea to not refactor anything yet I think, better to leave that for a separate PR.

1.) Do we need to remove the ability to run tests via pywt.test() or is there a way to maintain that without requiring nose? It is still present currently, but uses nose indirectly via NumPy and there are a couple of failures now when running the test this way because skips related to optional dependency pymatbridge were not respected.

Would be nice to keep it. Copying numpy._pytesttester should be an easy fix.

@rgommers
Copy link
Member

The Appveyor tests failed with an odd import error.

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 19, 2019

Appveyor is passing now. Not sure if this is the best way to call pytest, but adding --pyargs pywt seems to work.

@rgommers rgommers added this to the v1.1 milestone Mar 19, 2019
@rgommers
Copy link
Member

Not sure if this is the best way to call pytest, but adding --pyargs pywt seems to work.

Not really sure either, I can't remember the pytest cli most of the time because I always use .test(). It works and looks understandable, so fine to keep as is I'd say.

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 26, 2019

I think we should probably remove the use of NumPy's Tester which they no longer use internally. As of now, their __init__.py contains the following code/comment:

    # We don't actually use this ourselves anymore, but I'm not 100% sure that
    # no-one else in the world is using it (though I hope not)
    from .testing import Tester

    # Pytest testing
    from numpy._pytesttester import PytestTester
    test = PytestTester(__name__)
    del PytestTester

The new PytestTester class is not part of the NumPy API, but it looks fairly simple if we just wanted to make our own version of that class to use here.

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 27, 2019

I added a PytestTester class (and pytest.ini file) based on one in NumPy in the last two commits here.
Does this seem like a reasonable approach? After this change, tests pass again when running

import pywt
pywt.test()

and nose is no longer used for this functionality (the user must have pytest installed to run the tests in this way).

If we decide to do this, do we need to also append the NumPy BSD license to our LICENSE file? (Along the same lines, I think some files in ./util that originate in NumPy or SciPy)?

@rgommers
Copy link
Member

That looks good to me, thanks!

If we decide to do this, do we need to also append the NumPy BSD license to our LICENSE file? (Along the same lines, I think some files in ./util that originate in NumPy or SciPy)?

Good point. Not the whole license, but a short snippet I think, similar to https://github.com/numpy/numpy/blob/master/LICENSE.txt#L34

This is also consistent with the first answer I found on stackoverflow:
https://softwareengineering.stackexchange.com/questions/278126/is-it-legal-to-include-bsd-licensed-code-within-an-mit-licensed-project

So I suggest something like:

The PyWavelets repository and source distributions bundle some code that is adapted from
compatibly licensed projects. We list these here.

Name: NumPy
Files:  pywt/_pytesttester.py
License: 3-clause BSD

Name: SciPy
Files:  setup.py, util/*
License: 3-clause BSD

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 28, 2019

That sounds good. I have added the text you suggested to the main LICENSE file.

I also squashed a few of the trial/error commits related to Appveyor to clean up the commit history a bit.

@rgommers
Copy link
Member

Awesome. All green, so in it goes. Thanks!

@rgommers rgommers merged commit d68a6bb into PyWavelets:master Mar 28, 2019
@grlee77 grlee77 deleted the pytest branch November 13, 2019 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change test suite from nose to pytest
2 participants