issue_comments

@shoyer green!

@shoyer green!

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

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

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

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

Rebased on https://github.com/pydata/xarray/pull/1175, so that should be merged first if people agree

@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

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

In most cases I would guess these are broken tests?

@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?

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

Yes, see also https://github.com/pydata/xarray/issues/754

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.

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 notifications@github.com 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 https://github.com/pydata/xarray/pull/1147#issuecomment-267234900, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1hhpkMSPNJZ4mPZ-ncrUwRBuYVfeks5rIL9ogaJpZM4LA_ql .

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

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).

@shoyer green

Thanks for the feedback! Updated.

Updated.

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

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.