Fix a flaky test for issue 19915 #20253
Conversation
…own Version constants which are guaranteed to be valid/masked. Signed-off-by: Joe Liu <guoqing4@illinois.edu>
github-actions
bot
added
>test-failure
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/test/java/org/opensearch/env/NodeMetadataTests.java (1)
54-55: Consider extracting the magic number as a named constant.The value 234217727 appears both here and in
tooNewVersion()(line 121). Extracting it as a named constant (e.g.,MAX_TEST_VERSION_ID) would improve maintainability and make the intent clearer.Example:
+ private static final int MAX_TEST_VERSION_ID = 234217727; + private Version randomVersion() { // VersionUtils.randomVersion() only returns known versions, which are necessarily no later than Version.CURRENT; however we want // also to consider our behaviour with all versions, so occasionally pick up a truly random version. // // Version.fromId(...) requires the OpenSearch mask to be present, so we must generate ids in the valid masked range. - // The maximum id used elsewhere in this test class is 234217727 (see tooNewVersion()). - return rarely() ? Version.fromId(between(MASK, 234217727)) : VersionUtils.randomVersion(random()); + // The maximum id used elsewhere in this test class is MAX_TEST_VERSION_ID (see tooNewVersion()). + return rarely() ? Version.fromId(between(MASK, MAX_TEST_VERSION_ID)) : VersionUtils.randomVersion(random()); }Then update line 121 to use the constant as well:
public static Version tooNewVersion() { - return Version.fromId(between(Version.CURRENT.id + 1, 234217727)); + return Version.fromId(between(Version.CURRENT.id + 1, MAX_TEST_VERSION_ID)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/org/opensearch/env/NodeMetadataTests.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/org/opensearch/env/NodeMetadataTests.java (1)
test/framework/src/main/java/org/opensearch/test/VersionUtils.java (1)
VersionUtils(52-371)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
server/src/test/java/org/opensearch/env/NodeMetadataTests.java (1)
52-55: LGTM! Fix correctly addresses the flaky test issue.The change ensures that randomly generated version IDs always include the required OpenSearch mask (MASK) and stay within the valid range, preventing
Version.fromId(...)from failing. The upper bound 234217727 is consistent withtooNewVersion()(line 121), and the added comments clearly explain the rationale.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20253 +/- ##
============================================
+ Coverage 73.20% 73.23% +0.02%
- Complexity 71745 71751 +6
============================================
Files 5795 5795
Lines 328304 328304
Branches 47283 47283
============================================
+ Hits 240334 240432 +98
+ Misses 68663 68542 -121
- Partials 19307 19330 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
Fix 2 tests where the failure is coming from NodeMetadataTests.randomVersion() generating an invalid raw version id and then calling Version.fromId(...). Version.fromId(...) now enforces that ids carry the OpenSearch mask, so random integers will intermittently blow up.
Both
org.opensearch.env.NodeMetadataTests.testUpgradesLegitimateVersionsandorg.opensearch.env.NodeMetadataTests.testEqualsHashcodeSerializationshould be fixed by this changeRelated Issues
Resolves #19915
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.