-
Notifications
You must be signed in to change notification settings - Fork 2.7k
prow.k8s.io: add skip reason to Spyglass JUnit lens #25221
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
prow.k8s.io: add skip reason to Spyglass JUnit lens #25221
Conversation
@@ -156,7 +156,7 @@ | |||
{{range .Skipped}} | |||
{{$firstTest := index .Junit 0}} | |||
<tr> | |||
<td class="mdl-data-table__cell--non-numeric test-name">{{$firstTest.ClassName}}: {{$firstTest.Name}}</td> | |||
<td class="mdl-data-table__cell--non-numeric test-name">{{$firstTest.ClassName}}: {{$firstTest.Name}}<br/>{{$firstTest.Skipped}}</td> |
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.
Thank you! I wonder if it would be useful to include an explicit Reason:
prefix? Up to you.
<td class="mdl-data-table__cell--non-numeric test-name">{{$firstTest.ClassName}}: {{$firstTest.Name}}<br/>{{$firstTest.Skipped}}</td> | |
<td class="mdl-data-table__cell--non-numeric test-name">{{$firstTest.ClassName}}: {{$firstTest.Name}}<br/>Reason: {{$firstTest.Skipped}}</td> |
/hold
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.
@petr-muller, thank you for your feedback. I've revised the PR to include the suggestion for an explicit Reason:
prefix.
Adding context to the new changes:
- There are cases (mainly) where skipped tests do not pass any skipped reason string to the
e2eskipper.Skipf
method. So I added an if/else conditional check in the go template to handle the scenarios of an empty skipped reason (which required the addition of a helper functionSkippedReason()
in thelens.go
file) - I created the new function because:
- directly using
{{ if eq $firstTest.Skipped "" }}
in the go template didn't work, throwing incompatible string & pointer type comparisons. Here's a similar example. - Or in case of using
{{if $firstTest.Skipped}}
, it is neverFalse
(since it is always pointing to an empty skipped reason, therefore the conditional didn't move).
- directly using
The updated result is as follows:
8dc5157
to
685dc7e
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjwagner, petr-muller, Priyankasaggu11929 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 |
@cjwagner @petr-muller, thanks so much for the review! The PR above has a '/hold' on it, which is stopping it from merging. I need help removing that. Thank you! 🙂 |
/hold cancel @Priyankasaggu11929 you can use commands like the one above too ;) |
/retest |
What does this PR do:
At present, when an e2e test is skipped, the reason for the skip (if passed to the
e2eskipper.Skipf
function) is reflected in the artifacts/junit-*.xml files, but not on the Prow Spyglass JUnit lens:For example, in case of the following job:
This PR adds the skipped reason to the JUnit spyglass lens's
template.html
file (wherever passed).The outcome is as follows (updated):
Addresses Issue:
Fixes #24209