-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[trainer] fix issues where number of failures would always be zero #21432
[trainer] fix issues where number of failures would always be zero #21432
Conversation
The failure is unrelated to my changes. Likely existing issue. |
@joshdholtz any chance you can take a look at this? |
This reverts commit 0281dc5.
# Conflicts: # trainer/spec/test_parser_spec.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for providing a unit test, a fixture .xcresult and a sample project @tahirmt !
I reviewed your solution and it was great, but I thought of merging in the approach that @TheMetalCode applied in his PR that addresses this very same issue #21565 (but his PR was lacking unit tests 😅). In summary, I replaced the "String extension" that you created with a simple proc
, and changed the implementation of the sanitizer from using several individual custom string replacements, with a single regex-replace that looks for all \W
(i.e. non-word characters).
I think this is more fail-proof, and passes all the tests 😊
This PR LGTM already. I've made the changes above and solved some merge conflicts that were present. I'll give a few more days before merging it to see if there's anyone else interested in reviewing it too 🙇 maybe @lacostej ?
Thanks once again for this @tahirmt , this is a long-lasting issue! 🙏
PS: @tahirmt let me know if you have any thoughts on the changes I made 🙇 |
@rogerluan the solution looks good to me. Thanks for updating it and moving it forward. |
No problem! Looking forward to merging this soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@rogerluan any idea when we can expect it to be merged? |
Hi @tahirmt, I'm OOO till Sunday, but I'll take a look once I'm back :) |
Thanks for another great contribution to fastlane @tahirmt 🤗 this is going in now! Thank you for your patience 🙇 |
@rogerluan sorry to bug you but do you have any idea when the next fastlane release would go out? It's been weeks since it was merged but no release yet. |
Nope, sorry 😬 I don't have control over the releases yet 😅 I'll try to get a release out soon though 🙏 |
This change actually broke parsing for at least some result files: If I can't figure this out myself, I'll attach the sanitized output of Result bundle was generated by Xcode 15.1. Edit: the bug is in the
Since the Now to figure out what this sanitation is all about, and how best to handle the case where we know a test failed but we aren't able to find it. Edit 2: here's the data it's failing to parse correctly run through
Edit 3: okay, so the fundamental problem is that the two raw strings (from the parsed output of
When it "sanitizes" these values they become:
This whole endeavor seems fraught with peril. The old code worked (in this specific instance) because it stripped the extra characters from the test case name:
The new code no longer works because instead of stripping these characters entirely, it's replacing them with underscores which is not sufficient to match the normalization process that (apparently) xcode/xcresult uses. I think this PR may have misunderstood what the original code was trying to do. It's not sanitizing the test case name so much as normalizing it in order to match the identifier value from |
# Sanitize both test case name and identifier in a consistent fashion, then replace all non-word | ||
# chars with underscore, and compare them | ||
sanitized_test_case_name = sanitizer.call(failure.test_case_name) | ||
sanitized_identifier == sanitized_test_case_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a regression. See #21432 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix #21990
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 rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
Closes #21565
Description
If the test case name has spaces or any special characters, the reported
number_of_failures
will always be 0. This will also not include any failed tests inside the junit report.The reason this was happening was because during the comparison to check the test failure there was a sanitization step that's not applied to the identifier itself so it never matched.
This is from the log output of such a test.
Testing Steps
The easiest way to reproduce it was to use https://github.com/Quick/quick because Quick 7 produces test case names with spaces if the spec names had spaces in it. And when you run
scan
the output will always say the number of failures is 0.I have attached a sample project.
bundle install
bundle exec fastlane test
When it runs the output will show number of failures as 0.
Then you can uncomment the branch line in the
Gemfile
and run bundle install and switch to this branch. Then you will see that the number of failure is 1.SpaceTests.zip