Fix flaky ALSP manager tests#8606
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughThe ALSP manager tests disable periodic decay where exact penalties are expected, poll for asynchronous worker-pool state transitions, widen integration connectivity timing, and include decay accrued before disallow-list notification assertions. ChangesALSP manager test synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
network/alsp/manager/manager_test.go (2)
1459-1469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency:
DisallowListedcheck present inSingleHeartbeatphase-1 but missing inMultipleHeartbeatsphase-1.
TestDecayMisbehaviorPenalty_SingleHeartbeat(line 1459) checksrecord.DisallowListedin the phase-1 polling callback, butTestDecayMisbehaviorPenalty_MultipleHeartbeats(lines 1554-1560) does not. Both tests verify the same pre-decay state. Consider adding theDisallowListedcheck toMultipleHeartbeatsphase-1 for consistency, or document why it's intentionally omitted.Also applies to: 1554-1560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@network/alsp/manager/manager_test.go` around lines 1459 - 1469, The MultipleHeartbeats phase-1 validation lacks the pre-decay DisallowListed assertion present in TestDecayMisbehaviorPenalty_SingleHeartbeat. Update the phase-1 polling callback in TestDecayMisbehaviorPenalty_MultipleHeartbeats to reject records where record.DisallowListed is true, keeping its checks consistent with the single-heartbeat test.
192-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the default decay value to a shared variable.
The expression
model.SpamRecordFactory()(unittest.IdentifierFixture()).Decayis repeated 13+ times across these tests to retrieve the default decay value. Each call allocates a new identifier and spam record. Extract it to a package-level variable or a helper for clarity and to avoid drift.♻️ Suggested refactor
+// defaultDecay is the default decay value used by newly created spam records. +var defaultDecay = model.SpamRecordFactory()(unittest.IdentifierFixture()).DecayThen replace all occurrences:
- if record.Decay != model.SpamRecordFactory()(unittest.IdentifierFixture()).Decay { + if record.Decay != defaultDecay {Alternatively, if the value must be computed at test time, use a helper:
+func defaultDecayValue() float64 { + return model.SpamRecordFactory()(unittest.IdentifierFixture()).Decay + }Also applies to: 783-785, 911-913, 989-991, 1057-1059, 1138-1140, 1229-1231, 1311-1313, 1391-1393, 1467-1469, 1558-1560, 1649-1651, 1834-1836
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@network/alsp/manager/manager_test.go` around lines 192 - 194, Extract the repeated default decay expression used in the tests into a package-level shared variable or helper, such as near the test fixtures. Replace every occurrence in the affected comparisons, including those in the relevant test sections, with the shared value so the identifier and spam record are created only once and all tests use the same default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@network/alsp/manager/manager_test.go`:
- Around line 1459-1469: The MultipleHeartbeats phase-1 validation lacks the
pre-decay DisallowListed assertion present in
TestDecayMisbehaviorPenalty_SingleHeartbeat. Update the phase-1 polling callback
in TestDecayMisbehaviorPenalty_MultipleHeartbeats to reject records where
record.DisallowListed is true, keeping its checks consistent with the
single-heartbeat test.
- Around line 192-194: Extract the repeated default decay expression used in the
tests into a package-level shared variable or helper, such as near the test
fixtures. Replace every occurrence in the affected comparisons, including those
in the relevant test sections, with the shared value so the identifier and spam
record are created only once and all tests use the same default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c2b37e4-975a-42fd-a780-0b946b346c17
📒 Files selected for processing (1)
network/alsp/manager/manager_test.go
This comment has been minimized.
This comment has been minimized.
3 participants
Fixes flakiness in
network/alsp/managertests.My knowledge of this part of the code is a bit cursory, I tried retain the logic of the tests, but please check thoroughly if that is actually the case.
7fc8850 — Fix flaky ALSP manager tests
require.Eventuallycallbacks withreturn false, so the poll retries instead of aborting while reports are still being processed asynchronously.Test failure sample:
a75280b — Fix flaky slashing violations consumer integration test
TestHandleReportedMisbehavior_And_SlashingViolationsConsumer_Integrationis load-sensitive (reproduced 4/12 failures under CPU contention; 0/96 after the fix):Test failure sample:
Summary by CodeRabbit