fix(test): set LimitSet on the webhook quota fixtures to unbreak master - #2409
fix(test): set LimitSet on the webhook quota fixtures to unbreak master#2409Lakshya77089 wants to merge 1 commit into
Conversation
Project-HAMi#2313 gated FitQuota on Quota.LimitSet instead of Limit != 0, so a limit built directly as a struct literal rather than through AddQuota now reads as "no limit configured". That PR fixed the fixture in TestFitResourceQuota, but Project-HAMi#2347 had meanwhile added three more tests that build their fixtures the same way, and those were not updated. Both PRs were green on their own; the conflict only appears once they are both on master, so nothing caught it. TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice and TestFitResourceQuotaAscendMemoryFactor currently fail on master. Set LimitSet on the remaining fixtures so they match what AddQuota produces. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Lakshya77089 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe webhook quota test fixtures now set ChangesQuota fixture updates
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
fixed |
What type of PR is this?
/kind bug
/kind failing-test
What this PR does / why we need it:
masteris currently red. Three tests inpkg/scheduler/webhook_test.gofail:Reproduces on a clean checkout with
go test ./pkg/scheduler/ -run TestFitResourceQuota.#2313 added
Quota.LimitSetand changedFitQuotato gate on it rather thanLimit != 0, so that an explicit0is honoured as a hard block. AQuotabuilt directly as a struct literal, instead of going through
AddQuota, nowdefaults
LimitSetto false and reads as "no limit configured".#2313 spotted this and fixed the fixture in
TestFitResourceQuota. In themeantime #2347 had added three more tests that build their fixtures the same
way, and those were not updated. Each PR was green on its own; the conflict
only exists once both are on master, so neither CI run could have caught it.
This sets
LimitSet: trueon the remaining five fixtures so they match whatAddQuotaproduces. Test-only, no production code touched.Which issue(s) this PR fixes:
None filed — raising the fix directly since master is broken.
Special notes for your reviewer:
Worth considering separately: the same trap is waiting for the next person who
writes a quota test. A small constructor, or making
AddQuotathe only way toset a limit in tests, would stop it recurring. I have not done that here to keep
this to the minimum needed to get master green.
Does this PR introduce a user-facing change?:
AI assistance disclosure: this change was developed with Claude Code. Flagging
the extent up front per CONTRIBUTING.
Summary by CodeRabbit