Pytest assert functions #1147

max-sixty · 2016-12-01T04:36:56Z

Is this a reasonable function to use with py.test? In place of the inheritance .assertVariableEqual etc.

Do we want separate functions for each class?
There's a comment or just switch to py.test and add an appropriate hook. - is that different to what's here?
This still needs to be completed, with additional functions for assert _identical and assert_approx_equal.
Do we want more description for the failure, above just printing the arguments? That can also be added later

shoyer · 2016-12-02T04:11:06Z

Do we want separate functions for each class?

That seems like overkill -- I think one is fine.

There's a comment or just switch to py.test and add an appropriate hook. - is that different to what's here?

I was thinking of something like how python lets you override the message for assert a == b. But it actually looks like there is no suitable hook for custom explanations when using methods. __traceback__ = False, like you do here, is about the best you can do.

Do we want more description for the failure, above just printing the arguments? That can also be added later

It's nice to at least print the objects on separate lines, e.g., using something like '%r\n%r' % (a, b) for the assert message. Otherwise multi-line reprs end up with the start of the second object on the same line as the first.

shoyer · 2016-12-02T03:58:46Z

xarray/test/__init__.py

+        assert as_variable(a).equals(b), (a, b)
+    elif isinstance(a, xr.Dataset):
+        assert a.equals(b), (a, b)
+    elif isinstance(a, dict):  # coords


coords are actually not dict subclasses, so this won't work. They do subclass collections.Mapping, though.

shoyer · 2016-12-02T03:59:10Z

xarray/test/__init__.py

+    import xarray as xr
+    ___tracebackhide__ = True
+    assert type(a) == type(b)
+    if isinstance(a, xr.DataArray):


I think you have this mixed up with the Dataset clause?

shoyer · 2016-12-02T03:59:33Z

xarray/test/__init__.py

+        assert_equal(a.data_vars, b.data_vars)
+        assert_equal(a.coords, b.coords)
+    elif isinstance(a, xr.Variable):
+        assert as_variable(a).equals(b), (a, b)


No need to convert a to a variable -- it already is one.

shoyer · 2016-12-02T04:00:31Z

xarray/test/__init__.py

+    elif isinstance(a, xr.Variable):
+        assert as_variable(a).equals(b), (a, b)
+    elif isinstance(a, xr.Dataset):
+        assert a.equals(b), (a, b)


I think this (assert a.equals(b), (a, b)) would actually probably suffice for all of Dataset, DataArray and Variable.

max-sixty · 2016-12-02T21:55:47Z

Updated.

Is it worth 'testing the tests'? I couldn't find tests for the existing functions

shoyer · 2016-12-08T08:51:10Z

xarray/test/__init__.py

