tests: stabilize affinity handler suite#10983
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates affinity handler tests to poll for eventually consistent affinity-group responses and adds a helper that runs cleanup only on already-started test clusters. ChangesAffinity test reliability improvements
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10983 +/- ##
==========================================
- Coverage 79.26% 79.14% -0.13%
==========================================
Files 541 541
Lines 75674 75677 +3
==========================================
- Hits 59985 59893 -92
- Misses 11467 11542 +75
- Partials 4222 4242 +20
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test pull-unit-test-next-gen-3 |
| }) | ||
| // Exercise the full affinity handler matrix through the local scheduling | ||
| // path. TestAffinityForwardedHeader covers the microservice forwarding path. | ||
| suite.env.Env = tests.NonMicroserviceEnv |
There was a problem hiding this comment.
By forcing the affinity handler matrix to run only in the non-microservice env, the microservice-path coverage now mostly depends on TestAffinityForwardedHeader. However, that test only checks status 200 and X-Forwarded-To-Microservice; it does not verify that the forwarded GET response actually contains the affinity group created by the test.
I verified this with a local mutation: I temporarily changed the scheduling microservice affinity list endpoint to always return an empty affinity_groups map, and the current test still passed. After adding a response-body assertion, the test failed as expected. So this creates a real coverage gap. Please enhance TestAffinityForwardedHeader to decode the response body and use Eventually to assert that header-check is visible in the returned AffinityGroupsResponse.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/server/apiv2/handlers/affinity_test.go`:
- Around line 1308-1319: The polling block in the affinity test is using a
require-based check option through ReadGetJSON, which can stop Eventually
retries on transient failures. Update the Eventually callback around
handlers.AffinityGroupsResponse to use only non-fatal checks inside the loop
when calling ReadGetJSON, then keep the final assertion on
result.AffinityGroups["header-check"] after the loop so retries can continue
until the condition is met.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1574028-8bbf-4895-82a8-f5bded67b7f0
📒 Files selected for processing (1)
tests/server/apiv2/handlers/affinity_test.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, lhy1024 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: Fixes #10385, Fixes #10565, Fixes #10857
Stabilizes
TestAffinityHandlerTestSuiteby covering the currently open flaky-test issues from the affinity handler suite:TestAffinityListWithIDscould immediately list only a subset of newly created groups.TestAffinityListWithEmptyIDcould immediately list no matching group after creation.TearDownTestcould lazily touch an environment that was not already started and panic through a nil test server.What is changed and how does it work?
TestAffinityForwardedHeaderas the targeted microservice forwarding coverage.SchedulingTestEnvironmenthelper that only runs cleanup over already-started environments.Check List
Tests
Release note
Summary by CodeRabbit
Bug Fixes
Tests
Chores