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

Fix everest fixture for caching testcase results #9928

Closed
wants to merge 1 commit into from

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Jan 31, 2025

Issue
Resolves #9670

Approach
pytest-snapshot does not go well with pytest-xdist, so we have to manually set the snapshot path to avoid it using the worker-ids in the path.

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

Copy link

codspeed-hq bot commented Jan 31, 2025

CodSpeed Performance Report

Merging #9928 will not alter performance

Comparing jonathan-eq:fix-flaky-test (28bd0c3) with main (3f4a93e)

Summary

✅ 25 untouched benchmarks

@jonathan-eq jonathan-eq force-pushed the fix-flaky-test branch 2 times, most recently from 6066a8c to 8c2b68a Compare January 31, 2025 11:45
@jonathan-eq jonathan-eq marked this pull request as ready for review January 31, 2025 11:49
@jonathan-eq jonathan-eq added the release-notes:flaky-test-fix PR fixes a flaky test label Jan 31, 2025
@jonathan-eq jonathan-eq requested a review from yngve-sk February 3, 2025 08:31
@xjules xjules requested a review from frode-aarstad February 4, 2025 11:25
@@ -74,6 +74,9 @@ def test_api_snapshots(config_file, snapshot, cached_example):
.strip()
+ "\n"
)
snapshot.snapshot_dir = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part necessary? The snapshots are currently already scoped/split without this?

Screenshot 2025-02-05 at 14 43 50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it was pytest-repeat that changed the snapshot names

cache = pytestconfig.cache

def run_config(test_data_case: str):
if cache.get(f"cached_example:{test_data_case}", None) is None:
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes the everest tests take 16m to run (as opposed to currently 6-7mins) as it now has to create one set of complete runs per runner, in this case running sequential might actually be faster/cheaper than parallel. So it would be ideal to be able to share this cache thing between runners and look for another way to avoid the collision. I don't think I understand the problem this solves in full detail, nor whether this is the right way to go about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is solved in ert is by using --dist loadgroup to put all workflows that require the cached testdata run in the same process.

The fixture for caching had a race condition, where multiple workers
tried setting the cache.
@jonathan-eq
Copy link
Contributor Author

Superseded by #10041

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:flaky-test-fix PR fixes a flaky test
Projects
None yet
3 participants