Merge junit reports into one file #12064

wzieba · 2024-07-18T13:22:14Z

To speed up test collection time

Description

Problem to address

Collecting JUnit results for unit tests takes significant part of the unit tests job (3m for Buildkite annotations, 4m for Buildkite test collector, internal source: paaHJt-6MN-p2 )

Possible solution

I've noticed that the problem is visible in unit tests, which produces a separate .xml report file per tests-suite, rather than in instrumented tests, where there's a single .xml report.

Why unit tests produces multiple `.xml` reports and instrumented tests produce single?

It seems it's the way AGP is organizing running instrumented tests. According to my research, there's no way to communicate to JUnit to produce a single report file, hence a simple merge logic proposed in this PR.

I think it's better to use this custom, small piece of logic than e.g. intrudcing a 3rd party npm package like https://www.npmjs.com/package/junit-report-merger

Testing information

Go to a trunk commit that this PR is based on: 2a8a3d1
Check build results, unit-tests job
Tap on View in test analytics
Check the Test execution count (it's 3744)
Verify this is the same value of collected test results as in this job from this PR.

Future actions

I'd like to merge this PR as it is and see how impactful this change is. After asserting the impact and lack of problems, I'd like to move merge_junit.sh script to our CI toolkit, to make it available for both WC and WP/JP Android projects.

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

To speed up test collection time

wpmobilebot · 2024-07-18T13:29:43Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`ece3c52`
	Direct Download	`woocommerce-wear-prototype-build-pr12064-ece3c52.apk`

wpmobilebot · 2024-07-18T14:03:37Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`ece3c52`
	Direct Download	`woocommerce-prototype-build-pr12064-ece3c52.apk`

It's easier for sharing across projects

wzieba · 2024-07-22T13:39:43Z

merge_junit.sh

@@ -0,0 +1,41 @@
+#!/bin/bash


@AliSoftware 👋 can I have your review on this shell script? 🙂

.buildkite/commands/run-unit-tests.sh

AliSoftware · 2024-07-22T14:13:45Z

merge_junit.sh

+# Iterate over the XML files in the directory, merging their content
+for report in "$reports_dir"/*.xml; do
+    # Strip the outer <testsuites> tags and append the content to the merged file
+    sed 's/^<.*testsuites>//; s/<.*\/testsuites>$//' "$report" >> "$output_file"
+done


Why the .* in front of the testsuites tag name?

This doesn't account for the <testsuites …> tag oftentimes having attributes, e.g. <testsuites name='WordPressTest.xctest' tests='1885' failures='2'> (granted, this example was taken from a .junit file generated for an iOS project, while the JUnit file generated by Gradle and Android tools might (?) not add attributes to the <testsuites> tag, but better cover all our bases)

This also doesn't remove the <?xml version="1.0" …> from the input files

I'd suggest instead to use the following sed command, which prints only lines that are between 2 patterns, excluding said patterns:

sed -n '/^\<testsuites.*\>/,/\<\/testsuites\>$/{//!p;}' "$report" >> "$output_file"

Actually, you can also pass a list of input files to sed directly instead of doing a for loop, so I think something like this would work too:

Suggested change

# Iterate over the XML files in the directory, merging their content

for report in "$reports_dir"/*.xml; do

# Strip the outer <testsuites> tags and append the content to the merged file

sed 's/^<.*testsuites>//; s/<.*\/testsuites>$//' "$report" >> "$output_file"

done

# Strip the outer <testsuites> tags of each input file, and append the content to the merged file

sed -n '/^\<testsuites.*\>/,/\<\/testsuites\>$/{//!p;}' "$reports_dir"/*.xml >> "$output_file"

Tested with the following dummy files:

tests1.junit

<?xml version='1.0' encoding='UTF-8'?> <testsuites name='WordPressTest.xctest' tests='1885' failures='2'> <testsuite name='WordPressFluxTests.WordPressFluxTests' tests='6' failures='0'> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testQueryStore' time='0.011'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreEmitsChanges' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreWithoutTransaction' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreWithTransaction' time='0.003'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStoreEmitsChanges' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStoreReceivesActions' time='0.001'/> </testsuite> </testsuites>

tests2.junit

<?xml version='1.0' encoding='UTF-8'?> <testsuites name='WordPressTest.xctest' tests='1885' failures='2'> <testsuite name='WordPressTest.AbstractPostFixLocalMediaURLsTests' tests='4' failures='0'> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateAllLocalMediaPathsButDoesNotChangeRemotePaths' time='0.075'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateLocalMediaPathsInCachesDirectory' time='0.062'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateLocalMediaPathsInDocumentDirectory' time='0.059'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateMultipleLocalMediaPaths' time='0.060'/> </testsuite> <testsuite name='WordPressTest.AbstractPostTest' tests='2' failures='0'> <testcase classname='WordPressTest.AbstractPostTest' name='testFeaturedImageURLForDisplay' time='0.030'/> <testcase classname='WordPressTest.AbstractPostTest' name='testTitleForStatus' time='0.002'/> </testsuite> </testsuites>

tests3.junit

<?xml version='1.0' encoding='UTF-8'?> <testsuites name='WordPressTest.xctest' tests='1885' failures='2'> <testsuite name='WordPressTest.ZendeskUtilsPlans' tests='7' failures='0'> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testAddOnPlanSelected' time='0.031'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testBloggerPlanSelected' time='0.022'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testBusinessPlanSelected' time='0.027'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testEcommercePlanSelected' time='0.034'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testFreePlanSelected' time='0.021'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testPremiumPlanSelected' time='0.040'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testPresonalPlanSelected' time='0.018'/> </testsuite> </testsuites>

sed command output

$ sed -n '/^\<testsuites.*\>/,/\<\/testsuites\>$/{//!p;}' tests*.junit

<testsuite name='WordPressFluxTests.WordPressFluxTests' tests='6' failures='0'> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testQueryStore' time='0.011'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreEmitsChanges' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreWithoutTransaction' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStatefulStoreWithTransaction' time='0.003'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStoreEmitsChanges' time='0.002'/> <testcase classname='WordPressFluxTests.WordPressFluxTests' name='testStoreReceivesActions' time='0.001'/> </testsuite> <testsuite name='WordPressTest.AbstractPostFixLocalMediaURLsTests' tests='4' failures='0'> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateAllLocalMediaPathsButDoesNotChangeRemotePaths' time='0.075'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateLocalMediaPathsInCachesDirectory' time='0.062'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateLocalMediaPathsInDocumentDirectory' time='0.059'/> <testcase classname='WordPressTest.AbstractPostFixLocalMediaURLsTests' name='testUpdateMultipleLocalMediaPaths' time='0.060'/> </testsuite> <testsuite name='WordPressTest.AbstractPostTest' tests='2' failures='0'> <testcase classname='WordPressTest.AbstractPostTest' name='testFeaturedImageURLForDisplay' time='0.030'/> <testcase classname='WordPressTest.AbstractPostTest' name='testTitleForStatus' time='0.002'/> </testsuite> <testsuite name='WordPressTest.ZendeskUtilsPlans' tests='7' failures='0'> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testAddOnPlanSelected' time='0.031'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testBloggerPlanSelected' time='0.022'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testBusinessPlanSelected' time='0.027'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testEcommercePlanSelected' time='0.034'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testFreePlanSelected' time='0.021'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testPremiumPlanSelected' time='0.040'/> <testcase classname='WordPressTest.ZendeskUtilsPlans' name='testPresonalPlanSelected' time='0.018'/> </testsuite>

Thanks @AliSoftware !

This doesn't account for the <testsuites …> tag oftentimes having attributes, e.g. (granted, this example was taken from a .junit file generated for an iOS project, while the JUnit file generated by Gradle and Android tools might (?) not add attributes to the tag, but better cover all our bases)

For unit tests, there was no <testsuites> tag before, it's artificially created only in this PR. I'm not sure if we should add attributes to this tag, as we'd have to compute their values, and this is getting complex (and probably not needed, as it wasn't here before).

This also doesn't remove the <?xml version="1.0" …> from the input files

Good point, thanks!

I'd suggest instead to use the following sed command, which prints only lines that are between 2 patterns, excluding said patterns:

This, unfortunately, won't work as there are no testsuites tag in generated artifacts. I'm trying to figure out why the end result in merged-test-results.xml is as expected (beside additional <?xml(...) tags> 😬 .

Example singular report file for AddressTest test suite.

Locally in WCAndroid you can run sh .buildkite/commands/run-unit-tests.sh and check WooCommerce/build/test-results/merged-test-results.xml file.

So, after investigating more, I think we can simplify logic to this: 1b5891a WDYT?

For unit tests, there was no tag before, it's artificially created only in this PR. I'm not sure if we should add attributes to this tag, as we'd have to compute their values, and this is getting complex (and probably not needed, as it wasn't here before).

Sorry if I was unclear, what I meant is that your regex wouldn't match <testsuites> tag that might have attributes in the input files. Because the RegEx /^<.*testsuites>/ would not match the cases of <testsuites attribute="value">. So that would fail to remove them from the input file

I was NOT suggesting that we'd try to report the <testsuites> attributes of the input files in the output files. I don't think that's worth it nor that it would be easy nor useful anyway. So I'm totally ok with echo '<testsuites>' >>"$output_file" on line 29 🙂

This, unfortunately, won't work as there are no testsuites tag in generated artifacts.

This is very suspicious, as according the the specs I found of the JUnit file format, I'd expect <testsuites> to be the root tag of any JUnit file 🤔 But I guess as the README of this junitxml project says, there seems to be no official spec for the JUnit file format, only a common(-ish) format convention… so maybe some tools wrap the test suites within <testsuites> while some other tools (like Gradle?) don't?

so maybe some tools wrap the test suites within while some other tools (like Gradle?) don't?

Yes, it looks like this. I want to highlight only that lack of <testsuites> tag is present only for unit tests. The instrumented tests are reported in a single file with <testsuites> tag 🤷‍♂️ .

So, after investigating more, I think we can simplify logic to this: 1b5891a WDYT?

If we are sure / guaranteed that, in the context of the JUnit files generated by Android Unit Tests tooling:

The <testsuite> tags of the input files will never be wrapped in a parent <testsuites> tag (like I've seen in other JUnit files generated by other projects)

The XML processor directive will always look like <?xml version="1.0" encoding="UTF-8"?> (and never e.g. <?xml version="1.0" encoding="utf-8"?> or <?xml version="1.0"?> or <?xml version="1.0" encoding="UTF-8" standalone="yes" ?>

Then yes, this commit could work (and could even be simplified by avoiding the for loop and passing the list of input files directly to sed as I suggested above)

Sure, we can loosen this constraint! Added all improvements 👍

Remove only <?xml header from the single-test report

Co-authored-by: Olivier Halligon <[email protected]>

merge_junit.sh

So we don't have to iterate with `for` loop

AliSoftware

To assert Buildkite annotation logic works as expected

This reverts commit 46c216c.

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

* Introduce `merge_junit` script To improve performance of Android unit tests collection. Originated it woocommerce/woocommerce-android#12064 * Add changelog entry * Address SC2129 Consider using { cmd1; cmd2; } >> file instead of individual redirects. https://www.shellcheck.net/wiki/SC2129 * Remove `sh` suffix To align to other commands * Create output file before saving data there * Rename script to `merge_junit_reports` * Remove `<testsuites>` tag if it's present in any report that is about to be merged * Update CHANGELOG.md Co-authored-by: Olivier Halligon <[email protected]> --------- Co-authored-by: Olivier Halligon <[email protected]>

Merge junit reports into one file

fcac645

To speed up test collection time

wzieba added 2 commits July 18, 2024 15:29

Collect only merged file

d711121

Copy only merged file for Buildkite test collector

53488a6

wzieba added 4 commits July 18, 2024 16:48

Fix incorrect merged tests path for Report Tests Status

cdb7684

Migrate from Gradle task to shell script

b1d58a7

It's easier for sharing across projects

Update chmod for merge_junit.sh

8582900

Merge branch 'trunk' into merge_junit_report

4700cf8

wzieba requested review from AliSoftware and ParaskP7 July 22, 2024 13:38

wzieba commented Jul 22, 2024

View reviewed changes

merge_junit.sh

@@ -0,0 +1,41 @@

#!/bin/bash

Copy link

Contributor Author

wzieba Jul 22, 2024

Choose a reason for hiding this comment

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

@AliSoftware 👋 can I have your review on this shell script? 🙂

wzieba changed the title ~~[WIP] Merge junit reports into one file~~ Merge junit reports into one file Jul 22, 2024

wzieba added type: enhancement A request for an enhancement. category: unit tests Related to unit testing. labels Jul 22, 2024

wzieba added this to the 19.7 milestone Jul 22, 2024

wzieba marked this pull request as ready for review July 22, 2024 13:40

AliSoftware requested changes Jul 22, 2024

View reviewed changes

wzieba and others added 2 commits July 22, 2024 16:54

Simplify sed logic

1b5891a

Remove only <?xml header from the single-test report

Update .buildkite/commands/run-unit-tests.sh

8376d1f

Co-authored-by: Olivier Halligon <[email protected]>

wzieba requested a review from AliSoftware July 22, 2024 15:03

AliSoftware reviewed Jul 22, 2024

View reviewed changes

merge_junit.sh Outdated Show resolved Hide resolved

wzieba added 4 commits July 22, 2024 17:11

Pass list of report files directly to sed

d97b38c

So we don't have to iterate with `for` loop

Loose the xml tag requirements

e0c0b6a

Update the sed pattern to reflect xml tag better

9fc65f0

Add comment about lack of <testsuites> tag

01d9f61

AliSoftware approved these changes Jul 22, 2024

View reviewed changes

wzieba enabled auto-merge July 22, 2024 15:23

wzieba disabled auto-merge July 22, 2024 15:24

temp: break unit test

46c216c

To assert Buildkite annotation logic works as expected

Revert "temp: break unit test"

ece3c52

This reverts commit 46c216c.

wzieba enabled auto-merge July 22, 2024 15:44

wzieba merged commit 15f3df3 into trunk Jul 22, 2024
12 of 14 checks passed

wzieba deleted the merge_junit_report branch July 22, 2024 15:55

wzieba added a commit to Automattic/a8c-ci-toolkit-buildkite-plugin that referenced this pull request Aug 2, 2024

Introduce merge_junit script

3c5ffa1

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

wzieba added a commit to Automattic/a8c-ci-toolkit-buildkite-plugin that referenced this pull request Aug 2, 2024

Introduce merge_junit script

771b5f3

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

wzieba mentioned this pull request Aug 2, 2024

Introduce merge_junit script Automattic/a8c-ci-toolkit-buildkite-plugin#103

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge junit reports into one file #12064

Merge junit reports into one file #12064

wzieba commented Jul 18, 2024 •

edited

Loading

wpmobilebot commented Jul 18, 2024 •

edited

Loading

wpmobilebot commented Jul 18, 2024 •

edited

Loading

wzieba Jul 22, 2024

AliSoftware Jul 22, 2024 •

edited

Loading

AliSoftware Jul 22, 2024 •

edited

Loading

wzieba Jul 22, 2024

wzieba Jul 22, 2024

AliSoftware Jul 22, 2024

wzieba Jul 22, 2024

AliSoftware Jul 22, 2024 •

edited

Loading

wzieba Jul 22, 2024

AliSoftware left a comment

Merge junit reports into one file #12064

Merge junit reports into one file #12064

Conversation

wzieba commented Jul 18, 2024 • edited Loading

Description

Problem to address

Possible solution

Why unit tests produces multiple .xml reports and instrumented tests produce single?

Testing information

Future actions

wpmobilebot commented Jul 18, 2024 • edited Loading

wpmobilebot commented Jul 18, 2024 • edited Loading

wzieba Jul 22, 2024

Choose a reason for hiding this comment

AliSoftware Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

AliSoftware Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

wzieba Jul 22, 2024

Choose a reason for hiding this comment

wzieba Jul 22, 2024

Choose a reason for hiding this comment

AliSoftware Jul 22, 2024

Choose a reason for hiding this comment

wzieba Jul 22, 2024

Choose a reason for hiding this comment

AliSoftware Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

wzieba Jul 22, 2024

Choose a reason for hiding this comment

AliSoftware left a comment

Choose a reason for hiding this comment

wzieba commented Jul 18, 2024 •

edited

Loading

Why unit tests produces multiple `.xml` reports and instrumented tests produce single?

wpmobilebot commented Jul 18, 2024 •

edited

Loading

wpmobilebot commented Jul 18, 2024 •

edited

Loading

AliSoftware Jul 22, 2024 •

edited

Loading

AliSoftware Jul 22, 2024 •

edited

Loading

AliSoftware Jul 22, 2024 •

edited

Loading