-
Notifications
You must be signed in to change notification settings - Fork 258
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
Mark junit test cases as skipped if no pickle step results available #597
base: main
Are you sure you want to change the base?
Conversation
ef369b1
to
e319fd6
Compare
e319fd6
to
026541e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #597 +/- ##
==========================================
+ Coverage 83.21% 83.27% +0.06%
==========================================
Files 28 29 +1
Lines 3413 2858 -555
==========================================
- Hits 2840 2380 -460
+ Misses 458 357 -101
- Partials 115 121 +6 ☔ View full report in Codecov by Sentry. |
026541e
to
e82afb8
Compare
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.
Ant tests?
Sorry just noticed this comment - if this approach is acceptable @Johnlon (recovering panics instead of changing the return signatures of everything), I'm happy to add tests etc in order to get this mergeable. Will take a look shortly at what is needed. |
I'm on holiday so can't remind myself about this. |
Hi there - sorry I was myself away on holiday when you replied @Johnlon :) This PR was the smallest change to fix the issue - by recovering the panics that cause the issue and handling them that way. It works, but a more 'idiomatic go' solution would be to change the function signature that is panicking to return an error, and change all the places it is used to handle the error. That would be a much, much bigger change though - hence my question about whether this approach of just catching the panics was acceptable here. If there's no objection to this panic-recovering approach, it would be my preference to keep the change small and I'll add some tests to cover it as-is. |
🤔 What's changed?
This allows the junit reporter not to panic if the StopOnFirstFailure flag is used. It marks any subsequent test cases as 'skipped' (seemed like the best choice).
I kept the change as small as possible by recovering from the panics, instead of introducing a suite of "non-must' equivalents on the Storage struct. While that would have been far cleaner as code (recovering from panics feels decidedly icky), this solves the issue with relatively little impact to anything else. But if y'all think adding non-must equivalents onto the Storage struct is a better call, happy to make that change.
⚡️ What's your motivation?
Fixes #387
🏷️ What kind of change is this?
📋 Checklist: