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.
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
Introduce
merge_junit
script #103Introduce
merge_junit
script #103Changes from 5 commits
771b5f3
38b4010
36a662c
9cef1fa
496c3c1
a1c1dcc
1913f43
27e702c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nitpick: I'd recommend naming this script
merge_junit_reports
instead, to make it clearer that we're not trying to merge the Unit Tests themselves somehow (?), but just the report files 😛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.
While I was ok with this assumption being held for WooCommerce where you initially implemented this, now that this is going to become a broader utility for more repositories to use, I feel like we should be more careful in our implementation and assumptions here, especially to make sure we support cases and other projects where such assumption might not be always true 🤔
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.
What do you suggest? I think this is a standard behavior of JUnit working with Android Gradle Plugin - there's nothing specific in WooCommerce Android.
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.
My only fear is that someone would be tempted to use this
merge_junit_report
utility also on JUnit reports that don't come from Unit Tests created by Android Gradle Plugin, like JUnit reports created by fastlane, or by other wrappers (e.g. maybeelectron-builder
when building the Android app of an Electron codebase?), or for test reports that are also in the JUnit format but for tests other than Unit tests (e.g. UI Tests / E2E Tests?).If it happens that those other reports, despite being in the JUnit format, don't fit the assumption that this
merge_junit_reports
utility makes of them not being wrapped within<testsuites>
root node because they are generated by another tool than Android Gradle Plugin for Unit Tests, then this will break the merged file that it generates.By contrast, just adjusting the
sed
pattern to remove those<testsuites>
root elements if they happen to be present in the input file doesn't seem like something too complex to add (iirc I even suggested a pattern for it in the WCAndroid PR where you introduced that script initially) while making thismerge_junit_reports
utility more resilient and less likely to create invalid XML files.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.
Gotcha 👍 I've added a logic to ignore
<testsuites>
tag in reports that are being merged. WDYT? 1913f43I've tried to add a test for this with
bats
, but surprisingly it happened to be quite unstable even with a simple test - occasionally it was ending in a state, that was simply not finishing a test. After killing the process and running test again all worked fine. I didn't want to introduce flakiness though.