+        assert sorted(a, key=str) == sorted(a, key=str)
+        assert_xarray_equal(a.coords, b.coords)
+        [assert_xarray_close(
+            a.variables[k], b.variables[k], rtol=1e-05, atol=1e-08)


use rtol=rtol, atol=atol here

shoyer · 2016-12-08T08:52:11Z

xarray/test/__init__.py

+        assert_xarray_equal(a.data_vars, b.data_vars)
+        assert_xarray_equal(a.coords, b.coords)
+    elif isinstance(a, (xr.Variable, xr.DataArray, xr.Coordinate)):
+        assert a.equals(b), '{}/n{}'.format(a, b)


use \n not /n here and below

shoyer · 2016-12-08T08:52:52Z

xarray/test/__init__.py

+    if isinstance(a, xr.Dataset):
+        assert_xarray_equal(a.data_vars, b.data_vars)
+        assert_xarray_equal(a.coords, b.coords)
+    elif isinstance(a, (xr.Variable, xr.DataArray, xr.Coordinate)):


Coordinate (actually IndexVariable, now) is a Variable subclass, so you don't need to call it out separately

shoyer · 2016-12-08T08:55:34Z

xarray/test/__init__.py

+    import xarray as xr
+    ___tracebackhide__ = True
+    assert type(a) == type(b)
+    if isinstance(a, xr.Dataset):


I would just use .equals for Dataset, as well -- no need to handle data_vars and coord separately

shoyer · 2016-12-08T08:56:49Z

xarray/test/__init__.py

+        assert_xarray_equal(a.coords, b.coords)
+    elif isinstance(a, (xr.Variable, xr.DataArray, xr.Coordinate)):
+        assert a.equals(b), '{}/n{}'.format(a, b)
+    elif isinstance(a, xr.core.coordinates.AbstractCoordinates):


I'm not sure we really need need this case

max-sixty · 2016-12-08T17:46:37Z

Thanks for the feedback! Updated.

max-sixty · 2016-12-09T03:56:51Z

xarray/test/__init__.py

+    assert type(a) == type(b)
+    if isinstance(a, (xr.Variable, xr.DataArray, xr.Dataset)):
+        assert a.equals(b), '{}\n{}'.format(a, b)
+    elif isinstance(a, xr.core.coordinates.AbstractCoordinates):


I think this is needed for coordinates - let me know if there's a better way

max-sixty · 2016-12-12T22:17:28Z

@shoyer green

shoyer · 2016-12-15T02:10:53Z

This looks good to me. The last thing I would do is switch the existing test methods like assertDatasetEqual to use these functions, just to reduce the amount of redundant code (and also test your new functions more extensively).

max-sixty · 2016-12-15T04:17:44Z

The last thing I would do is switch the existing test methods like assertDatasetEqual to use these functions, just to reduce the amount of redundant code (and also test your new functions more extensively).

All of tests? I think that's a fairly heavy lift for this PR. It also happens to be really tedious and unfortunately I don't think Find / Replace-able... If you feel strongly I'll plug in and go through though!

Otherwise we can roll out over time as people write new tests

shoyer · 2016-12-15T04:30:50Z

No no no... Just the definitions of the methods like assertDatasetEqual on the bass xarray TestCase class, which you can now define as aliases to your new functions.

…

On Wed, Dec 14, 2016 at 8:17 PM Maximilian Roos ***@***.***> wrote: The last thing I would do is switch the existing test methods like assertDatasetEqual to use these functions, just to reduce the amount of redundant code (and also test your new functions more extensively). All of tests? I think that's a fairly heavy lift for this PR. It also happens to be really tedious and unfortunately I don't think Find / Replace-able... If you feel strongly I'll plug in and go through though! Otherwise we can roll out over time as people write new tests — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1147 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1hhpkMSPNJZ4mPZ-ncrUwRBuYVfeks5rIL9ogaJpZM4LA_ql> .

shoyer · 2016-12-15T04:36:07Z

xarray/test/__init__.py

+        raise TypeError('{} not supported by assertion comparison'
+                        .format(type(a)))
+
+def assert_xarray_close(a, b, rtol=1e-05, atol=1e-08):


It might be slightly more consistent to call this allclose rather than close.

Why allclose but not allequal?

This is the name of the functions in numpy.testing

shoyer · 2016-12-15T04:45:17Z

You might also add these to the API docs page and what's new. I know there has been interest in public test functions for quite some time.

fmaussion · 2016-12-15T10:24:31Z

I know there has been interest in public test functions for quite some time.

Yes, see also #754

max-sixty · 2016-12-15T20:46:23Z

@shoyer do you have ideas for when the types are not the same? For example, many of the tests compare Variable to IndexVariable. Should we not check types? Add a check_type=True argument to the testing functions? Change the tests so they're compliant?

shoyer · 2016-12-15T20:50:17Z

In most cases I would guess these are broken tests?

max-sixty · 2016-12-15T21:56:50Z

There are more than a dozen: https://travis-ci.org/pydata/xarray/jobs/184356287#L2611

Also some binary / unicode errors - let me know if you have context

shoyer · 2016-12-16T03:30:36Z

xarray/test/__init__.py

+        assert_xarray_equal(a.coords, b.coords)
+    elif isinstance(a, xr.Dataset):
+        assert sorted(a, key=str) == sorted(a, key=str)
+        assert_xarray_equal(a.coords, b.coords)


I think this is why many (not all) of the new tests are failing (at least the unicode/bytes ones, because we have a hack that says unicode/bytes can still be "close" if they encode the same string). Previously we used allclose for all variables, now coords are required to be equal.

I can see why this makes sense, but maybe better to keep with what we have now or add a keyword argument to switch.

max-sixty · 2016-12-21T01:37:30Z

@shoyer I fixed most of the tests. Will come back and check the remaining few failures

I didn't quite realize how many .assertVariable... tests were relying on coercion, so I ended up changing the tests in a lot of places. The test is explicit at the test site, at least.

There were a couple that I think are probably breaks - things like .argsort changing the type of a Variable. I marked those with comments but haven't fixed them - or we'll never get this in! Let me know if you have thoughts on any notes

max-sixty · 2016-12-21T04:59:30Z

Rebased on #1175, so that should be merged first if people agree

… instance

fmaussion · 2016-12-21T09:48:29Z

xarray/test/__init__.py



 def requires_netCDF4(test):
-    return test if has_netCDF4 else unittest.skip('requires netCDF4')(test)
+    return test if has_netCDF4 else pytest.mark.skip('requires dask')(test)



dask -> netCDF4

fmaussion · 2016-12-21T09:51:03Z

This is great! Before merge it would be good to add the new standalone assert_* functions to the API doc?

max-sixty · 2016-12-21T17:35:16Z

@fmaussion had a go - is that what you were thinking?

fmaussion · 2016-12-21T18:32:10Z

Thanks @MaximilianR , this will be useful for all libraries downstream of xarray

spencerahill · 2016-12-21T22:41:55Z

This is great. At least in terms of the functionality I was looking for, I'd say this closes #754.

shoyer

Thanks @MaximilianR. I have a few more minor adjustments but this is very close.

shoyer · 2016-12-21T23:15:44Z

xarray/test/__init__.py

+            [assert_xarray_allclose(a[k], b[k], rtol=rtol, atol=atol) for k in a.variables]
+        else:
+            # unsure if we should need this branch
+            # https://github.com/pydata/xarray/issues/1152


Per my comment in #1152, DataArray does not have a .variables attribute. I think we should update this case to test both .variable and everything in .coords.variables.

🤦‍♂️

shoyer · 2016-12-21T23:17:16Z

xarray/test/__init__.py

+    elif isinstance(a, xr.Dataset):
+        assert sorted(a, key=str) == sorted(a, key=str)
+        [assert_xarray_allclose(a[k], b[k], rtol=rtol, atol=atol)
+         for k in list(a.variables) + list(a.coords)]


Just a minor style point, but I find it a little unexpected to use a list comprehension when throwing away the result. I would write this with a normal for loop:

for k in list(a.variables) + list(a.coords): assert_xarray_allclose(a[k], b[k], rtol=rtol, atol=atol)

shoyer · 2016-12-21T23:18:34Z

xarray/test/test_backends.py

@@ -912,7 +913,12 @@ def test_cross_engine_read_write_netcdf3(self):
                    for read_engine in valid_engines:
                        with open_dataset(tmp_file,
                                          engine=read_engine) as actual:
-                            self.assertDatasetAllClose(data, actual)
+                            # hack to allow test to work:


This is fine for now, but we should look into this later because it looks like a legit bug to me.

It might be worth making a dedicated VariablesDict that is simply an OrderedDict with runtime type checking that verifies you can never put in anything other than a Variable. Or we should use pytype for static checks to ensure Dataset._variables and DataArray._coords are typed typing.OrderedDict[Any, xarray.Variable].

Agreed it's a bug

shoyer · 2016-12-21T23:25:03Z

xarray/test/test_variable.py

+
+        # should we need `.to_base_variable()`?
+        # probably a break that `+v` changes type?
+        v = self.cls(['x'], x).to_base_variable()


I think this it's OK that any arithmetic converts IndexVariable to Variable. That seems pretty consistent to me.

Instead of always converting v to a base variable, I would make another copy that is the base variable and using that for the expected variable. Thus these tests will still check that math with IndexVariable works even though it doesn't preserve the type.

shoyer · 2016-12-21T23:27:58Z

xarray/test/test_variable.py

        for actual in [expected.T,
                       expected[...],
                       expected.squeeze(),
                       expected.isel(x=slice(None)),
                       expected.expand_dims({'x': 3}),
                       expected.copy(deep=True),
                       expected.copy(deep=False)]:
+
            self.assertVariableIdentical(expected, actual)


Use .to_base_variable() in the assert here (on both sides). Otherwise this test doesn't have any coverage for IndexVariable objects.

shoyer · 2016-12-21T23:28:51Z

xarray/test/test_variable.py

@@ -304,22 +309,23 @@ def test_equals_all_dtypes(self):
    def test_eq_all_dtypes(self):
        # ensure that we don't choke on comparisons for which numpy returns
        # scalars
-        expected = self.cls('x', 3 * [False])
+        expected = self.cls('x', 3 * [False]).to_base_variable()


Note that you can just use Variable('x', 3 * [False]) in this case and other ones like it, which is maybe a little clearer.

max-sixty · 2016-12-22T04:23:49Z

@shoyer green!

shoyer · 2016-12-22T18:35:03Z

xarray/test/test_variable.py

        for v, _ in self.example_1d_objects():
            actual = 'z' == v
            self.assertVariableIdentical(expected, actual)
            actual = ~('z' != v)
            self.assertVariableIdentical(expected, actual)

    def test_encoding_preserved(self):
-        expected = self.cls('x', range(3), {'foo': 1}, {'bar': 2}).to_base_variable()
+        expected = Variable('x', range(3), {'foo': 1}, {'bar': 2})


This should still be self.cls, not Variable.

max-sixty · 2016-12-22T19:07:08Z

@shoyer green!

shoyer reviewed Dec 2, 2016

View reviewed changes

shoyer reviewed Dec 8, 2016

View reviewed changes

max-sixty changed the title ~~RFC for initial pytest assert function~~ Pytest assert functions Dec 9, 2016

max-sixty commented Dec 9, 2016

View reviewed changes

shoyer reviewed Dec 15, 2016

View reviewed changes

shoyer reviewed Dec 16, 2016

View reviewed changes

max-sixty mentioned this pull request Dec 21, 2016

Scalar coords seep into index coords #1152

Closed

max-sixty added 9 commits December 21, 2016 01:22

RFC for initial pytest assert function

f30d7cd

functions all filled out

83ae3c6

tweaks

25d09fb

only run invalid args test with bn

1af5a50

move existing assert funcs to wrappers of new funcs

f2e8209

typo

994c3e2

exclude DS_Store

95a99eb

allclose

a80829c

WIP

b09f9cc

max-sixty added 6 commits December 21, 2016 01:22

a few fixes to existing tests

9fa25dd

should have realized earlier to change the test func rather than each…

e7215b4

… instance

patch tests even if potentially broken

8bf4ae3

note on skipping import tests

4dac537

remove old functions

e77cfa0

test change to get 3.3 build to pass

f0cb0ae

fmaussion reviewed Dec 21, 2016

View reviewed changes

max-sixty added 2 commits December 21, 2016 11:13

api and whatsnew

c1e59aa

typo

378e713

shoyer reviewed Dec 21, 2016

View reviewed changes

final changes

91b3abc

shoyer reviewed Dec 22, 2016

View reviewed changes

final final change

dcf4f05

shoyer merged commit 9dcfa73 into pydata:master Dec 22, 2016

Pytest assert functions #1147

Pytest assert functions #1147

Conversation

max-sixty commented Dec 1, 2016

shoyer commented Dec 2, 2016

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

max-sixty commented Dec 2, 2016

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

max-sixty commented Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

max-sixty commented Dec 12, 2016

shoyer commented Dec 15, 2016

max-sixty commented Dec 15, 2016

shoyer commented Dec 15, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Dec 15, 2016

fmaussion commented Dec 15, 2016

max-sixty commented Dec 15, 2016

shoyer commented Dec 15, 2016

max-sixty commented Dec 15, 2016

Choose a reason for hiding this comment

max-sixty commented Dec 21, 2016 • edited Loading

max-sixty commented Dec 21, 2016

Choose a reason for hiding this comment

fmaussion commented Dec 21, 2016

max-sixty commented Dec 21, 2016

fmaussion commented Dec 21, 2016

spencerahill commented Dec 21, 2016

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

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

max-sixty commented Dec 22, 2016

Choose a reason for hiding this comment

max-sixty commented Dec 22, 2016

max-sixty commented Dec 8, 2016 •

edited

Loading

max-sixty commented Dec 21, 2016 •

edited

Loading