Skip to content

[SwiftTestTool] Add test failure messages to xUnit report #6174

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.

Sign up for GitHub

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

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

[SwiftTestTool] Add test failure messages to xUnit report #6174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...iscellaneous/ParallelTestsPkg/Tests/ParallelTestsPkgTests/ParallelTestsFailureTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,13 @@ class ParallelTestsFailureTests: XCTestCase {
func testSureFailure() {
XCTFail("Giving up is the only sure way to fail.")
}

func testAssertionFailure() {
XCTAssertTrue(false, "Expected assertion failure.")
}

func testExpectationFailure() {
let expectation = XCTestExpectation(description: "failing expectation")
wait(for: [expectation], timeout: 0.0)
}
}
32 changes: 28 additions & 4 deletions Sources/Commands/SwiftTestTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ final class ParallelTestRunner {
var output = ""
let outputLock = NSLock()
let start = DispatchTime.now()
let success = testRunner.test(outputHandler: { _output in outputLock.withLock{ output += _output }})
let success = testRunner.test(outputHandler: { _output in outputLock.withLock{ output += _output.appending("\n") }})
let duration = start.distance(to: .now())
if !success {
self.ranSuccessfully = false
Expand Down Expand Up @@ -1034,9 +1034,17 @@ final class XUnitGenerator {

"""

/// Captured groups
/// $1 = file:line
/// $2 = reason
let testErrorRegex = try RegEx(pattern: #".*\/(.*\d+)\:\serror:.*\s\:\s(.*)"#)

// Captured groups
// $1 = file:line
// $2 = reason
let fatalErrorRegex = try RegEx(pattern: #".*\/(.*\d+)\:\s(Fatal error:.*)"#)

// Generate a testcase entry for each result.
//
// FIXME: This is very minimal right now. We should allow including test output etc.
for result in results {
let test = result.unitTest
let duration = result.duration.timeInterval() ?? 0.0
Expand All @@ -1046,7 +1054,13 @@ final class XUnitGenerator {
"""

if !result.success {
stream <<< "<failure message=\"failed\"></failure>\n"
let message = [testErrorRegex, fatalErrorRegex].map { $0.matchGroups(in: result.output) }
.first(where: { !$0.isEmpty })?
.first?
.joined(separator: " ")
.xmlEscaped ?? "failed"

stream <<< "<failure message=\"\(message)\"></failure>\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an improvement over the hard coded "failed" message, but a bit fragile. @stmontgomery any advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

It really should not be necessary to scrape the textual logging output of XCTests and parse it using regex. We have a proper API for observing events and retrieving this data in structured ways: XCTestObservation.

However, I think SwiftPM needs to coordinate doing this for it to be effective but it doesn't do that today. So I can understand why this approach relying on more-and-more intricate regex parsing is seen as the easiest solution at the moment. I believe SwiftPM already parses the textual logging output to a certain extent (right?), so this is only adding a bit more and I'm not going to stand in the way of landing this. But I think we really should spend some time building the necessary infrastructure for SwiftPM to coordinate more closely with the runner process to gather its results in a structured way, to avoid the need for any of this.

@tomerd Do you know if we already have an Issue tracking this sort of enhancement on the SwiftPM side?

Copy link
Contributor

@tomerd tomerd Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@stmontgomery we have been pushing back on adding parsing of XCTest output and we would very much like to see a structured output from XCTest instead of the non-structured one we have now - it will help solve several issues. iirc we looked at this before and the conclusion was that the invocation of XCTest goes through several layers that essentially shell out so there is little room for setting up delegates etc, but maybe there was a flag we can pass to the process that can assist? open to ideas on how to be at achieve this

}

stream <<< "</testcase>\n"
Expand Down Expand Up @@ -1116,3 +1130,13 @@ private extension Basics.Diagnostic {
.warning("No matching test cases were run")
}
}

private extension String {
var xmlEscaped: String {
self.replacingOccurrences(of: "&", with: "&amp;")
.replacingOccurrences(of: "\"", with: "&quot;")
.replacingOccurrences(of: "'", with: "&#39;")
.replacingOccurrences(of: ">", with: "&gt;")
.replacingOccurrences(of: "<", with: "&lt;")
}
}
13 changes: 10 additions & 3 deletions Tests/CommandsTests/TestToolTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ final class TestToolTests: CommandsTestCase {
XCTAssertMatch(error.stdout, .contains("testExample2"))
XCTAssertNoMatch(error.stdout, .contains("'ParallelTestsTests' passed"))
XCTAssertMatch(error.stdout, .contains("'ParallelTestsFailureTests' failed"))
XCTAssertMatch(error.stdout, .contains("[3/3]"))
XCTAssertMatch(error.stdout, .contains("[5/5]"))
}

do {
Expand All @@ -115,15 +115,22 @@ final class TestToolTests: CommandsTestCase {
XCTAssertMatch(error.stdout, .contains("testExample2"))
XCTAssertMatch(error.stdout, .contains("'ParallelTestsTests' passed"))
XCTAssertMatch(error.stdout, .contains("'ParallelTestsFailureTests' failed"))
XCTAssertMatch(error.stdout, .contains("[3/3]"))
XCTAssertMatch(error.stdout, .contains("[5/5]"))
}

// Check the xUnit output.
XCTAssertFileExists(xUnitOutput)
let contents: String = try localFileSystem.readFileContents(xUnitOutput)
XCTAssertMatch(contents, .contains("tests=\"3\" failures=\"1\""))
XCTAssertMatch(contents, .contains("tests=\"5\" failures=\"3\""))
XCTAssertMatch(contents, .regex("time=\"[0-9]+\\.[0-9]+\""))
XCTAssertNoMatch(contents, .contains("time=\"0.0\""))
XCTAssertMatch(contents, .contains(#"message="ParallelTestsFailureTests.swift:7 failed - Giving up is the only sure way to fail."#))
XCTAssertMatch(contents, .contains(#"message="ParallelTestsFailureTests.swift:11 XCTAssertTrue failed - Expected assertion failure.""#))
#if os(Linux)
XCTAssertMatch(contents, .contains(#"message="ParallelTestsFailureTests.swift:16 Asynchronous wait failed - Exceeded timeout of 0.0 seconds, with unfulfilled expectations: failing expectation"#))
#else
XCTAssertMatch(contents, .contains(#"message="ParallelTestsFailureTests.swift:16 Asynchronous wait failed: Exceeded timeout of 0 seconds, with unfulfilled expectations: &quot;failing expectation&quot;."#))
#endif
}
}
}
Expand Down