Fix the system compressible test case to use drop-in files#31332
Fix the system compressible test case to use drop-in files#31332ngopalak-redhat wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe test changes how it obtains the CPU reservation value for system compressible validation. Instead of reading ChangesSystem Compressible Test — CPU Reservation Source
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@ngopalak-redhat: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/006a3fe0-6f86-11f1-8dce-6e93fa2429c9-0 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ngopalak-redhat 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/extended/node/system_compressible.go`:
- Around line 71-73: The `resource.MustParse(cpuQuantity)` call on line 71 will
panic if the configuration value is malformed, causing the test to abort without
a clear error message. Replace `resource.MustParse(cpuQuantity)` with
`resource.ParseQuantity(cpuQuantity)` and add explicit error handling to check
if parsing fails, then use `o.Expect()` or a similar test assertion to fail
gracefully with a descriptive error message explaining that the CPU quantity
could not be parsed.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d1b3519-963a-4320-9a09-a1e447c4af92
📒 Files selected for processing (1)
test/extended/node/system_compressible.go
| cpuResource := resource.MustParse(cpuQuantity) | ||
| systemReservedCPU := float64(cpuResource.MilliValue()) / 1000.0 | ||
| o.Expect(systemReservedCPU).To(o.BeNumerically(">", 0), "systemReserved.cpu should be greater than 0") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines in question
cat -n test/extended/node/system_compressible.go | sed -n '60,85p'Repository: openshift/origin
Length of output: 2035
🏁 Script executed:
# Let's also check a broader context to understand where cpuQuantity comes from
cat -n test/extended/node/system_compressible.go | sed -n '1,100p'Repository: openshift/origin
Length of output: 5316
🏁 Script executed:
# Let's search for other uses of MustParse in this test file to see if there's a pattern
rg "MustParse|ParseQuantity" test/extended/node/system_compressible.go -nRepository: openshift/origin
Length of output: 206
Avoid panic-prone parsing for node config values.
Line 71 uses resource.MustParse(cpuQuantity), which panics on malformed config and aborts the test. Even though this value comes from a system-controlled configuration file, defensive parsing ensures test stability and provides clear error messages. Use resource.ParseQuantity() with explicit error handling.
Suggested fix
- cpuResource := resource.MustParse(cpuQuantity)
+ cpuResource, err := resource.ParseQuantity(cpuQuantity)
+ o.Expect(err).NotTo(o.HaveOccurred(), "systemReserved.cpu must be a valid resource quantity")
systemReservedCPU := float64(cpuResource.MilliValue()) / 1000.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpuResource := resource.MustParse(cpuQuantity) | |
| systemReservedCPU := float64(cpuResource.MilliValue()) / 1000.0 | |
| o.Expect(systemReservedCPU).To(o.BeNumerically(">", 0), "systemReserved.cpu should be greater than 0") | |
| cpuResource, err := resource.ParseQuantity(cpuQuantity) | |
| o.Expect(err).NotTo(o.HaveOccurred(), "systemReserved.cpu must be a valid resource quantity") | |
| systemReservedCPU := float64(cpuResource.MilliValue()) / 1000.0 | |
| o.Expect(systemReservedCPU).To(o.BeNumerically(">", 0), "systemReserved.cpu should be greater than 0") |
🤖 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 `@test/extended/node/system_compressible.go` around lines 71 - 73, The
`resource.MustParse(cpuQuantity)` call on line 71 will panic if the
configuration value is malformed, causing the test to abort without a clear
error message. Replace `resource.MustParse(cpuQuantity)` with
`resource.ParseQuantity(cpuQuantity)` and add explicit error handling to check
if parsing fails, then use `o.Expect()` or a similar test assertion to fail
gracefully with a descriptive error message explaining that the CPU quantity
could not be parsed.
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@ngopalak-redhat: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/59b0d0c0-6fc0-11f1-89f1-1bdda8b83aee-0 |
1 participant
Summary by CodeRabbit