Skip to content
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 #103

Merged
merged 8 commits into from
Aug 5, 2024
Merged

Introduce merge_junit script #103

merged 8 commits into from
Aug 5, 2024

Conversation

wzieba
Copy link
Member

@wzieba wzieba commented Aug 2, 2024

To improve performance of Android unit tests collection. Originated it woocommerce/woocommerce-android#12064


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

wzieba added 3 commits August 2, 2024 13:19
To improve performance of Android unit tests collection. Originated it woocommerce/woocommerce-android#12064
Consider using { cmd1; cmd2; } >> file instead of individual redirects. https://www.shellcheck.net/wiki/SC2129
@wzieba wzieba requested a review from AliSoftware August 2, 2024 11:46
bin/merge_junit Outdated
Copy link
Contributor

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 😛

bin/merge_junit Outdated
Comment on lines 34 to 35
# (Note that in the case of Unit Tests, the JUnit XML files produced by Gradle
# don't have a parent `<testsuites>` root tag, so there's no need to try and remove it)
Copy link
Contributor

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 🤔

Copy link
Member Author

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.

Copy link
Contributor

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. maybe electron-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 this merge_junit_reports utility more resilient and less likely to create invalid XML files.

Copy link
Member Author

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? 1913f43

I'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.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Olivier Halligon <[email protected]>
@wzieba wzieba merged commit f7a0940 into trunk Aug 5, 2024
4 checks passed
@wzieba wzieba deleted the merge_junit_report branch August 5, 2024 15:19
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.

2 participants