fix(test): set LimitSet on quota fixtures added in #2347 - #2408
fix(test): set LimitSet on quota fixtures added in #2347#2408veyron-kairo wants to merge 1 commit into
Conversation
Project-HAMi#2313 made FitQuota gate on LimitSet instead of Limit != 0, so an explicit ResourceQuota of 0 is honored as a hard block. The quota tests added in Project-HAMi#2347 build device.Quota values directly with only Limit set, so after Project-HAMi#2313 those limits read as not-configured and the over-quota cases were admitted instead of denied. AddQuota sets LimitSet in the real path; set it in the fixtures too. This is why master currently fails TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice and TestFitResourceQuotaAscendMemoryFactor: Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> Project-HAMi#2347 and Project-HAMi#2313 each passed CI on their own base and only conflict once combined.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: veyron-kairo 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 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:
|
|
Already fixed |
What type of PR is this?
/kind bug
What this PR does / why we need it:
master is red on the scheduler unit tests right now. #2313 changed FitQuota to gate on LimitSet instead of
Limit != 0, so an explicit ResourceQuota of "0" is honored as a hard block. The quota tests added in #2347 builddevice.Quotavalues directly with onlyLimitset, so once both landed on master those limits read as "not configured" and the over-quota cases got admitted instead of denied. AddQuota sets LimitSet in the real path, so this sets it in the fixtures to match.Both PRs passed CI on their own base and only conflict once they're on master together, which is why neither run caught it.
Tests this brings back to green:
Which issue(s) this PR fixes:
none filed, this is a master CI regression
Special notes for your reviewer:
test-only, no production code touched.
go test ./pkg/scheduler/...passes locally. #2313 was mine, so this is cleaning up after it.AI assistance disclosure:
I used AI assistance (Claude Code) to bisect which merge combination broke master and to confirm the fix. I read and ran the tests myself.
Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit