[trainer] Support parsing test results from SwiftTesting #29463
Conversation
Extract parsing of PLIST and Parsing of --legacy xcresulttool output each in their own dedicated file, on preparation for refactoring and introducing new xcresult parsing method for Xcode 16
To avoid storing useless large data from fixture in git, while it's not used for our test cases
To ensure that tools that parse the generated JUnit would consider those as different test cases, instead of multiple runs of the same test case. We already have <property name="argument" value="…"> to help us distinguish, but in practice most of the tools that read JUnit XML files will now know about that property or account for it, and only look at the `<testcase classname=…name=…>` attributes to differentiate cases.
Into a dedicated file, for easier browsing of the code and smaller files to scroll through.
Generated by 🚫 Danger |
So that even you run the tests on CI with older versions of Xcode (that don't need nor know the `--legacy` flag) would still run and pass those tests
Use Open3 to run the command instead of backticks - So that we can capture the error if it happens - So that the code is more resilient against special characters - So that we avoid the involvement of an intermediate shell (that would have to interpret the string into parameters before passing it to the process, requiring us to `Shellwords.escape` it in the first place… instead of just passing the parameters to the process directly without involving some unnecessary shell escaping + parsing intermediary)
|
Happy to test this with my projects! Thank you! 👏 I had just started to figure out Swift Testing issues and had started testing with #29470, which addressed issues with test cases nested in inner classes and included some tests specific to Swift Testing, but I saw no way to handle parameterized tests (see #29174) and a more explicit/clean way to detect and remove auto-retries without using the newer, non-legacy |
|
@AliSoftware One thing I'm seeing so far in my testing: [16:41:04]: Called from Fastfile at line 154
[16:41:04]: ```
[16:41:04]: 152: desc "Uses trainer to collect test results after a test run"
[16:41:04]: 153: lane :collect_test_results do |options|
[16:41:04]: => 154: trainer(
[16:41:04]: 155: path: "#{options[:test_plan]}_test_result_bundle/Test-#{options[:test_plan]}.xcresult",
[16:41:04]: 156: output_directory: "test_results",
[16:41:04]: ```
[16:41:04]: Illegal character "\u0001" in raw string "audioPlayerReturnedForOnlyValidInput(\u0001)"For a test that looks something like: @Test(.serialized, arguments: (0..<128).map(UnicodeScalar.init).map(Character.init))
func audioPlayerReturnedForOnlyValidInput(asciiCharacter: Character) throws { ... }Hopefully some kind of sanitization we can do on test names? Similar to https://github.com/fastlane/fastlane/blob/alisoftware/trainer/support-swift-testing/trainer/lib/trainer/legacy_xcresult.rb#L180? |
|
Thanks so much @TheMetalCode for testing this PR against some more concrete tests!
Ah, good catch. Yeah, that sanitization seems like a decent plan and easy solution. I'd like to better understand the root cause of this issue first though rather than blindly applying a workaround that was applied at the time the first |
|
Btw, for refs, below is the dummy Xcode project I used to generate the Maybe I should commit it in the repo, even if the project itself is not used by the tests, so that it'd be easier to re-generate the Speaking of which, feel free to add your test case from above to that dummy Xcode project then regenerate the |
The `*.xcresult/database.sqlite3` database file contains the same data as what's serialized in the `.xcresult/Data/*` files, and is just an alternate (and more modern and disk-space-optimized) representation of the same info that the latest `xcresulttool` supports So we might as well commit just the database instead of the whole Data/* folder and its files, not only to make it take less space on disk, but also to make the git diff and logs easier to deal with.
As apparently JUnit conventions don't allow the root <testsuites> element to contain <properties> elements, only <testsuite> child elements can h/t @TheMetalCode for noticing in #29463 (comment)
As apparently JUnit conventions don't allow the root <testsuites> element to contain <properties> elements, only <testsuite> child elements can h/t @TheMetalCode for noticing in #29463 (comment)
`trainer` -> _trainer_
254deb7 to
cefe134
Compare
Co-authored-by: Jason Hagglund <jhagglund@doximity.com>
|
|
||
| def ensure_file_valid!(raw_json) | ||
| format_version = raw_json["FormatVersion"] | ||
| supported_versions = ["1.1", "1.2"] |
There was a problem hiding this comment.
Does it mean that with every new version of Xcode, we need to check if the format has changed and support the new version if necessary?
There was a problem hiding this comment.
Yes and no.
To the best of my understanding, this .plist format is legacy format, even older than before .xcresult bundles and xcresulttool were introduced. This is just existing code that was initially in xcresult.rb and that I extracted in its own file just like I did for legacy_xcresult.rb so we keep that backwards compatibility, but in practice I'm not sure any modern version of Xcode produces those .plist file formats for test summaries anymore, so I don't see a new FormatVersion being added ever by now.
| @@ -50,6 +50,11 @@ def self.available_options | |||
| description: "Produces class name and test name identical to xcpretty naming in junit file", | |||
| is_string: false, | |||
| default_value: false), | |||
| FastlaneCore::ConfigItem.new(key: :force_legacy_xcresulttool, | |||
There was a problem hiding this comment.
This new option key is going to help to rollout this implementation with minimized regression risk 👏
|
Hey Ali, Thanks a lot for this PR! Supporting SwiftTesting is a very much welcomed addition, and I truly appreciate the effort you put into it. Also, a special shoutout for the well-written PR description. It was super informative and made reviewing this big PR much smoother. The clear steps you provided helped me a lot! Great work, and thanks again for contributing! 🚀 |
I agree, we could easily add more test cases in the future there 👍 |
Especially to be clearer about the Repetition type
|
@mollyIV I should have addressed all your suggestions, so ready for a re-review (and hopefully approval) 🙂 |
There was a problem hiding this comment.
For whatever my approval is worth: I've been continuously running this branch on two iOS projects over the last week or so, just over 10K test cases, about 60-70% of which are Swift Testing. Aside from the ASCII character parsing and junit schema issues that have already been addressed with follow-up commits, I've seen no issues and can happily report that all failing test cases, Swift Testing or otherwise, are being properly reflected in the XML report.
This is a very welcome and deeply appreciated change, thank you! 👏
This PR allows
trainer(and by extensionscan/run_tests) to support test results from Swift Testing—by parsing the.xcresultbundle using the newxcresulttoolAPI☑️ Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)❔ Motivation and Context
In Xcode 16b3, the
xcresulttooltool—that ships with Xcode Command Line Tools to parse and extract data from.xcresultbundles—had its CLI API updated.So far we've relied on adding the
--legacyflag to ourxcresulttoolinvocations to be able to still execute old CLI and still use the old way of calling the tool, usingxcresulttool get <object> --format json/xcresulttool get <object> --id <id>. But:The version of
xcresulttoolthat ships with Xcode 16+ now expects us to call the CLI withxcresulttool get test-results test …, and generates a new JSON format (whose JSONSchema is described inxcresulttool get test-results test --help) that is easier to parse but more importantly also supports Swift Testing test results.✍️ Description
Instead of trying to modify the implementation of
xcresult.rbto adapt to the new command and JSON format—which would make the code difficult to read if we wanted to keep backwards compatibility in the same codebase—and because the new JSON format is way better structured and doesn't need the complexity of what the old XCResult parser implementation needed, I've decided to instead avoid piling up on the legacy implementation and implement the parsing of new JSON format completely separately.I have thus:
.xcresultformat as well as the old.plistformat in dedicated classes and files, to separate that legacy code clearly..xcresultformat toLegacyXCResultXCResultclass instead, dedicated to parsing the new formatTestParsercall theparse_xcresultmethod of eitherLegacyXCResultorXCResultclass depending on the version ofxcresulttoolbeing used/installed (with a possibility to force the legacy mode if needed, to avoid breaking things accidentally if we missed some edge cases in the new implementationThe new implementation of
XCResultparsing is also now split into nested classes underxcresult/*.rb, namelyTestPlan,TestSuite,TestCase, andRepetitions, which make semantic sense and are easier to reason about when parsing thexcresultJSON representation.The generation of the output JUnit file is also directly handled in the model objects (
to_xmlmethods) rather than using ERB (viaJunitGenerator) like the old implementation was using.🧪 Testing Steps
I've added a couple of Unit Tests to
test_parser_spec.rb, that use an.xcresultfile that I generated from a dummyxcproject1, which contained a mix of Unit and UITests, implemented with a mix of XCTest and SwiftTesting, and using features like test skipping, arguments, and retries.Hopefully this should cover a good set of features that those testing frameworks offer.
That being said, I'd love if more people would test this implementation against some
.xcresultfiles of their own Xcode projects, to validate the generated JUnit.xmllooks right and contains all necessary info (failure messages, etc) and so that it covers a whole range of possibilities (e.g. tests runs with or without retries enabled, custom traits, etc…)👁️ Suggestions on how to review this PR
This PR's diff is huge, but this is mostly because:
.junitexpectation fixtures which are XMLs of >1K lines each).xcresultfixture, even if that Xcode project is not directly referenced by any of the rspec tests, but so that it'd be easy to add more tests in the future in we wanted to regenerate the.xcresultto cover more edge casesI also had to commit the new[EDIT] I ended up only commiting the.xcresultfixture… which is in practice a bundle containing a lot of small (mostly-binary) files. So that shows up as a lot of files in this PR's diff, while those don't need to be manually reviewed at all when reviewing that PR..xcresult/database.sqlite3file as I discovered that it was enough forxcresulttoolto operate.I'd thus suggest the reviewers to instead focus on:
trainer/lib/*— keeping in mind that some of those changes are just existing code for the legacy parsers having been moved to dedicated filestrainer/spec/*_spec.rbtrainer/spec/fixtures, only check the*.junitexpectation fixtures that were added. You can in particular safely ignore:trainer/spec/fixtures/Xcode16-Mixed-XCTest-SwiftTesting.xcresult/*folder and its files, since that fixture was auto-generated (it's the bundle that Xcode generated from my dummy project)Xcode16-Mixed-XCTest-SwiftTesting.jsonfixture, as that fixture was also auto-generated (by runningxcresulttoolon the added.xcresultbundle fixture)trainer/spec/fixtures/Xcode16-Mixed-XCTest-SwiftTesting-Project/folder, since this just contains the dummy Xcode project with lots of dummy tests that was used to generate the.xcresultfixture. Of course it's ok if you want to review the Xcode project itself to compare how those various ways of writing tests in Swift appear in the JUnit XML fixture files, but it's probably not worth reviewing that in GitHub directly if so, but directly in Xcode instead.Hopefully those suggestions should help reduce the scope of the review of this PR by a lot
Footnotes
Created with Cursor and AI agent, so I could quickly generate a full project with a wide range of all possible API usages and edge cases of XCTest and SwiftTesting features ↩