HYPERFLEET-625 - test: Verify Prow reports integration test panic failures #49
+51
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[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 |
rafabene
changed the title
rafabene
force-pushed
the
test/integration-failure-reporting
branch
from
76207ad to
87e9dff
Compare
|
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:
WalkthroughTest teardown and helpers were reordered: the Health server is now started during setup and stopped during teardown before the JWK mock teardown. pkg/db Testcontainer.Close now uses a 30-second timeout context for container termination and SQL close operations. TestMain introduces a 45-second watchdog goroutine that logs an error and force-exits with a non-zero code if teardown hangs. A new integration test, TestClusterPanicFailure, was added to intentionally panic for CI failure verification. Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
4 tasks
rafabene
force-pushed
the
test/integration-failure-reporting
branch
3 times, most recently
from
673cf24 to
2f60556
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
rafabene
force-pushed
the
test/integration-failure-reporting
branch
from
2f60556 to
80ddf99
Compare
![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
🤖 Fix all issues with AI agents
In `@test/integration/clusters_test.go`:
- Around line 782-788: The TestClusterPanicFailure test currently always panics;
change it to only panic when an opt‑in environment flag is set (e.g.,
ENABLE_PANIC_TEST) so it doesn't break CI by default: in
TestClusterPanicFailure, after calling test.RegisterIntegration(t) check the env
var (or use t.Skip unless enabled) and only call panic("intentional panic...")
when the flag is present; keep the rest of the test intact so the diagnostic
remains opt‑in.
In `@test/integration/integration_test.go`:
- Around line 61-70: The watchdog goroutine currently calls os.Exit(exitCode)
which may be zero if tests passed, masking a teardown timeout; update the
anonymous goroutine so it forces a non‑zero exit: if exitCode == 0 { exitCode =
1 } (or call os.Exit(1) directly) before invoking os.Exit, referring to the
existing variables and calls (the anonymous goroutine, logger.Error, and
os.Exit(exitCode)) so a teardown timeout always returns failure.
f4e22eb to
b314309
Compare
…t Prow hang When integration tests fail with a panic, the process continues to the teardown phase where container.Terminate() is called with no timeout. If the Docker container termination hangs, the process never exits, causing the Prow CI job to stay stuck in "pending" state indefinitely. Add a 30-second timeout context to Testcontainer.Close() so the teardown always completes, allowing the process to exit and Prow to report the test failure status back to GitHub.
rafabene
force-pushed
the
test/integration-failure-reporting
branch
from
b314309 to
b1f3fa6
Compare
rafabene
force-pushed
the
test/integration-failure-reporting
branch
from
b1f3fa6 to
d61a694
Compare
|
@rafabene: 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
TestClusterPanicFailure) that intentionally panics to verify whether Prow correctly reports the failure status back to GitHub.What this tests
The test calls
panic()after initializing the integration test helper. This simulates a crash during test execution to check if:gotestsumcaptures the panic and returns a non-zero exit codeCleanup
This test must be removed after confirming the Prow behavior.
Summary by CodeRabbit