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

test[cartesian]: update hypothesis and fix the health check issues found in daily CI #1333

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

DropD
Copy link
Contributor

@DropD DropD commented Sep 6, 2023

Description

The daily CI found that after updating to hypothesis 6.82.1 some tests in cartesian are failing hypothesis health checks. This PR tries to fix that.

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

@DropD DropD requested a review from egparedes September 6, 2023 13:28
@DropD
Copy link
Contributor Author

DropD commented Sep 6, 2023

Seems that the new version caused the health check for multiple executors to trigger on parametrized tests. From the tests in hypothesis itself it looked like this might not happen if parametrization is the outer decorator (applied after @given).

@DropD DropD changed the title test:[cartesian]: update hypothesis and fix the health check issues found in daily CI test[cartesian]: update hypothesis and fix the health check issues found in daily CI Sep 6, 2023
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

We need to run the requirements update workflow before merging.

@@ -5,7 +5,7 @@
## for pkg in ['hypothesis', 'pytest']:
## print(re.search(f"\n({pkg} *[=>~!].*)\n", versions)[1])
##]]]
hypothesis>=6.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever any requirement is modified, the tox workflow updating all config files need to be executed as explained here: https://github.com/GridTools/gt4py/blob/main/docs/development/tools/requirements.md

Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

The mypy version in pre-commit-config needs to be updated manually because the hook is defined in the mirror repository and it's not always up-to-date.

This has been now fixed so the PR is ready to be merged IMHO.

It looks good to me

@egparedes egparedes force-pushed the fix-hypothesis-differing_executors branch from 2439c54 to 8c312cf Compare September 18, 2023 12:26
@egparedes egparedes merged commit 34516a6 into GridTools:main Sep 19, 2023
@DropD DropD deleted the fix-hypothesis-differing_executors branch October 4, 2023 08:07
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.

2 participants