fix unintentional skipped tests #1557

jhamman · 2017-09-07T04:47:27Z

Closes @requires_pinio mass disables unrelated tests #1531
Tests added / passed
Passes git diff upstream/master | flake8 --diff
Fully documented, including whats-new.rst for all changes and api.rst for new API

As @crusaderky pointed out, just moving the @requires_pynio decorator inside the TestPyNio class seems to fix the problems described in #1531. It looks like requires_pynio is nullifying CFEncodedDataTest, Only32BitTypes, TestCase in addition to TestPyNio, hence the propagation.

This PR also includes a cleanup of tests/__init__.py.

crusaderky

What about the other @requires class decorators?

fmaussion · 2017-09-07T13:50:48Z

I wonder if this propagation to parent classes is a feature or a bug? Seems quite messy to me

jhamman · 2017-09-07T14:15:00Z

What about the other @requires class decorators?

I had this thought too. We don't decorate too many classes but I image we have other issues.

I wonder if this propagation to parent classes is a feature or a bug?

I would think this is a bug. I'm working on a simplified example to report this to pytest.

shoyer

Thanks for taking this on!

I wonder if it might work to use pytest's importorskip inside the roundtrip() methods?

shoyer · 2017-09-07T17:02:36Z

xarray/tests/__init__.py

+has_h5netcdf, requires_h5netcdf = _importorskip('h5netcdf')
+has_pynio, requires_pynio = _importorskip('pynio')
+has_dask, requires_dask = _importorskip('dask')
+has_bottleneck, requires_bottleneck = _importorskip('bottleneck', '1.0')


bottleneck 1.0 came out in Feb 2015. I'm not sure it's worth having the version check here.

shoyer · 2017-09-07T17:07:36Z

xarray/tests/test_backends.py

@@ -1529,6 +1527,8 @@ def roundtrip(self, data, save_kwargs={}, open_kwargs={},
                              autoclose=self.autoclose, **open_kwargs) as ds:
                yield ds

+    @requires_scipy
+    @requires_pynio


I don't think this is enough -- the other methods defined on super-classes should also need pynio to pass for this TestCase. Looking at the test log on Travis, I see a lot of xfail and XPASS test cases, so maybe they are getting disabled by something else?

totally agree, this move does fix most of the issues but there are some additional issues. Now that we know what the problem is and have confirmation that this is a bug from the pytest team, I'll work on a more robust solution.

shoyer · 2017-09-07T17:54:38Z

Maybe it's worth trying the work-around from pytest-dev/pytest#568 (comment)

…kends.py

jhamman · 2017-09-07T18:34:48Z

Maybe it's worth trying the work-around from pytest-dev/pytest#568 (comment)

This gets us quite close but doesn't allow us to have use multiple decorators at once. I'll keep digging.

shoyer · 2017-10-02T16:12:22Z

Another option is to give up on class decorators and only use a method decorator -- which we could even write ourselves if necessary. We have most of us backend specific logic in a few helper functions that we override for each subclass, so we only really need to decorate those, e.g.,

def conditional_skip(condition, reason=''):
    def wrapped(*args, **kwargs):
        if condition:
            raise unittest.SkipTest(reason)
        return wrapped(*args, **kwargs)
    return wrapped

class NetCDFSubclassTest(UnitTest):
    ...
    @contextmanager
    @conditional_skip(...)
    def roundtrip(self):
        ...

shoyer

Looks great, thank you!

Can we note this as "Bug fix", since this is probably relevant to redistributors?

shoyer · 2017-10-03T03:22:05Z

xarray/tests/__init__.py

+def _importorskip(modname, minversion=None):
+    try:
+        mod = importlib.import_module(modname)
+        has = True


put everything from here on down an the else clause

shoyer · 2017-10-03T03:25:08Z

xarray/tests/test_backends.py

-@pytest.mark.xfail
-class H5NetCDFDataTestAutocloseTrue(H5NetCDFDataTest):
-    autoclose = True
+# @pytest.mark.xfail  @shoyer - is this still an issue?


Yes, I'm afraid so. It's probably an h5py issue -- not really clear what's going on.

reminder to myself to remove this comment

shoyer · 2017-10-03T03:25:48Z

xarray/tests/__init__.py

+                raise ImportError('Minimum version not satisfied')
+    except ImportError:
+        has = False
+    func = pytest.mark.skipif((not has), reason='requires {}'.format(modname))


remove redundant parentheses around (not has).

shoyer

Looks great, thanks!

shoyer · 2017-10-03T19:01:27Z

doc/whats-new.rst

@@ -212,6 +212,11 @@ Bug fixes
  the first argument was a numpy variable (:issue:`1588`).
  By `Guido Imperiale <https://github.com/crusaderky>`_.

+  - Fix bug when using ``pytest`` class decorators to skiping certain unittests.
+    The previous behavior unintentionally causing additional tests to be skipped.
+    A temporary work-around has been applied in :issue:`1531`.


This is temporary for xarray, but from a user (or distributor) perspective the bug is fixed. I would probably drop that part.

jhamman

I don't think this is ready to merge. I have a few small syntax/cleanup things to fix but more importantly, the TestPyNioAutocloseTrue test class is not being skipped correctly.

jhamman · 2017-10-04T00:21:37Z

xarray/tests/test_backends.py

-@pytest.mark.xfail
-class H5NetCDFDataTestAutocloseTrue(H5NetCDFDataTest):
-    autoclose = True
+# @pytest.mark.xfail  @shoyer - is this still an issue?


reminder to myself to remove this comment

jhamman · 2017-10-04T00:22:30Z

xarray/tests/test_backends.py

+    @requires_pynio
+    def setUp(self):
+        pass
+


This doesn't seem to be working. I'll try to come up with something else but if anyone has any ideas for how to get around this, I'm all ears.

Does it work if you hand-write the test-skipping decorator to raise unittest.SkipTest rather than relying on pytest.mark.skipif?

shoyer · 2017-10-04T00:50:40Z

xarray/tests/test_plot.py

@@ -91,6 +90,7 @@ def contourf_called(self, plotmethod):

 class TestPlot(PlotTestCase):

+    @requires_matplotlib


We don't need to move the requires_matplotlib decorator, since we don't rely on inheritance for enabling/disabling tests.

shoyer · 2017-10-04T00:54:35Z

xarray/tests/test_backends.py

+    @requires_pynio
+    def setUp(self):
+        pass
+


Does it work if you hand-write the test-skipping decorator to raise unittest.SkipTest rather than relying on pytest.mark.skipif?

jhamman · 2017-10-04T21:41:45Z

@shoyer - ready for a final review. Turns out, we can just use the unittest decorators. Basically no changes to the test code base :).

shoyer

Even better, thank you!

shoyer · 2017-10-04T22:49:50Z

xarray/tests/__init__.py

-)
-
+        has = False
+    # TODO: use pytest skip


can you briefly elaborate on why pytest.skip doesn't work? a link to this issue would be heplful.

shoyer · 2017-10-04T23:10:44Z

LGTM, thanks

Joe Hamman added 2 commits September 6, 2017 21:42

move requires_pynio to fix GH1531, refactor tests/__init__.py

e98bf36

remove ModuleNotFoundError

36ad7c3

crusaderky reviewed Sep 7, 2017

View reviewed changes

jhamman mentioned this pull request Sep 7, 2017

skipif propagates through multiple inheritance pytest-dev/pytest#2759

Closed

4 tasks

shoyer reviewed Sep 7, 2017

View reviewed changes

no pytest decorators on classes with multiple inheritance in test_bac…

8768d65

…kends.py

wrap class decorators for tests

18e01c9

jhamman added needs work topic-testing labels Sep 14, 2017

jhamman mentioned this pull request Oct 2, 2017

WIP: Zarr backend #1528

Merged

4 tasks

Joe Hamman added 2 commits October 2, 2017 15:21

Merge branch 'master' of github.com:pydata/xarray into fix/1531

0220da5

no more class decorators in test_backends

27b635b

shoyer reviewed Oct 3, 2017

View reviewed changes

whats new and some more cleanup

5b4c966

shoyer approved these changes Oct 3, 2017

View reviewed changes

whatsnew update

5f73351

shoyer approved these changes Oct 3, 2017

View reviewed changes

jhamman commented Oct 4, 2017

View reviewed changes

shoyer reviewed Oct 4, 2017

View reviewed changes

jhamman changed the title ~~move requires_pynio~~ stop using class decorators with pytest Oct 4, 2017

use unittest skip decorators, way easier

6651516

jhamman changed the title ~~stop using class decorators with pytest~~ fix unintentional skipped tests Oct 4, 2017

jhamman requested a review from rabernat October 4, 2017 21:42

shoyer approved these changes Oct 4, 2017

View reviewed changes

update workaround comment

993992d

jhamman merged commit e713668 into pydata:master Oct 4, 2017

jhamman deleted the fix/1531 branch October 4, 2017 23:12

jhamman mentioned this pull request Jan 3, 2018

Documentation about contributing zarr-developers/zarr-python#227

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix unintentional skipped tests #1557

fix unintentional skipped tests #1557

jhamman commented Sep 7, 2017 •

edited

Loading

crusaderky left a comment

fmaussion commented Sep 7, 2017

jhamman commented Sep 7, 2017 •

edited

Loading

shoyer left a comment

shoyer Sep 7, 2017

shoyer Sep 7, 2017

jhamman Sep 7, 2017

shoyer commented Sep 7, 2017

jhamman commented Sep 7, 2017

shoyer commented Oct 2, 2017 •

edited

Loading

shoyer left a comment

shoyer Oct 3, 2017

shoyer Oct 3, 2017

jhamman Oct 4, 2017

shoyer Oct 3, 2017

shoyer left a comment

shoyer Oct 3, 2017

jhamman left a comment

jhamman Oct 4, 2017

jhamman Oct 4, 2017

shoyer Oct 4, 2017

shoyer Oct 4, 2017

shoyer Oct 4, 2017

jhamman commented Oct 4, 2017

shoyer left a comment

shoyer Oct 4, 2017

shoyer commented Oct 4, 2017

		@@ -91,6 +90,7 @@ def contourf_called(self, plotmethod):

		class TestPlot(PlotTestCase):

		@requires_matplotlib

fix unintentional skipped tests #1557

fix unintentional skipped tests #1557

Conversation

jhamman commented Sep 7, 2017 • edited Loading

crusaderky left a comment

Choose a reason for hiding this comment

fmaussion commented Sep 7, 2017

jhamman commented Sep 7, 2017 • edited Loading

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Sep 7, 2017

jhamman commented Sep 7, 2017

shoyer commented Oct 2, 2017 • edited Loading

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamman commented Oct 4, 2017

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Oct 4, 2017

jhamman commented Sep 7, 2017 •

edited

Loading

jhamman commented Sep 7, 2017 •

edited

Loading

shoyer commented Oct 2, 2017 •

edited

Loading