Skip to content

Commit

Permalink
Pull the XCTest runner(s) and test sharding/filtering logic out of th…
Browse files Browse the repository at this point in the history
…e generator and just make them regular Swift classes in the observer module.

There's no compelling reason to generate all of this code; the only thing that actually needs to be generated is the list of tests for Linux, and the small `main` that invokes it. For simplicity, we generate it on Darwin platforms as well, even though it's not strictly necessary because all the discovery there is dynamic.

This change also removes the detached `Task` hack that I introduced previously. This worked when all the tests pass (the common case), but if a test failed, XCTest complained that the `recordIssue` method wasn't being called on the main thread. I'll revisit this in a subsequent change to support swift-testing.

PiperOrigin-RevId: 664761141
  • Loading branch information
allevato authored and swiple-rules-gardener committed Aug 19, 2024
1 parent d7f555d commit 6d6727a
Show file tree
Hide file tree
Showing 9 changed files with 366 additions and 279 deletions.
1 change: 0 additions & 1 deletion tools/test_discoverer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ swift_binary(
name = "test_discoverer",
srcs = [
"DiscoveredTests.swift",
"ObjcTestPrinter.swift",
"SymbolCollector.swift",
"SymbolGraphTestPrinter.swift",
"SymbolKitExtensions.swift",
Expand Down
131 changes: 0 additions & 131 deletions tools/test_discoverer/ObjcTestPrinter.swift

This file was deleted.

87 changes: 10 additions & 77 deletions tools/test_discoverer/SymbolGraphTestPrinter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -99,20 +99,20 @@ struct SymbolGraphTestPrinter {
contents += """
\(availabilityAttribute)
func collect\(allTestsIdentifier(for: discoveredModule))(into collector: inout ShardingFilteringTestCollector) {
let \(allTestsIdentifier(for: discoveredModule)) = [
"""

for className in sortedClassNames {
let testClass = discoveredModule.classes[className]!
contents += """
collector.addTests("\(className)", \(className).\(allTestsIdentifier(for: testClass)))
testCase(\(className).\(allTestsIdentifier(for: testClass))),
"""
}

contents += """
}
]
"""

Expand All @@ -127,96 +127,29 @@ struct SymbolGraphTestPrinter {
// harmlessly compiled as an empty module, and the user's `main` from their own sources will
// be used instead.
return """
@MainActor
struct XCTestRunner {
static func run() {
// No XCTest-based tests discovered; this is intentionally empty.
}
}
private let __allDiscoveredXCTests: [XCTestCaseEntry] = []
"""
}

var contents = """
import BazelTestObservation
import Foundation
import XCTest
\(availabilityAttribute)
@MainActor
struct XCTestRunner {
static func run() throws {
XCTestObservationCenter.shared.addTestObserver(BazelXMLTestObserver.default)
var testCollector = try ShardingFilteringTestCollector()
private let __allDiscoveredXCTests: [XCTestCaseEntry] = {
var allTests: [XCTestCaseEntry] = []
"""

for moduleName in discoveredTests.modules.keys.sorted() {
let module = discoveredTests.modules[moduleName]!
contents += """
collect\(allTestsIdentifier(for: module))(into: &testCollector)
allTests.append(contentsOf: \(allTestsIdentifier(for: module)))
"""
}

// We don't pass the test filter as an argument because we've already filtered the tests in the
// collector; this lets us do better filtering (i.e., regexes) than XCTest itself allows.
contents += """
// The preferred overload is one that calls `exit`, which we don't want because we have
// post-work to do, so force the one that returns an exit code instead.
let _: CInt = XCTMain(testCollector.testsToRun)
}
}
"""

contents += createShardingFilteringTestCollector(
extraProperties: "private(set) var testsToRun: [XCTestCaseEntry] = []\n")
contents += """
extension ShardingFilteringTestCollector {
mutating func addTests<T: XCTestCase>(
_ suiteName: String,
_ tests: [(String, (T) -> () -> Void)]
) {
guard shardCount != 0 || filter != nil else {
// If we're not sharding or filtering, just add all the tests.
testsToRun.append(testCase(tests))
return
}
var shardTests: [(String, (T) -> () -> Void)] = []
for test in tests {
guard isIncludedByFilter("\\(suiteName)/\\(test.0)") else {
continue
}
if isIncludedInShard() {
shardTests.append(test)
}
seenTestCount += 1
}
testsToRun.append(testCase(shardTests))
}
mutating func addTests<T: XCTestCase>(
_ suiteName: String,
_ tests: [(String, (T) -> () throws -> Void)]
) {
guard shardCount != 0 || filter != nil else {
// If we're not sharding or filtering, just add all the tests.
testsToRun.append(testCase(tests))
return
}
var shardTests: [(String, (T) -> () throws -> Void)] = []
for test in tests {
guard isIncludedByFilter("\\(suiteName)/\\(test.0)") else {
continue
}
if isIncludedInShard() {
shardTests.append(test)
}
seenTestCount += 1
}
testsToRun.append(testCase(shardTests))
}
}
return allTests
}()
"""

Expand Down
17 changes: 12 additions & 5 deletions tools/test_discoverer/TestDiscoverer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,15 @@ struct TestDiscoverer: ParsableCommand {

var contents = """
import BazelTestObservation
import Foundation
import XCTest
\(availabilityAttribute)
@main
struct Main {
static func main() async {
static func main() {
do {
try await XCTestRunner.run()
try XCTestRunner.run(__allDiscoveredXCTests)
try XUnitTestRecorder.shared.writeXML()
guard !XUnitTestRecorder.shared.hasFailure else {
Expand All @@ -128,9 +130,14 @@ struct TestDiscoverer: ParsableCommand {

let mainFileURL = URL(fileURLWithPath: mainOutput)
if objcTestDiscovery {
// Print the runner source file, which implements the `@main` type that executes the tests.
let testPrinter = ObjcTestPrinter()
contents.append(testPrinter.testRunnerSource())
// On Darwin platforms, tests are discovered by the Objective-C runtime, so we don't need to
// generate anything. We use a dummy parameter to keep the call site the same on both
// platforms.
contents.append("""
// Unused by the Objective-C XCTestRunner; tests are discovered by the runtime.
private let __allDiscoveredXCTests: () = ()
""")
} else {
// For each module, print the list of test entries that were discovered in a source file that
// extends that module.
Expand Down
65 changes: 0 additions & 65 deletions tools/test_discoverer/TestPrinterCommon.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,68 +23,3 @@ let availabilityAttribute = """
func createTextFile(at url: URL, contents: String) {
FileManager.default.createFile(atPath: url.path, contents: contents.data(using: .utf8)!)
}

/// The parts of the `ShardingAwareTestCollector` type that are common to both the Objective-C and
/// symbol-graph-based implementations. Those generators then should add extensions to provide
/// runner-specific logic.
func createShardingFilteringTestCollector(extraProperties: String = "") -> String {
return """
struct ShardingFilteringTestCollector {
struct Error: Swift.Error, CustomStringConvertible {
var message: String
var description: String { message }
}
private var shardCount: Int
private var shardIndex: Int
private var seenTestCount: Int
private var filter: Regex<AnyRegexOutput>?
\(extraProperties)
init() throws {
// Bazel requires us to write out an empty file at this path to tell it that we support
// sharding.
if let statusPath = ProcessInfo.processInfo.environment["TEST_SHARD_STATUS_FILE"] {
guard FileManager.default.createFile(atPath: statusPath, contents: nil, attributes: nil)
else {
throw Error(message: "Could not create TEST_SHARD_STATUS_FILE (\\(statusPath))")
}
}
self.shardCount =
ProcessInfo.processInfo.environment["TEST_TOTAL_SHARDS"].flatMap { Int($0, radix: 10) } ?? 0
self.shardIndex =
ProcessInfo.processInfo.environment["TEST_SHARD_INDEX"].flatMap { Int($0, radix: 10) } ?? 0
self.seenTestCount = 0
guard (shardCount == 0 && shardIndex == 0) || (shardCount > 0 && shardIndex < shardCount)
else {
throw Error(
message: "Invalid shard count (\\(shardCount)) and shard index (\\(shardIndex))")
}
if let filterString = ProcessInfo.processInfo.environment["TESTBRIDGE_TEST_ONLY"] {
guard let maybeFilter = try? Regex(filterString) else {
throw Error(message: "Could not parse '--test_filter' string as a regular expression")
}
filter = maybeFilter
} else {
filter = nil
}
}
private func isIncludedByFilter(_ testName: String) -> Bool {
guard let filter = self.filter else { return true }
do {
return try filter.firstMatch(in: testName) != nil
} catch {
return false
}
}
private func isIncludedInShard() -> Bool {
return shardCount == 0 || seenTestCount % shardCount == shardIndex
}
}
"""
}
3 changes: 3 additions & 0 deletions tools/test_observer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ swift_library(
testonly = 1,
srcs = [
"BazelXMLTestObserver.swift",
"LinuxXCTestRunner.swift",
"Locked.swift",
"ObjectiveCXCTestRunner.swift",
"ShardingFilteringTestCollector.swift",
"StringInterpolation+XMLEscaping.swift",
"XUnitTestRecorder.swift",
],
Expand Down
Loading

1 comment on commit 6d6727a

@brentleyjones
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please sign in to comment.