Skip to content

Commit

Permalink
Fix trainer failure result handling with objective C tests
Browse files Browse the repository at this point in the history
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.

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:

- #21565
- #21432
  • Loading branch information
jaysoffian committed Apr 24, 2024
1 parent df12128 commit b3848ec
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 28 deletions.
10 changes: 4 additions & 6 deletions trainer/lib/trainer/test_parser.rb
Expand Up @@ -267,19 +267,17 @@ def summaries_to_data(summaries, failures, output_remove_retry_attempts: false)
end
info[:retry_count] = retry_count

# Set failure message if failure found
failure = test.find_failure(failures)
if failure
case test.test_status
when "Failure"
test_row[:failures] = [{
file_name: "",
line_number: 0,
message: "",
performance_failure: {},
failure_message: failure.failure_message
failure_message: test.failure_message(failures)
}]

info[:failure_count] += 1
elsif test.test_status == "Skipped"
when "Skipped"
test_row[:skipped] = true
info[:skip_count] += 1
else
Expand Down
41 changes: 19 additions & 22 deletions trainer/lib/trainer/xcresult.rb
Expand Up @@ -172,29 +172,11 @@ def all_subtests
return [self]
end

def find_failure(failures)
sanitizer = proc { |name| name.gsub(/\W/, "_") }
sanitized_identifier = sanitizer.call(self.identifier)
if self.test_status == "Failure"
# Tries to match failure on test case name
# Example TestFailureIssueSummary:
# producingTarget: "TestThisDude"
# test_case_name: "TestThisDude.testFailureJosh2()" (when Swift)
# or "-[TestThisDudeTests testFailureJosh2]" (when Objective-C)
# Example ActionTestMetadata
# identifier: "TestThisDude/testFailureJosh2()" (when Swift)
# or identifier: "TestThisDude/testFailureJosh2" (when Objective-C)

found_failure = failures.find do |failure|
# 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
end
return found_failure
else
return nil
def failure_message(failures)
found = failures.find do |failure|
self.identifier == failure.normalized_name
end
return found ? found.failure_message : "unknown failure message"
end
end

Expand Down Expand Up @@ -385,6 +367,21 @@ def initialize(data)
super
end

def normalized_name
# Return test_case_name normalized to match the identifier from ActionTestMetadata
# Example TestFailureIssueSummary:
# producingTarget: "TestThisDude"
# test_case_name: "TestThisDude.testFailureJosh2()" (when Swift)
# or "-[TestThisDudeTests testFailureJosh2]" (when Objective-C)
# Example ActionTestMetadata
# identifier: "TestThisDude/testFailureJosh2()" (when Swift)
# or identifier: "TestThisDude/testFailureJosh2" (when Objective-C)
if self.test_case_name.start_with?("-[") && self.test_case_name.end_with?("]")
return self.test_case_name[2...-1].tr(" ", "/")
end
return self.test_case_name.tr(".", "/")
end

def failure_message
new_message = self.message
if self.document_location_in_creating_workspace&.url
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
29 changes: 29 additions & 0 deletions trainer/spec/fixtures/Test.objc.xcresult/Info.plist
@@ -0,0 +1,29 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>dateCreated</key>
<date>2024-04-23T23:38:03Z</date>
<key>externalLocations</key>
<array/>
<key>rootId</key>
<dict>
<key>hash</key>
<string>0~QcTkA55UPcxEP1nO1AY1osK3wJ6ccJbYiM6K7BJmWZ0MFG5UOgWiKPIxZQdQeP-BtGoBEwiEcoYSkz_QqfjjMA==</string>
</dict>
<key>storage</key>
<dict>
<key>backend</key>
<string>fileBacked2</string>
<key>compression</key>
<string>standard</string>
</dict>
<key>version</key>
<dict>
<key>major</key>
<integer>3</integer>
<key>minor</key>
<integer>39</integer>
</dict>
</dict>
</plist>
47 changes: 47 additions & 0 deletions trainer/spec/test_parser_spec.rb
Expand Up @@ -245,6 +245,53 @@
}
])
end

it "works as expected with Objective C xcresult", requires_xcode: true do
tp = Trainer::TestParser.new("./trainer/spec/fixtures/Test.objc.xcresult")
expect(tp.data).to eq([
{
project_path: "HelloWorldTest.xcodeproj",
target_name: "HelloWorldTestTests",
test_name: "HelloWorldTestTests",
configuration_name: "Test Scheme Action",
duration: 0.04512190818786621,
tests: [
{
identifier: "HelloWorldTestTests.testExample",
name: "testExample",
duration: 0.0011409521102905273,
status: "Success",
test_group: "HelloWorldTestTests",
guid: "",
},
{
identifier: "HelloWorldTestTests.testFailure",
name: "testFailure",
duration: 0.043980956077575684,
status: "Failure",
test_group: "HelloWorldTestTests",
guid: "",
failures: [
{
failure_message: "((NO) is true) failed - Fail (/Users/jsoffian/HelloWorldTest/HelloWorldTestTests/HelloWorldTestTests.m#CharacterRangeLen=0&EndingLineNumber=39&StartingLineNumber=39)",
file_name: "",
line_number: 0,
message: "",
performance_failure: {}
}
]
}
],
number_of_tests: 2,
number_of_failures: 1,
number_of_skipped: 0,
number_of_tests_excluding_retries: 2,
number_of_failures_excluding_retries: 1,
number_of_retries: 0
}
])
end

end
end
end

0 comments on commit b3848ec

Please sign in to comment.