Skip to content

Make tests optional that depend on pydot and pandas (optional installs).#479

Merged
rhattersley merged 1 commit intoSciTools:masterfrom
pp-mo:options_tests
May 15, 2013
Merged

Make tests optional that depend on pydot and pandas (optional installs).#479
rhattersley merged 1 commit intoSciTools:masterfrom
pp-mo:options_tests

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Apr 29, 2013

Failed on these in my amazon instance VM.

We could probably do with an "official" list of what installs developers should use?
See also #478 -- but possibly this should have gone on discussion groups instead??

@pp-mo
Copy link
Member Author

pp-mo commented May 2, 2013

Re: recent change (pp-mo@d9451c3)
Generalised as now want to make other tests 'optional' too (specifically for #453)

@rhattersley
Copy link
Member

The generalised version is certainly clever, but I'm not sure it's worth it since there's still quite a lot of boilerplate left in the client module.

How about if the client code (e.g. iris.tests.test_pandas) just did:

# Since pandas is an optional dependency, we disable all these tests
# if it's not installed.
try:
    import pandas
    import iris.pandas
    skip_pandas = lambda fn: fn
except ImportError:
    skip = unittest.skip('Test(s) require pandas, which is not installed.')
    skip_pandas = lambda fn: skip(fn)

I'd have more time for the generalised version if it could be used more like:

pandas, skip_pandas = tests.skip_module('pandas')
if pandas is not None:
    import iris.pandas

@pp-mo
Copy link
Member Author

pp-mo commented May 2, 2013

Thanks @rhattersley . But you're reading my mind now... That's just the thought process I've been going through since pushing this (which I now wish I hadn't).
There may even still be an idiom here that's as good as a definition. I'm looking into it..

@pp-mo
Copy link
Member Author

pp-mo commented May 2, 2013

The generalised version is certainly clever, but I'm not sure it's worth it since there's still quite a lot of boilerplate left in the client module.

I'm tending to agree, and I think it's more obvious to leave the workings visible in the test file.
Changes here now done that way : pp-mo@88919fd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be simplified to just unittest.skipIf(...) with a condition clause based on dot.DOT_AVAILABLE?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps ditch DOT_AVAILABLE altogether since it's only used here and just test the executable path directly.

@ghost ghost assigned rhattersley May 14, 2013
@pp-mo
Copy link
Member Author

pp-mo commented May 14, 2013

That's a nasty catch in the new licence header checks
(cf. https://travis-ci.org/SciTools/iris/builds/7167947)
It fails if you don't have spaces around the dash in "YY1Y - YY2Y", and doesn't tell you which file is to blame..

@pp-mo
Copy link
Member Author

pp-mo commented May 14, 2013

Thanks @rhattersley, @esc24 .
I'm hoping this latest addresses all comments so far.
Please re-review.

@rhattersley
Copy link
Member

Please re-review.

👍 Looks good enough for me (at cfdd8483dc559e969ddd7fa26795dc5908deb9a6). Please squash and I'll merge.

@pp-mo
Copy link
Member Author

pp-mo commented May 15, 2013

@rhattersley Please squash and I'll merge.

Thanks, done that. All re-tested, should be ok this time.

@rhattersley
Copy link
Member

Thanks @pp-mo. 😄

rhattersley added a commit that referenced this pull request May 15, 2013
Make tests optional that depend on pydot and pandas (optional installs).
@rhattersley rhattersley merged commit 07c781d into SciTools:master May 15, 2013
@esc24
Copy link
Member

esc24 commented May 15, 2013

Great stuff @pp-mo. These kind of contributions, although not the most exciting features to work on, are really important.

@pp-mo pp-mo deleted the options_tests branch May 15, 2013 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants