Replace test-collector-swift with JUnit upload#25251
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 30990 | |
| Version | PR #25251 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | c09fdcb | |
| Installation URL | 5sbjkvpprh5q8 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 30990 | |
| Version | PR #25251 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | c09fdcb | |
| Installation URL | 02913jhi7oh58 |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the test-collector-swift SPM package (runtime XCTest hook) with the Buildkite test-collector plugin that uploads JUnit XML files to Test Engine. This migration enables test analytics for Swift Testing tests, which were invisible to the runtime hook approach.
Changes:
- Removed
test-collector-swiftSPM dependency and all supporting code - Added Buildkite test-collector plugin (v1.11.0) configuration to pipeline
- Updated test reporting to use JUnit XML format with standard filename
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
fastlane/lanes/build.rb |
Removed TEST_ANALYTICS_ENVIRONMENT constant, inject_buildkite_analytics_environment method, trainer call, and buildkite_ci? helper; changed output_types to 'junit' |
Tests/WordPressKitTests/WordPressKitTests/Tests/TestCollector+Constants.swift |
Deleted file containing TestCollector API host constant |
Tests/WordPressKitTests/WordPressKitTests/Tests/RemoteTestCase.swift |
Removed BuildkiteTestCollector import and updated stubAllNetworkRequestsWithNotConnectedError to stub all requests |
Modules/Package.swift |
Removed test-collector-swift package dependency from three test targets |
Modules/Package.resolved |
Removed resolved test-collector-swift package entry |
.buildkite/shared-pipeline-vars |
Added TEST_COLLECTOR_PLUGIN_VERSION and TEST_COLLECTOR environment variables |
.buildkite/pipeline.yml |
Added test-collector plugin configuration to WordPress Unit Tests, Jetpack UI Tests (iPhone), and Jetpack UI Tests (iPad) |
.buildkite/commands/run-unit-tests.sh |
Removed BUILDKITE_ANALYTICS_TOKEN export and updated annotate_test_failures file path to report.junit |
.buildkite/commands/run-unit-tests-reader.sh |
Removed commented-out test analytics token configuration |
.buildkite/commands/run-ui-tests.sh |
Removed BUILDKITE_ANALYTICS_TOKEN export logic and updated annotate_test_failures file path to report.junit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Not sure why the Prototype builds error with: While I'd think that it worked before? Also, there are some Rubocop lint errors, (I'm not sure why they didn't get inlined as PR comment though… ): |
AliSoftware
left a comment
There was a problem hiding this comment.
Approving to unblock, but left some minor suggestion about artifact naming + a recommendation for keeping fail_build: true, so disabled auto-merge to let you the opportunity to fix them—in addition to the 3 test failures on CI—before merging.
| plugins: | ||
| - $CI_TOOLKIT_PLUGIN | ||
| - ${TEST_COLLECTOR}: | ||
| files: build/results/report.junit |
There was a problem hiding this comment.
It would be nice if the JUnit report files could have a different/unique name for WordPress vs Jetpack and UI vs Unit tests (like they had before), so that once they're uploaded as build artifacts (builds/results/*) they are easy to distinguish?
Since we have one step/job for each of those technically this shouldn't be an issue—each step will have its own separate artifacts attached to it, so it's not like one would overwrite the other across jobs—but in case someone needs to call buildkite-agent artifact download at some point to download them, it'd be nicer if they wouldn't need to specify the --step <step_key_name> to be able to differentiate / indicate which of they they want to download… especially since some of those steps don't have a key to allow to identify them anyway. While if those artifacts had unambiguous names, different across jobs, the --step wouldn't be needed to begin with.
| max_concurrent_simulators: CONCURRENT_SIMULATORS | ||
| ) | ||
|
|
||
| trainer(path: lane_context[SharedValues::SCAN_GENERATED_XCRESULT_PATH], fail_build: true) |
There was a problem hiding this comment.
If we remove this line, shouldn't we then restore the fail_build: argument of run_tests to true on line L138R125 to keep the behavior that failed tests should mark the CI build as failed?
Indeed, before run_tests was using fail_build: false because we then called trainer as a subsequent action, with its fail_build: true. But now that we don't call trainer (I think because it's implicitly called by run_tests when output_types contains junit, iinm), to keep the behavior we want to keep fail_build: true somewhere.
[EDIT] Added an adhoc GitHub suggestion for this change above
fastlane/lanes/build.rb
Outdated
| result_bundle: true, | ||
| output_types: '', | ||
| output_types: 'junit', | ||
| fail_build: false, |
There was a problem hiding this comment.
| fail_build: false, | |
| fail_build: true, |
Looks like #25232 merged a major version bump of the plugin that broke the prototype build.
And #25264 fixed it. |
--- Generated with the help of Claude Code, https://code.claude.com Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The default is `true`, which is what we want now that `scan` handles failures directly via JUnit instead of delegating to `trainer`. --- Generated with the help of Claude Code, https://claude.ai/claude-code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>
c4a24e9 to
c09fdcb
Compare
|
|
@AliSoftware thank you for the JUnit report file name suggestion. I'm going to merge without it and think about it. Just because I'd like to find a neat way not to duplicate the file name between pipeline and Fastfile |
Maybe another approach is to be ok with them having the same name… but ensuring all pipeline steps that use those artifacts have a |





Summary
Replaces the
test-collector-swiftSPM package (runtime XCTest hook) with thetest-collectorBuildkite plugin that uploads JUnit XML files to Test Engine.The SPM package hooks into XCTest at runtime, so Swift Testing tests are invisible to Test Engine.
The Buildkite plugin uploads JUnit XML files post-step, which works with any test framework.
The SPM dependency and all its supporting code (env injection, token exports,
trainercalls) are removed.The Buildkite plugin is added to the 3 test steps that have a Test Engine token.
Fixes AINFRA-1977
Testing
Posted by Claude (Opus 4.6) on behalf of @mokagio with approval.