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

[trainer] fix issues where number of failures would always be zero #21432

Merged
merged 6 commits into from Jan 10, 2024

Conversation

tahirmt
Copy link
Contributor

@tahirmt tahirmt commented Jul 31, 2023

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.

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.

** TEST FAILED **
[15:11:03]: Exit status: 65
+--------------------+---+
|      Test Results      |
+--------------------+---+
| Number of tests    | 3 |
| Number of failures | 0 |
+--------------------+---+

[15:11:04]: Test execution failed. Exit status: 65

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.

  1. Download project
  2. Run bundle install
  3. Run 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

@tahirmt tahirmt marked this pull request as ready for review July 31, 2023 19:21
@tahirmt tahirmt changed the title [Trainer] Sanitize both identifier and test case to allow spaces in test name [trainer] Sanitize both identifier and test case to allow spaces in test name Jul 31, 2023
@tahirmt
Copy link
Contributor Author

tahirmt commented Jul 31, 2023

The failure is unrelated to my changes. Likely existing issue.

@tahirmt
Copy link
Contributor Author

tahirmt commented Sep 27, 2023

@joshdholtz any chance you can take a look at this?

@rogerluan rogerluan changed the title [trainer] Sanitize both identifier and test case to allow spaces in test name [trainer] fix issues where number of failures would always be zero Dec 28, 2023
Copy link
Member

@rogerluan rogerluan left a 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! 🙏

@rogerluan
Copy link
Member

rogerluan commented Dec 28, 2023

PS: @tahirmt let me know if you have any thoughts on the changes I made 🙇

@tahirmt
Copy link
Contributor Author

tahirmt commented Dec 28, 2023

@rogerluan the solution looks good to me. Thanks for updating it and moving it forward.

@rogerluan
Copy link
Member

No problem! Looking forward to merging this soon!

Copy link
Collaborator

@lacostej lacostej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tahirmt
Copy link
Contributor Author

tahirmt commented Jan 4, 2024

@rogerluan any idea when we can expect it to be merged?

@rogerluan
Copy link
Member

Hi @tahirmt, I'm OOO till Sunday, but I'll take a look once I'm back :)

@rogerluan
Copy link
Member

Thanks for another great contribution to fastlane @tahirmt 🤗 this is going in now! Thank you for your patience 🙇

@rogerluan rogerluan merged commit dda4007 into fastlane:master Jan 10, 2024
2 checks passed
@tahirmt tahirmt deleted the fix-trainer-not-reporting-failures branch January 10, 2024 12:19
@tahirmt
Copy link
Contributor Author

tahirmt commented Jan 22, 2024

@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.

@rogerluan
Copy link
Member

Nope, sorry 😬 I don't have control over the releases yet 😅 I'll try to get a release out soon though 🙏

@jaysoffian
Copy link
Contributor

jaysoffian commented Apr 19, 2024

This change actually broke parsing for at least some result files:

Screenshot 2024-04-19 at 5 33 57 PM

If I can't figure this out myself, I'll attach the sanitized output of xcrun xcresulttool get --format json for the xcresult bundle in question. I'm just noting this PR introduced a regression.

Result bundle was generated by Xcode 15.1.

Edit: the bug is in the find_failure method, specifically in the updated failures.find block.

  • self.identifier : OPHUIManageAccountsTestCase/testManageAccountsSignIn
  • sanitized_identifier : OPHUIManageAccountsTestCase_testManageAccountsSignIn
  • failure.test_case_name : -[OPHUIManageAccountsTestCase testManageAccountsSignIn]
  • sanitized_test_case_name: __OPHUIManageAccountsTestCase_testManageAccountsSignIn_

Since the sanitized_identifier does not match the sanitized_test_case_name, the failures.find block returns nil, so find_failure returns nil to the summaries_to_data function that calls it.

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 gron and eliding a path slightly:

$ cat ActionTestPlanRunSummaries.json | jq '.summaries._values[0].testableSummaries._values[0].tests._values[0].subtests._values[0].subtests._values[0].subtests._values[1]' | gron
json = {};
json._type = {};
json._type._name = "ActionTestMetadata";
json._type._supertype = {};
json._type._supertype._name = "ActionTestSummaryIdentifiableObject";
json._type._supertype._supertype = {};
json._type._supertype._supertype._name = "ActionAbstractTestSummary";
json.duration = {};
json.duration._type = {};
json.duration._type._name = "Double";
json.duration._value = "38.547163009643555";
json.identifier = {};
json.identifier._type = {};
json.identifier._type._name = "String";
json.identifier._value = "OPHUIManageAccountsTestCase/testManageAccountsSignIn";
json.identifierURL = {};
json.identifierURL._type = {};
json.identifierURL._type._name = "String";
json.identifierURL._value = "test://com.apple.xcode/<elided>/OPHUIManageAccountsTestCase/testManageAccountsSignIn";
json.name = {};
json.name._type = {};
json.name._type._name = "String";
json.name._value = "testManageAccountsSignIn";
json.summaryRef = {};
json.summaryRef._type = {};
json.summaryRef._type._name = "Reference";
json.summaryRef.id = {};
json.summaryRef.id._type = {};
json.summaryRef.id._type._name = "String";
json.summaryRef.id._value = "<elided>";
json.summaryRef.targetType = {};
json.summaryRef.targetType._type = {};
json.summaryRef.targetType._type._name = "TypeDefinition";
json.summaryRef.targetType.name = {};
json.summaryRef.targetType.name._type = {};
json.summaryRef.targetType.name._type._name = "String";
json.summaryRef.targetType.name._value = "ActionTestSummary";
json.summaryRef.targetType.supertype = {};
json.summaryRef.targetType.supertype._type = {};
json.summaryRef.targetType.supertype._type._name = "TypeDefinition";
json.summaryRef.targetType.supertype.name = {};
json.summaryRef.targetType.supertype.name._type = {};
json.summaryRef.targetType.supertype.name._type._name = "String";
json.summaryRef.targetType.supertype.name._value = "ActionTestSummaryIdentifiableObject";
json.summaryRef.targetType.supertype.supertype = {};
json.summaryRef.targetType.supertype.supertype._type = {};
json.summaryRef.targetType.supertype.supertype._type._name = "TypeDefinition";
json.summaryRef.targetType.supertype.supertype.name = {};
json.summaryRef.targetType.supertype.supertype.name._type = {};
json.summaryRef.targetType.supertype.supertype.name._type._name = "String";
json.summaryRef.targetType.supertype.supertype.name._value = "ActionAbstractTestSummary";
json.testStatus = {};
json.testStatus._type = {};
json.testStatus._type._name = "String";
json.testStatus._value = "Failure";

$ cat ActionsInvocationRecord.json | jq '.issues.testFailureSummaries._values[0]' | gron
json = {};
json._type = {};
json._type._name = "TestFailureIssueSummary";
json._type._supertype = {};
json._type._supertype._name = "IssueSummary";
json.documentLocationInCreatingWorkspace = {};
json.documentLocationInCreatingWorkspace._type = {};
json.documentLocationInCreatingWorkspace._type._name = "DocumentLocation";
json.documentLocationInCreatingWorkspace.concreteTypeName = {};
json.documentLocationInCreatingWorkspace.concreteTypeName._type = {};
json.documentLocationInCreatingWorkspace.concreteTypeName._type._name = "String";
json.documentLocationInCreatingWorkspace.concreteTypeName._value = "DVTTextDocumentLocation";
json.documentLocationInCreatingWorkspace.url = {};
json.documentLocationInCreatingWorkspace.url._type = {};
json.documentLocationInCreatingWorkspace.url._type._name = "String";
json.documentLocationInCreatingWorkspace.url._value = "file:///<elided>/Tests/Activities/OPHAuthenticationActivity.m#EndingLineNumber=18&StartingLineNumber=18";
json.issueType = {};
json.issueType._type = {};
json.issueType._type._name = "String";
json.issueType._value = "Uncategorized";
json.message = {};
json.message._type = {};
json.message._type._name = "String";
json.message._value = "(([[[OPHUITestHomePage alloc] init] userPresentWithUser: userIdp:) is true) failed";
json.testCaseName = {};
json.testCaseName._type = {};
json.testCaseName._type._name = "String";
json.testCaseName._value = "-[OPHUIManageAccountsTestCase testManageAccountsSignIn]";

Edit 3: okay, so the fundamental problem is that the two raw strings (from the parsed output of xcresulttool it's trying to match up are:

  1. OPHUIManageAccountsTestCase/testManageAccountsSignIn (identifier._value from ActionTestPlanRunSummaries)
  2. -[OPHUIManageAccountsTestCase testManageAccountsSignIn] (testCaseName._value from ActionsInvocationRecord)

When it "sanitizes" these values they become:

  1. OPHUIManageAccountsTestCase_testManageAccountsSignIn
  2. __OPHUIManageAccountsTestCase_testManageAccountsSignIn_

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:

  • Sanitize for Swift by replacing "." for "/"
  • Sanitize for Objective-C by removing "-", "[", "]", and replacing " " for ?/

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 ActionTestPlanRunSummaries. For it to work correctly it has to normalize the strings identically to how xcresulttool does so, not blindly sanitize each of them.

# 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
Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix #21990

jaysoffian added a commit to jaysoffian/fastlane that referenced this pull request Apr 24, 2024
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 added a commit to jaysoffian/fastlane that referenced this pull request Apr 25, 2024
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
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

4 participants