test: Configurable memory and disk size on test suites - #5269
Conversation
Allow both test suite to manage memory and disk-size properties without relay on default values, as bundles might force a change still not present in crc
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
WalkthroughThis PR threads ChangesE2E Test Disk Size Configuration
Integration Test Memory and Disk Size Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/testsuite/testsuite.go`:
- Around line 900-905: The cleanup path currently unsets memory/preset but not
disk-size, so add a cleanup call to revert disk-size to avoid config leakage
between scenarios; locate the `@cleanup` block and call
SetConfigPropertyToValueSucceedsOrFails with "disk-size" and an empty value
(e.g. SetConfigPropertyToValueSucceedsOrFails("disk-size", "", "succeeds")) so
the disk-size key is removed/restored during teardown.
- Around line 900-905: CheckCRCStatusJSONOutput currently compares the
JSON-reported disk size to constants.DefaultDiskSize even when
EnsureCRCIsRunning/SetConfigPropertyToValueSucceedsOrFails set a custom
CRCDiskSize; update CheckCRCStatusJSONOutput to honor CRCDiskSize (use
CRCDiskSize when non-empty, fallback to constants.DefaultDiskSize) when
validating the disk-size field from crc status -ojson, ensuring any unit/format
normalization matches how CRCDiskSize is provided.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18180cba-5dc6-4543-b452-f66e9b180819
📒 Files selected for processing (8)
images/build-e2e/lib/darwin/run.shimages/build-e2e/lib/linux/run.shimages/build-e2e/lib/windows/run.ps1images/build-integration/lib/darwin/run.shimages/build-integration/lib/linux/run.shimages/build-integration/lib/windows/run.ps1test/e2e/testsuite/testsuite.gotest/integration/testsuite_test.go
| if CRCDiskSize != "" { | ||
| err = SetConfigPropertyToValueSucceedsOrFails("disk-size", CRCDiskSize, "succeeds") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Unset disk-size in the @cleanup path too.
This now persists disk-size into CRC config, but the cleanup block only unsets memory, preset, and the older keys. After one scenario runs with a custom disk size, later scenarios can inherit that value from ~/.crc, which breaks suite isolation.
Suggested follow-up
err = crcCmd.UnsetConfigPropertySucceedsOrFails("memory", "succeeds") // unsetting property that is not set gives 0 exitcode, so this works
if err != nil {
fmt.Println(err)
os.Exit(1)
}
+
+ err = crcCmd.UnsetConfigPropertySucceedsOrFails("disk-size", "succeeds")
+ if err != nil {
+ fmt.Println(err)
+ os.Exit(1)
+ }
err = crcCmd.UnsetConfigPropertySucceedsOrFails("preset", "succeeds") // unsetting property that is not set gives 0 exitcode, so this works
if err != nil {
fmt.Println(err)
os.Exit(1)🤖 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/e2e/testsuite/testsuite.go` around lines 900 - 905, The cleanup path
currently unsets memory/preset but not disk-size, so add a cleanup call to
revert disk-size to avoid config leakage between scenarios; locate the `@cleanup`
block and call SetConfigPropertyToValueSucceedsOrFails with "disk-size" and an
empty value (e.g. SetConfigPropertyToValueSucceedsOrFails("disk-size", "",
"succeeds")) so the disk-size key is removed/restored during teardown.
Make crc status -ojson validation honor the configured disk size.
EnsureCRCIsRunning() can now start CRC with a non-default disk-size, but CheckCRCStatusJSONOutput() still compares against constants.DefaultDiskSize. Any suite that sets --crc-disk-size below the default will fail that validation even when CRC was configured correctly.
Suggested follow-up
crcDiskSize := crcStatusJSONOutputObj["diskSize"]
- if strongunits.GiB(cast.ToUint64(crcDiskSize)) < strongunits.GiB(constants.DefaultDiskSize-1) {
+ expectedDiskSize := constants.DefaultDiskSize
+ if CRCDiskSize != "" {
+ if parsed, err := strconv.Atoi(CRCDiskSize); err == nil {
+ expectedDiskSize = parsed
+ }
+ }
+ if strongunits.GiB(cast.ToUint64(crcDiskSize)) < strongunits.GiB(expectedDiskSize-1) {
// This is a workaround for the fact that crc status json output
// which doesn't return the actual disk size, but rather the
// size of `/sysroot` mount, which is less than the actual disk size.
- return fmt.Errorf("failure in asserting 'diskSize' field of crc status json output, expected greater than or equal to %d bytes, actual : %d bytes", strongunits.GiB(constants.DefaultDiskSize).ToBytes(), strongunits.B(cast.ToUint64(crcDiskSize)))
+ return fmt.Errorf("failure in asserting 'diskSize' field of crc status json output, expected greater than or equal to %d bytes, actual : %d bytes", strongunits.GiB(expectedDiskSize).ToBytes(), strongunits.B(cast.ToUint64(crcDiskSize)))
}🤖 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/e2e/testsuite/testsuite.go` around lines 900 - 905,
CheckCRCStatusJSONOutput currently compares the JSON-reported disk size to
constants.DefaultDiskSize even when
EnsureCRCIsRunning/SetConfigPropertyToValueSucceedsOrFails set a custom
CRCDiskSize; update CheckCRCStatusJSONOutput to honor CRCDiskSize (use
CRCDiskSize when non-empty, fallback to constants.DefaultDiskSize) when
validating the disk-size field from crc status -ojson, ensuring any unit/format
normalization matches how CRCDiskSize is provided.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Sure, need to rework this part
|
@albfan can we update it or this is no longer required? |
|
I think It will be good to have |
In that case let's update this PR as per #5269 (comment) review. |
2 participants
Allow both test suite to manage memory and disk-size properties without relay on default values, as bundles might force a change still not present in crc
Description
Configurable properties for both test suites
Type of change
test, version modification, documentation, etc.)
Proposed changes
CI can configure memory or disk-size
Testing
Use new defined properties
Contribution Checklist
Summary by CodeRabbit
Release Notes