Skip to content

Mark junit test cases as skipped if no pickle step results available #597

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mrsheepuk
Copy link
Contributor

🤔 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?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

📋 Checklist:

@mrsheepuk mrsheepuk force-pushed the fix-junit-stop-on-first-fail branch from ef369b1 to e319fd6 Compare February 21, 2024 10:08
@mrsheepuk mrsheepuk force-pushed the fix-junit-stop-on-first-fail branch from e319fd6 to 026541e Compare April 29, 2024 08:51
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.93%. Comparing base (153db4e) to head (f91598d).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
internal/formatters/fmt_junit.go 92.68% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #597      +/-   ##
==========================================
- Coverage   83.21%   77.93%   -5.28%     
==========================================
  Files          28       41      +13     
  Lines        3413     4074     +661     
==========================================
+ Hits         2840     3175     +335     
- Misses        458      780     +322     
- Partials      115      119       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mrsheepuk mrsheepuk force-pushed the fix-junit-stop-on-first-fail branch from 026541e to e82afb8 Compare April 29, 2024 08:53
Copy link
Member

@Johnlon Johnlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ant tests?

@mrsheepuk
Copy link
Contributor Author

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.

@Johnlon
Copy link
Member

Johnlon commented Aug 28, 2024

I'm on holiday so can't remind myself about this.
your comment,"instead of changing signatures" sounds like you might be trying to make a compromise? maybe? anyway I wouldn't bother with compromising anything. just do what ever is the simple and clean thing to improve the code base and functionality.

@mrsheepuk
Copy link
Contributor Author

mrsheepuk commented Sep 3, 2024

I'm on holiday so can't remind myself about this. your comment,"instead of changing signatures" sounds like you might be trying to make a compromise? maybe? anyway I wouldn't bother with compromising anything. just do what ever is the simple and clean thing to improve the code base and functionality.

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.

@mrsheepuk mrsheepuk force-pushed the fix-junit-stop-on-first-fail branch from e82afb8 to f91598d Compare March 26, 2025 10:58
@mrsheepuk
Copy link
Contributor Author

I've rebased this over the latest and actually added some tests for it - I don't know if you're still the right person to review @Johnlon but hopefully should be mergeable if the CI flow passes - take a look and let me know any concerns.

@mrsheepuk mrsheepuk requested a review from Johnlon March 26, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't get report on test failure and StopOnFailure=true
2 participants