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.

Sign up for GitHub

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

Jump to bottom

Improve testing framework and infrastructure #1454

Merged
merged 9 commits into from
Aug 26, 2020
Merged

Improve testing framework and infrastructure #1454

merged 9 commits into from
Aug 26, 2020

Conversation

dopplershift
Copy link
Member

@dopplershift dopplershift commented Aug 8, 2020 •
edited
Loading

Description Of Changes

  • Make it possible to run tests without cartopy/pyproj
  • Add CI run to check this configuration
  • Refactor XArray support to no longer unconditionally import CartoPy

While it's nice to be able to have people run the tests with our so-called minimum dependencies, more importantly this allows us to have a CI configuration that proves to us that this is the case. This is not theoretical, at various points we've accidentally had a top-level import of pyproj (even though it's supposed to be "optional"), and before this PR on master we could not import MetPy without CartoPy installed.

This also makes it clear how much of our functionality relies on "optional" dependencies, which is a discussion for another issue...

Checklist

@dopplershift dopplershift requested a review from dcamron as a code owner August 8, 2020 20:03
@dopplershift dopplershift added Area: Infrastructure Pertains to project infrastructure (e.g. CI, linting) Area: Tests Affects tests Type: Enhancement Enhancement to existing functionality labels Aug 8, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2020

This pull request introduces 2 alerts when merging aa1e64d into 3569494 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2020

This pull request introduces 2 alerts when merging d3b10af into 345ddd0 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2020

This pull request introduces 2 alerts when merging e6ca470 into 345ddd0 - view on LGTM.com

new alerts:

  • 2 for Unused import

@dopplershift dopplershift force-pushed the improve-tests branch 3 times, most recently from 39f740b to 11f7e75 Compare August 9, 2020 06:02
@dopplershift dopplershift mentioned this pull request Aug 9, 2020
Closed
@jthielen
Copy link
Collaborator

jthielen commented Aug 9, 2020

#1455 may make this comment defunct, but would you be able to make the import cartopy.crs as ccrs in src/metpy/plots/mapping.py guarded so that CFProjection is still available without CartoPy? That way more of the xarray accessor should work without CartoPy.

@dopplershift
Copy link
Member Author

Ok, so now we have a function for importing cartopy.crs where it makes sense to do it conditionally. In that case you replace:

import cartopy.crs as ccrs

with

ccrs = import_cartopy()

If no cartopy.crs is available, it returns a stub object that gives a useful error message when an attribute from ccrs is accessed, thus converting our import-time failure to a purely run-time one.

Doing this and restoring CFProjection as always available means we now skip only 109 tests rather than 147. Beyond that is going to take making the various parts of projection math always available (i.e. #1455)

@dopplershift dopplershift added this to the 1.0 milestone Aug 23, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 23, 2020

This pull request introduces 1 alert when merging ecb4976 into 9acac60 - view on LGTM.com

new alerts:

  • 1 for Non-standard exception raised in special method

@jthielen
Copy link
Collaborator

jthielen commented Aug 23, 2020 •
edited
Loading

This looks good to me, thanks for doing those updates! I might start playing around with #1455 based off of this branch.

dcamron
dcamron requested changes Aug 24, 2020
View reviewed changes
Copy link
Member

@dcamron dcamron left a comment

Choose a reason for hiding this comment

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

Everything else looks good!

src/metpy/testing.py Outdated Show resolved Hide resolved
src/metpy/testing.py Outdated Show resolved Hide resolved
@dopplershift
CI: Add CI run without the optional dependencies
3a355e1
@dopplershift
CI: Eliminate experimental flag from test workflow
e10ef6c 
Just check for Prerelease. This simplifies the matrix configuration and
eliminates duplicate state to maintain.
@dopplershift
TST: Eliminate using np.random
f63215a 
At a minimum, seeding the random number generator needs to be within the
fixture, otherwise tests become order-dependent. Since none of the tests
are using the values, just use zeros.
@dopplershift
MNT: Refactor to eliminate xarray importing cartopy
67f34a4 
Instead, provide the Geodetic CRS as an attribute on CFProjection.
@dopplershift
MNT: Make cartopy_utils safe to import without CartoPy
cda09dc
@dopplershift
TST: Skip appropriate tests when no CartoPy present
9dbfe43 
This allows us to run the test suite without CartoPy. This adds two
global fixtures that allow tests that need either cartopy.crs or
cartopy.feature to have fixture paramters that provide it rather than
import the library--the tests are skipped if the import fails. There's
also a `needs_cartopy` decorator used to mark tests that need cartopy to
function (usually because of `parse_cf`).
@dopplershift
MNT: Improve conditional CartoPy handling
d484cd9 
Add a function that handles importing cartopy.crs. If this is not
importable, a stub will be returned that will emit a useful error
message--but only when something is actually used from CartoPy.
@dopplershift
TST: Skip appropriate tests when no PyProj present
b2d4fff 
This allows us to run the tests without installing PyProj. It adds a
decorator for test functions/fixtures that will skip if importing pyproj
fails.
@dopplershift
TST: Mark a couple tests that need both cartopy and pyproj
2c77a74
@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2020

This pull request introduces 1 alert when merging 2c77a74 into 931c114 - view on LGTM.com

new alerts:

  • 1 for Non-standard exception raised in special method

@dopplershift dopplershift merged commit cdeb8c7 into Unidata:master Aug 26, 2020
@dopplershift dopplershift deleted the improve-tests branch August 26, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcamron dcamron dcamron approved these changes

Assignees
No one assigned
Labels
Area: Infrastructure Pertains to project infrastructure (e.g. CI, linting) Area: Tests Affects tests Type: Enhancement Enhancement to existing functionality
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

CartoPy required for master Test import with basic install
3 participants
@dopplershift @jthielen @dcamron