Skip to content
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

Fix trainer failure result handling with objective C tests #21990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaysoffian
Copy link
Contributor

@jaysoffian jaysoffian commented Apr 24, 2024

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

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 (#21432), 2024-01-10) changed this transformation code in a way that fixed test names that contain spaces, but broke Objective-C test names.

Description

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:

Testing Steps

I ran rubocop and rspec on the trainer code. It passes the existing tests as well as a new test I added to identify this issue and prevent regressions.

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
@jaysoffian jaysoffian force-pushed the fix-trainer-objc-test-name-parsing branch from b3848ec to 0df4bea Compare April 25, 2024 22:08
@jaysoffian
Copy link
Contributor Author

Not sure why the "Execute tests on macOS (Xcode 15.3, Ruby 3.3) job fails, but it doesn't seem to have anything to do with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant