[QE] add cpu and memory consume track to e2e test #4834
Conversation
WalkthroughAdded memory usage collection alongside existing CPU collection in the OpenShift end-to-end scenario; new unexported functions collect CPU and memory metrics and write them to result files. The file write utility was changed to append mode so metric outputs accumulate. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as E2E Runner
participant Suite as testsuite.go
participant Lib as gopsutil (cpu/mem)
participant File as WriteToFile
Runner->>Suite: execute @story_health scenario steps
Suite->>Lib: get CPU data (getCPUdata)
Lib-->>Suite: CPU metrics
Suite->>File: append CPU metrics to ../test-results/cpu-consume.txt
Suite->>Lib: get Memory data (getMemoryData)
Lib-->>Suite: Memory metrics
Suite->>File: append Memory metrics to ../test-results/memory-consume.txt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)test/e2e/testsuite/testsuite.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[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.
Needs 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/e2e/features/story_openshift.feature(2 hunks)test/e2e/testsuite/testsuite.go(4 hunks)test/extended/util/fileops.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/testsuite/testsuite.go (1)
test/extended/util/fileops.go (1)
WriteToFile(93-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (macOS-13, 1.23)
🔇 Additional comments (5)
test/e2e/features/story_openshift.feature (1)
15-15: Well-placed CPU data collection points.The CPU data collection steps are strategically positioned to capture metrics at key phases of the test scenario: after project creation, after deployment, and after cluster stop. This provides comprehensive coverage for performance monitoring.
Also applies to: 27-27, 32-32
test/extended/util/fileops.go (1)
94-94: Appropriate modification to support append mode.The change from
os.O_RDWRtoos.O_CREATE|os.O_APPEND|os.O_WRONLYis well-suited for the CPU data collection use case, allowing data to be accumulated in the results file without overwriting existing content.test/e2e/testsuite/testsuite.go (3)
32-32: Import addition looks good.The import of the gopsutil CPU package is appropriate for the CPU monitoring functionality.
266-268: Good integration with scenario tags.The conditional check for
@story_healthtag to trigger CPU data collection before scenario start is well-integrated with the existing tag-based setup logic.
576-577: Step definition registration is correct.The step definition follows the proper godog pattern and matches the feature file usage.
lilyLuLiu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/e2e/testsuite/testsuite.go (3)
267-274: Rename to getCPUData for consistency; keep logging non-fatalAlign with Go naming conventions and prior review. Update the call site to the corrected function name. Optionally switch to util.LogMessage for consistent test logging.
- if err := getCPUdata("Before start"); err != nil { + if err := getCPUData("Before start"); err != nil { fmt.Printf("Failed to collect CPU data: %v\n", err) }
582-586: Bind step to getCPUData (naming consistency with Go conventions)Keep step text the same; just bind to the properly cased function name.
-s.Step(`^get cpu data "([^"]*)"`, - getCPUdata) +s.Step(`^get cpu data "([^"]*)"`, + getCPUData) s.Step(`^get memory data "([^"]*)"`, getMemoryData)
1323-1335: Fix CPU data formatting and handle fs path errors (writes may be broken as-is)
- data formatting uses a []float64 with "%.2f", producing invalid output like %!f([]float64=...). Use cpuData[0].
- stop ignoring os.Getwd error; surface failure.
- ensure the output directory exists to avoid write failures.
-func getCPUdata(content string) error { - cpuData, err := cpu.Percent(0, false) +func getCPUData(content string) error { + cpuData, err := cpu.Percent(0, false) if err != nil { return fmt.Errorf("failed to get CPU data: %v", err) } if len(cpuData) == 0 { return fmt.Errorf("no CPU data available") } - data := fmt.Sprintf("%s: %.2f%%\n", content, cpuData) - wd, _ := os.Getwd() - file := filepath.Join(wd, "../test-results/cpu-consume.txt") - return util.WriteToFile(data, file) + data := fmt.Sprintf("%s: %.2f%%\n", content, cpuData[0]) + wd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get working directory: %v", err) + } + file := filepath.Join(wd, "../test-results/cpu-consume.txt") + if err := os.MkdirAll(filepath.Dir(file), 0o755); err != nil { + return fmt.Errorf("failed to create results directory: %v", err) + } + return util.WriteToFile(data, file) }Optional: for more stable readings, consider a small sampling window, for example cpu.Percent(500*time.Millisecond, false).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/e2e/features/story_openshift.feature(2 hunks)test/e2e/testsuite/testsuite.go(4 hunks)test/extended/util/fileops.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/util/fileops.go
- test/e2e/features/story_openshift.feature
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/testsuite/testsuite.go (1)
test/extended/util/fileops.go (1)
WriteToFile(93-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (1)
test/e2e/testsuite/testsuite.go (1)
31-32: LGTM: imports for host metrics are appropriate and in usecpu and mem from gopsutil v4 are correctly imported and used below.
| func getMemoryData(content string) error { | ||
| v, _ := mem.VirtualMemory() | ||
| data := fmt.Sprintf("%s, UsedPercent: %f%%, Free: %v Mib\n", content, v.UsedPercent, v.Free/1024/1024) | ||
| wd, _ := os.Getwd() | ||
| file := filepath.Join(wd, "../test-results/memory-consume.txt") | ||
| return util.WriteToFile(data, file) | ||
| } |
There was a problem hiding this comment.
Handle mem.VirtualMemory and os.Getwd errors; improve units/formatting
- Don’t ignore mem.VirtualMemory error; check for nil.
- Don’t ignore os.Getwd error.
- Use two decimals for UsedPercent; print Free in MiB with %d and correct “MiB” spelling.
- Ensure output directory exists.
-func getMemoryData(content string) error {
- v, _ := mem.VirtualMemory()
- data := fmt.Sprintf("%s, UsedPercent: %f%%, Free: %v Mib\n", content, v.UsedPercent, v.Free/1024/1024)
- wd, _ := os.Getwd()
- file := filepath.Join(wd, "../test-results/memory-consume.txt")
- return util.WriteToFile(data, file)
-}
+func getMemoryData(content string) error {
+ v, err := mem.VirtualMemory()
+ if err != nil {
+ return fmt.Errorf("failed to get memory data: %v", err)
+ }
+ if v == nil {
+ return fmt.Errorf("no memory data available")
+ }
+ data := fmt.Sprintf("%s, UsedPercent: %.2f%%, Free: %d MiB\n", content, v.UsedPercent, v.Free/1024/1024)
+ wd, err := os.Getwd()
+ if err != nil {
+ return fmt.Errorf("failed to get working directory: %v", err)
+ }
+ file := filepath.Join(wd, "../test-results/memory-consume.txt")
+ if err := os.MkdirAll(filepath.Dir(file), 0o755); err != nil {
+ return fmt.Errorf("failed to create results directory: %v", err)
+ }
+ return util.WriteToFile(data, file)
+}📝 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.
| func getMemoryData(content string) error { | |
| v, _ := mem.VirtualMemory() | |
| data := fmt.Sprintf("%s, UsedPercent: %f%%, Free: %v Mib\n", content, v.UsedPercent, v.Free/1024/1024) | |
| wd, _ := os.Getwd() | |
| file := filepath.Join(wd, "../test-results/memory-consume.txt") | |
| return util.WriteToFile(data, file) | |
| } | |
| func getMemoryData(content string) error { | |
| v, err := mem.VirtualMemory() | |
| if err != nil { | |
| return fmt.Errorf("failed to get memory data: %v", err) | |
| } | |
| if v == nil { | |
| return fmt.Errorf("no memory data available") | |
| } | |
| data := fmt.Sprintf("%s, UsedPercent: %.2f%%, Free: %d MiB\n", content, v.UsedPercent, v.Free/1024/1024) | |
| wd, err := os.Getwd() | |
| if err != nil { | |
| return fmt.Errorf("failed to get working directory: %v", err) | |
| } | |
| file := filepath.Join(wd, "../test-results/memory-consume.txt") | |
| if err := os.MkdirAll(filepath.Dir(file), 0o755); err != nil { | |
| return fmt.Errorf("failed to create results directory: %v", err) | |
| } | |
| return util.WriteToFile(data, file) | |
| } |
🤖 Prompt for AI Agents
In test/e2e/testsuite/testsuite.go around lines 1337 to 1343, getMemoryData
currently ignores errors from mem.VirtualMemory and os.Getwd, prints UsedPercent
with default precision and Free with incorrect units/spelling, and may write to
a non-existent directory; update the function to check and handle the error
returned by mem.VirtualMemory (returning it on failure), check and handle
os.Getwd error, format UsedPercent with two decimals (e.g., %.2f), convert Free
to MiB as an integer and print with %d and "MiB", ensure the target directory
(../test-results) exists before writing (create it with appropriate permissions
if missing), and propagate any file-write errors returned by util.WriteToFile.
There was a problem hiding this comment.
@lilyLuLiu ^^ looks like this is correct and we shouldn't ignore the errors, instead return those if any?
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.
@praveenkumar, I have added error handling for getMemory and getwd. For mkdir, I think it's not needed, as the folder test-result has already been created.
|
@lilyLuLiu: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
lilyLuLiu
moved this from Work In Progress
to Ready for review
in Project planning: crc
github-project-automation
bot
moved this from Ready for review
to Done
in Project planning: crc
Description
Add cpu consume track to e2e test case
Type of change
test, version modification, documentation, etc.)
Proposed changes
Testing
Contribution Checklist
Summary by CodeRabbit
New Features
Bug Fixes