New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
e2e: improve failure handling #113383
e2e: improve failure handling #113383
Conversation
@pohly: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
c724c39
to
6353736
Compare
/hold There is code in kubernetes/test/e2e/upgrades/upgrade_suite.go Lines 68 to 96 in 6e31c65
I don't think that code works in all cases (for example, when Ginkgo itself discovers a failure, which is possible now, or when when some test doesn't use the skipper helper package). We need to figure out what that upgrade test really needs to do and how to do it reliably. |
6353736
to
fc024a5
Compare
/hold cancel I removed that code from upgrade_suite.go because the result wasn't used anywhere. |
c53607f
to
c50c926
Compare
The original intention (adding more information for later analysis) is probably obsolete because there is no code which does anything with the extended error. The code in upgrade_suite.go collected it in an in-memory JUnit report, but then didn't do anything with that field. The code also wouldn't work for failures detected by Ginkgo itself, like the upcoming timeout handling. If the upgrade suite needs the information, it probably should get it from Gingko with a ReportAfterSuite call instead of depending in some fragile interception mechanism.
c50c926
to
80c554c
Compare
/retest |
/retest |
/assign @aojea Can you perhaps review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, spiffxp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The code was unnecessarily complex (intercepting panics without anyone using that).
Special notes for your reviewer:
Does this PR introduce a user-facing change?