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

Handling flaky tests #732

Merged
merged 2 commits into from
Aug 4, 2023
Merged

Handling flaky tests #732

merged 2 commits into from
Aug 4, 2023

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Aug 2, 2023

Closes #731 by marking tests as flaky. Through discussion with @khaeru, we agreed that this is the best option for now; should the tests make more issues in the future, we can still identify their root cause.

How to review

  • Read the diff and note that the CI checks all pass.

PR checklist

  • Continuous integration checks all ✅
  • Adapt CI tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation. Nothing new added.
  • [ ] Update release notes. Nothing public-facing added

@glatterf42 glatterf42 added enh New features & functionality ci Continuous integration labels Aug 2, 2023
@glatterf42 glatterf42 self-assigned this Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #732 (d65e36b) into main (b5e93b7) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #732   +/-   ##
=====================================
  Coverage   94.4%   94.5%           
=====================================
  Files         43      43           
  Lines       3434    3463   +29     
=====================================
+ Hits        3245    3273   +28     
- Misses       189     190    +1     
Files Changed Coverage Δ
...age_ix/tests/test_feature_bound_activity_shares.py 100.0% <100.0%> (ø)
message_ix/tests/test_feature_price_emission.py 100.0% <100.0%> (ø)
message_ix/tests/test_integration.py 100.0% <100.0%> (ø)
message_ix/tests/test_legacy_version.py 100.0% <100.0%> (ø)
message_ix/tests/test_macro.py 100.0% <100.0%> (ø)
message_ix/tests/test_tutorials.py 97.5% <100.0%> (-2.5%) ⬇️

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Looks effective, thanks.

Per the changes to the files, I think it would be better to do something like:

# Define a mark as a module global
FLAKY = pytest.mark.flaky(
    reruns=5
    rerun_delay=2,
    condition="GITHUB_ACTIONS" in os.environ and sys.platform == "darwin",
    reason="Flaky; See iiasa/message_ix#731"
)

…

# Use the mark to decorate multiple tests
@FLAKY
def test_add_bound_activity_up_all_modes(message_test_mp):
    …

Two reasons:

  • A general heuristic (not sure where or from which example I picked it up) for pytest usage is to organize tests (modules, files, classes, and functions) by functional meaning or in a way that mirrors the organization of the code being tested. For instance, we might have a class TestPlatform that collects tests of class Platform. This makes it easy to navigate.
    If these tests cease to be flaky (for instance, with a new ixmp4 backend) then the class grouping would no longer be meaningful and we'd have to move the tests out again.
  • It would create minimally-invasive, small diffs.
  • It would be clear when multiple tests are marked in the same meaning, without having to read an compare the function arguments.

@glatterf42
Copy link
Member Author

Thank you for the comment, I agree that your suggestion is more elegant. I will re-work this PR and the other ones.

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Good to go, thanks for the thorough and clean work.

@khaeru khaeru merged commit 0b1983f into iiasa:main Aug 4, 2023
19 checks passed
@glatterf42 glatterf42 deleted the enh/ci/flaky-tests branch August 4, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling flaky tests
2 participants