NETOBSERV-2768: Implement DUMP_EVENTS_ON_FAILURE env var#2799
Conversation
|
@Amoghrd: This pull request references NETOBSERV-2768 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTest initialization now reads DUMP_EVENTS_ON_FAILURE to control dump-on-failure. After-suite reporting collects g.SpecReport entries into passed/failed/skipped slices and prints detailed FAILED/PASSED/SKIPPED sections. The upgrade test avoids mutating global catalog source objects and removes an OCP version skip guard. ChangesTest logging, reporting, and upgrade flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
/hold |
![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
🧹 Nitpick comments (1)
integration-tests/backend/backend_suite_test.go (1)
31-35: Improve parsing ofDUMP_EVENTS_ON_FAILUREinintegration-tests/backend/backend_suite_test.go.
os.Getenv("DUMP_EVENTS_ON_FAILURE") == "true"is brittle (case/1/etc. won’t work); usestrconv.ParseBooland treat invalid values asfalse.Suggested patch
import ( "flag" "fmt" "os" + "strconv" "testing" @@ - dumpEvents := false - if os.Getenv("DUMP_EVENTS_ON_FAILURE") == "true" { - dumpEvents = true - } - exutil.TestContext.DumpLogsOnFailure = dumpEvents + dumpEvents, err := strconv.ParseBool(os.Getenv("DUMP_EVENTS_ON_FAILURE")) + if err != nil { + dumpEvents = false + } + exutil.TestContext.DumpLogsOnFailure = dumpEvents🤖 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 `@integration-tests/backend/backend_suite_test.go` around lines 31 - 35, Replace the brittle equality check against "true" with strconv.ParseBool on the DUMP_EVENTS_ON_FAILURE env var: call strconv.ParseBool(os.Getenv("DUMP_EVENTS_ON_FAILURE")), treat any parse error as false, assign the resulting boolean to dumpEvents (or set exutil.TestContext.DumpLogsOnFailure directly), and ensure exutil.TestContext.DumpLogsOnFailure is set to that parsed value so case/ "1"/"t" are handled correctly.
🤖 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 `@integration-tests/backend/backend_suite_test.go`:
- Around line 150-154: The summary prints "totalEvaluated/totalEvaluated" which
always shows 100%; update both fmt.Printf calls to use the actual passed count
(len(passedSpecs)) as the numerator instead of totalEvaluated so the headline
reflects real pass/fail ratio; reference the existing symbols totalEvaluated,
passedSpecs and report.RunTime.Seconds() when making this change.
---
Nitpick comments:
In `@integration-tests/backend/backend_suite_test.go`:
- Around line 31-35: Replace the brittle equality check against "true" with
strconv.ParseBool on the DUMP_EVENTS_ON_FAILURE env var: call
strconv.ParseBool(os.Getenv("DUMP_EVENTS_ON_FAILURE")), treat any parse error as
false, assign the resulting boolean to dumpEvents (or set
exutil.TestContext.DumpLogsOnFailure directly), and ensure
exutil.TestContext.DumpLogsOnFailure is set to that parsed value so case/
"1"/"t" are handled correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 622346ba-74f8-45bd-b12c-4b7bc4dfad48
📒 Files selected for processing (1)
integration-tests/backend/backend_suite_test.go
Amoghrd
force-pushed
the
NETOBSERV-2768
branch
3 times, most recently
from
22fee16 to
366bcfc
Compare
|
/unhold |
|
@kapiljain1989: changing LGTM is restricted to collaborators DetailsIn response to this:
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. |
|
/lgtm |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2799 +/- ##
============================
============================
🚀 New features to boost your workflow:
|
|
New changes are detected. LGTM label has been removed. |
|
|
||
| // Set up provider config after parsing flags | ||
| e2eframework.AfterReadingAllFlags(exutil.TestContext) | ||
| e2eframework.AfterReadingAllFlags(&e2eframework.TestContext) |
There was a problem hiding this comment.
@oliver-smakal - do you see any side effect if we didn't use exutil.TestContext?
|
@Amoghrd: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
Description
Dependencies
n/a
Test Results
Ran 2 tests
Passed: 1, Failed: 1, Skipped: 1
FAILED TESTS (1):
/Users/amoghrd/repos/netobserv/netobserv-operator/integration-tests/backend/test_flowcollector.go:853 [646.025 seconds]
Error: Expected
: 1.12.0
not to equal
: 1.12.0
PASSED TESTS (1):
SKIPPED TESTS (1):
Reason: Skipping because the TechPreviewNoUpgrade is not enabled on the cluster.
Checklist
Summary by CodeRabbit