[trainer] sanitize test case name and test identifier consistently when looking up test failures #21565
Conversation
| .tr("-", "") | ||
| .tr("[", "") | ||
| .tr("]", "") | ||
| .tr(" ", "/") |
There was a problem hiding this comment.
In my case, this ended up being the unmet expectation - not all spaces were replaced by / in the identifier. So the comparison ended up being [somewhat redacted due to proprietary info]:
[1] pry(#<Trainer::XCResult::ActionTestMetadata>)> self.identifier
=> "MyPrivateSpec/my test condition, should return an error()"
[2] pry(#<Trainer::XCResult::ActionTestMetadata>)> sanitized_test_case_name
=> "MyPrivateSpec/my/test/condition,/should/return/an/error()"
Anyways, the bet I'm making with this PR is: rather than have any expectation of how self.identifier is formatted, sanitize both it and failure.test_case_name in the same fashion then compare.
…e-test-case-and-identifier-consistently
…e-test-case-and-identifier-consistently
…e-test-case-and-identifier-consistently
…e-test-case-and-identifier-consistently
…e-test-case-and-identifier-consistently
…e-test-case-and-identifier-consistently
There was a problem hiding this comment.
Hi @TheMetalCode 👋
Thanks for this PR! Your rationale makes sense to me, and I think this is a good solution moving forward.
Do you mind adding unit tests to this PR, hopefully with real-life strings for both test case name and identifiers? Ideally we should cover the happy path (scenarios where the previous implementation used to work) as well as the edge case you were seeing (so that it fails, when run using the previous comparison algorithm, but succeeds now using the new algorithm). That would be super!
Let me know if you need any support adding these tests 🤗 thanks again for your hard work! 🙏
PS: If you could also add steps to reproduce this issue locally so we can verify your results, that would be great. The simpler these steps are, the better (extra points if you can provide a demo project where the issue is reproducible) 🙇
rogerluan
changed the title
|
Heads up: no need to implement the unit tests in this PR. I decided to merge part of this PR's solution into the PR #21432 where we already have unit tests and a sample project, so once we merge that PR, this one can be closed 🙇 Thanks for the contribution in this PR! 🙌 |
That's great, thank you! My struggle with unit tests on this one is that I had them locally but they were relying on sample xcresult bundles from a private project and I didn't know of a way to edit xcresult files to make them non-proprietary. This private project is using Quick/Nimble though, didn't realize that in and of itself would create the error condition. Anyways, happy to have influenced #21432, looking forward to the fix! |
TheMetalCode
force-pushed
the
jjh/trainer-sanitize-test-case-and-identifier-consistently
branch
from
13ed3d4
to
ce58be5
Compare
When trainer is parsing an xcresult bundle which contains test failures, it has an `ActionTestMetadata` object that is identified one way (by "identifier") and it's trying to find the corresponding `TestFailureIssueSummary` object that's identified another way (by `testCaseName`). These values are not the same, so in order to locate the correct `TestFailureIssueSummary` object it needs to transform the `testCaseName` in the same way that Xcode does when generating the `ActionTestMetadata` identifier. Exactly how to do this is undocumented, so folks have attempted to figure it out empirically. Most recently, dda4007 ([trainer] fix issues where number of failures would always be zero (fastlane#21432), 2024-01-10) changed this transformation code in a way that fixed test names that contain spaces, but broke Objective-C test names. This commit fixes the regression caused by dda4007 with ObjC tests as follows: 1) It reverts the transformation behavior to how the code worked previously. Instead of "sanitizing" both the identifier and the test case name, it now keeps the identifier as is and "normalizes" the test case name. The test case name normalization has been moved into its own method which passes unit tests for ObjC, Swift, and tests with spaces. 2) It adds an ObjC xcresult bundle and unit test to prevent regressing parsing ObjC test names. 3) It refactors summaries_to_data so that if the corresponding test failure message cannot be located (but hopefully that won't happen anymore), we at least properly report that the test failed, but use the generic "unknown failure message" as the failure message. See discussion on: - fastlane#21565 - fastlane#21432
When trainer is parsing an xcresult bundle which contains test failures, it has an `ActionTestMetadata` object that is identified one way (by "identifier") and it's trying to find the corresponding `TestFailureIssueSummary` object that's identified another way (by `testCaseName`). These values are not the same, so in order to locate the correct `TestFailureIssueSummary` object it needs to transform the `testCaseName` in the same way that Xcode does when generating the `ActionTestMetadata` identifier. Exactly how to do this is undocumented, so folks have attempted to figure it out empirically. Most recently, dda4007 ([trainer] fix issues where number of failures would always be zero (fastlane#21432), 2024-01-10) changed this transformation code in a way that fixed test names that contain spaces, but broke Objective-C test names. This commit fixes the regression caused by dda4007 with ObjC tests as follows: 1) It reverts the transformation behavior to how the code worked previously. Instead of "sanitizing" both the identifier and the test case name, it now keeps the identifier as is and "normalizes" the test case name. The test case name normalization has been moved into its own method which passes unit tests for ObjC, Swift, and tests with spaces. 2) It adds an ObjC xcresult bundle and unit test to prevent regressing parsing ObjC test names. 3) It refactors summaries_to_data so that if the corresponding test failure message cannot be located (but hopefully that won't happen anymore), we at least properly report that the test failed, but use the generic "unknown failure message" as the failure message. See discussion on: - fastlane#21565 - fastlane#21432
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
I recently had an issue in CI builds where failing tests were not being reported as failures once trainer converted the xcresult to junit xml. After digging in and debugging, I found that the existing logic in
Trainer::XCResult::ActionTestMetadata#find_failurewas expecting the test identifier to have a particular format that didn't end up matching. This led me to the insight that instead of having this expectation of the identifier, we could sanitize both it and the test case name consistently, then compare them for a potential match.Using xcresult artifacts from the failing CI runs, this fix was effective. Unfortunately, since those xcresult artifacts contain proprietary data, I've yet to figure out how to unit test this safely. 🙈
Description
Testing Steps