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.

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

Streamline fixtures, use pytest-xdist, drop Python3.8 #345

Merged
merged 26 commits into from
Oct 3, 2024
Merged

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Aug 26, 2024

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes issue #xyz
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)

What kind of change does this PR introduce?:

  • Sets a version for the xclim-testdata repo to prevent breaking changes there from causing issues here.
  • Creates two fixtures (open_dataset and get_file) that uses thread safe caching
  • Integrates pytest-xdist to distribute tests across multiple workers
  • Optimizes the testing data setup to reduce potential for data races (with the help of pooch).
  • Removes a lot of calls to fixtures that weren't needed.
  • Updates the numpy.random.RandomState call.
  • Employs caching of pip, tox, and testing data to significantly speed up subsequent builds.

Does this PR introduce a breaking change?:

Yes. Testing now requires the pytest-xdist plugin (has been added to pyproject.toml and environment.yml). By default, running pytest will only use one worker, but tox will use up to eight workers.

Other information:

https://pytest-xdist.readthedocs.io/en/stable/

@Zeitsperre Zeitsperre added enhancement New feature or request dependencies Pull requests that update a dependency file labels Aug 26, 2024
@Zeitsperre Zeitsperre self-assigned this Aug 26, 2024
@cehbrecht
Copy link
Collaborator

@Zeitsperre looks good 👍 ... but probably need some support in adapting PR #342.

@Zeitsperre
Copy link
Collaborator Author

@cehbrecht There's a ton of work to do. I'm completely tearing the testing apart. I might need some help to move mini-esgf-data to a new system that's easier to maintain.

@Zeitsperre Zeitsperre changed the title Streamline fixtures, use pytest-xdist Streamline fixtures, use pytest-xdist, drop Python3.8 Sep 4, 2024
@Zeitsperre
Copy link
Collaborator Author

There are a handful of tests failing that I can't seem to make sense of. It may be due to breaking changes in more recent versions of dask and xarray, but it isn't clear to me which changes are the culprit.

Within some of the internals of clisops (e.g. clisops.ops.base_operation.Operation) there are direct calls to roocs-utils's open_xr_dataset. Is it possible that these helper functions are broken?

One thing for certain is that this affects tests that open paths ending in wildcards (i.e. path/to/file/*.nc). I've tried to load() these on open, but it doesn't seem to have an effect. I'm totally stumped.

@coveralls
Copy link

coveralls commented Sep 5, 2024

Pull Request Test Coverage Report for Build 11163828659

Details

  • 137 of 181 (75.69%) changed or added relevant lines in 12 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 73.183%

Changes Missing Coverage Covered Lines Changed/Added Lines %
clisops/utils/testing.py 116 160 72.5%
Files with Coverage Reduction New Missed Lines %
clisops/ops/base_operation.py 3 87.5%
Totals Coverage Status
Change from base Build 10473239325: 0.6%
Covered Lines: 1853
Relevant Lines: 2532

💛 - Coveralls

@Zeitsperre
Copy link
Collaborator Author

@cehbrecht Waiting on your review here. Lots of changes, but things are much smoother here.

@cehbrecht
Copy link
Collaborator

@Zeitsperre thanks for the update. I'm fine with the changes. Moving to pooch looks good. Probably we will need some support for the other PR #342 after the merge of this one. Maybe have a release after the merge of this PR? PR #342 also partially replaces some functions of roocs-utils ... but there is more to do.

@Zeitsperre Zeitsperre merged commit 898c672 into master Oct 3, 2024
13 checks passed
@Zeitsperre Zeitsperre deleted the fix-tests branch October 3, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants