Skip to content

[5.5] fix a bug in test discovery where tests define in extensions break the discovery #3435

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

Merged
merged 1 commit into from
Apr 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions Fixtures/Miscellaneous/TestDiscovery/Extensions/Package.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// swift-tools-version:4.2
import PackageDescription

let package = Package(
name: "Simple",
targets: [
.target(name: "Simple"),
.testTarget(name: "SimpleTests", dependencies: ["Simple"]),
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
struct Simple {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import XCTest
@testable import Simple

class SimpleTests1: XCTestCase {
func testExample1() {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import XCTest
@testable import Simple

class SimpleTests2: XCTestCase {
func testExample2() {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import XCTest
@testable import Simple

extension SimpleTests1 {
func testExample1_a() {
}
}

extension SimpleTests2 {
func testExample2_a() {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import XCTest
@testable import Simple

class SimpleTests4: XCTestCase {
func testExample() {
}

func testExample1() {
}

func testExample2() {
}
}
16 changes: 10 additions & 6 deletions Sources/Build/BuildOperationBuildSystemDelegateHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@ final class TestDiscoveryCommand: CustomLLBuildCommand {
) throws {
let stream = try LocalFileOutputByteStream(path)

let testsByClass = Dictionary(grouping: tests, by: { $0.name })

stream <<< "import XCTest" <<< "\n"
stream <<< "@testable import " <<< module <<< "\n"

for klass in tests {
for classTests in testsByClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is now iterating over a dictionary instead of an array, should the keys be sorted here to avoid spurious differences in how the output file is emitted?

Copy link
Contributor Author

@tomerd tomerd Apr 23, 2021

Choose a reason for hiding this comment

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

do you mean so that the output won't cause unnecessary compiles? if so, I think this is a good idea but I would do that in a separate optimization PR on main, and let the fix through as-is in 5.5

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant — just to always have stable outputs. Agreed that it can be done separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let className = classTests.key
let testMethods = classTests.value.flatMap{ $0.methods }
stream <<< "\n"
stream <<< "fileprivate extension " <<< klass.name <<< " {" <<< "\n"
stream <<< indent(4) <<< "static let __allTests__\(klass.name) = [" <<< "\n"
for method in klass.methods {
stream <<< "fileprivate extension " <<< className <<< " {" <<< "\n"
stream <<< indent(4) <<< "static let __allTests__\(className) = [" <<< "\n"
for method in testMethods {
let method = method.hasSuffix("()") ? String(method.dropLast(2)) : method
stream <<< indent(8) <<< "(\"\(method)\", \(method))," <<< "\n"
}
Expand All @@ -74,8 +78,8 @@ final class TestDiscoveryCommand: CustomLLBuildCommand {
return [\n
"""

for klass in tests {
stream <<< indent(8) <<< "testCase(\(klass.name).__allTests__\(klass.name)),\n"
for className in testsByClass.keys {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about order.

stream <<< indent(8) <<< "testCase(\(className).__allTests__\(className)),\n"
}

stream <<< """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,23 @@ class TestDiscoveryTests: XCTestCase {
}
#endif
}

func testTestExtensions() throws {
#if os(macOS)
try XCTSkipIf(true)
#else
fixture(name: "Miscellaneous/TestDiscovery/Extensions") { path in
let (stdout, _) = try executeSwiftTest(path, extraArgs: ["--enable-test-discovery"])
XCTAssertTrue(stdout.contains("Merging module Simple"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests1.testExample1"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests1.testExample1_a"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests2.testExample2"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests2.testExample2_a"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests4.testExample"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests4.testExample1"), stdout)
XCTAssertTrue(stdout.contains("SimpleTests4.testExample2"), stdout)
XCTAssertTrue(stdout.contains("Executed 7 tests"), stdout)
}
#endif
}
}