fix(scheduler): set LimitSet in the remaining ResourceQuota test fixtures - #2432
Conversation
Project-HAMi#2313 added a LimitSet flag that FitQuota now gates on instead of Limit != 0, and updated TestFitResourceQuota's fixture accordingly, but missed three sibling tests in the same file that also build device.Quota directly: TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice, and TestFitResourceQuotaAscendMemoryFactor. Their fixtures defaulted to LimitSet: false, so FitQuota treated the configured limits as unset and admitted every pod, failing the denial assertions in all three tests on current master. Set LimitSet: true on the five affected fixture entries to match what AddQuota produces, mirroring the fix already applied to TestFitResourceQuota. Signed-off-by: Aditya Raut <araut7798@gmail.com>
|
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 test fixtures now set ChangesQuota fixture enforcement
Estimated code review effort: 1 (Trivial) | ~3 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:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, archlitchi 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 |
What this fixes
#2313 added a
LimitSetflag todevice.Quotaand switchedFitQuotato gate onLimitSetinstead ofLimit != 0, so an explicitly configured zero limit is honored as a hard block. That PR updatedTestFitResourceQuota's fixture to setLimitSet: trueto match, but three sibling tests in the same file also builddevice.Quotavalues directly and were missed:TestFitResourceQuotaNonNvidiaTestFitResourceQuotaCountsEveryDeviceTestFitResourceQuotaAscendMemoryFactorTheir fixtures default to
LimitSet: false, soFitQuotanow treats their configured limits as "not set" and admits every pod — the denial assertions in all three tests currently fail on master:I ran into this while rebasing an unrelated PR (#2174) onto current master —
make testfails on a clean master checkout with no other changes involved.Fix
Set
LimitSet: trueon the five affected fixture entries across the three tests, matching whatAddQuotaproduces and mirroring the fix already applied toTestFitResourceQuota.Testing
go test ./pkg/scheduler/... -run TestFitResourceQuota -race -count=1passes (all fourTestFitResourceQuota*tests).go test ./...scoped topkg/scheduler/...passes.AI disclosure
This PR was written primarily by Claude Code (Anthropic), used to investigate the codebase, identify the bug, implement the fix, and verify it. I reviewed and verified the change before submitting.
Summary by CodeRabbit