Merge pull request #1662 from plemarquand/fully-specified-test-ids

plemarquand · web-flow · commit d09caf52ccac · 2024-09-23T16:11:16.000-04:00
Fully specify test IDs if they are not unique
diff --git a/Sources/SourceKitLSP/TestDiscovery.swift b/Sources/SourceKitLSP/TestDiscovery.swift
@@ -278,6 +278,7 @@ extension SourceKitLSPServer {
       .flatMap { $0 }
       .sorted { $0.testItem.location < $1.testItem.location }
       .mergingTestsInExtensions()
+      .deduplicatingIds()
   }
 
   func documentTests(
@@ -288,6 +289,7 @@ extension SourceKitLSPServer {
     return try await documentTestsWithoutMergingExtensions(req, workspace: workspace, languageService: languageService)
       .prefixTestsWithModuleName(workspace: workspace)
       .mergingTestsInExtensions()
+      .deduplicatingIds()
   }
 
   private func documentTestsWithoutMergingExtensions(
@@ -452,7 +454,14 @@ fileprivate extension Array<AnnotatedTestItem> {
           rootItem.testItem.children += item.testItem.children
         }
 
-        itemDict[id] = rootItem
+        // If this item shares an ID with a sibling and both are leaf
+        // test items, store it by its disambiguated id to ensure we
+        // don't overwrite the existing element.
+        if rootItem.testItem.children.isEmpty && item.testItem.children.isEmpty {
+          itemDict[item.testItem.ambiguousTestDifferentiator] = item
+        } else {
+          itemDict[id] = rootItem
+        }
       } else {
         itemDict[id] = item
       }
@@ -502,7 +511,55 @@ fileprivate extension Array<AnnotatedTestItem> {
   }
 }
 
+fileprivate extension Array<TestItem> {
+  /// If multiple testItems share the same ID we add more context to make it unique.
+  /// Two tests can share the same ID when two swift testing tests accept
+  /// arguments of different types, i.e:
+  /// ```
+  /// @Test(arguments: [1,2,3]) func foo(_ x: Int) {}
+  /// @Test(arguments: ["a", "b", "c"]) func foo(_ x: String) {}
+  /// ```
+  ///
+  /// or when tests are in separate files but don't conflict because they are marked
+  /// private, i.e:
+  /// ```
+  /// File1.swift: @Test private func foo() {}
+  /// File2.swift: @Test private func foo() {}
+  /// ```
+  ///
+  /// If we encounter one of these cases, we need to deduplicate the ID
+  /// by appending `/filename:filename:lineNumber`.
+  func deduplicatingIds() -> [TestItem] {
+    var idCounts: [String: Int] = [:]
+    for element in self where element.children.isEmpty {
+      idCounts[element.id, default: 0] += 1
+    }
+
+    return self.map {
+      var newItem = $0
+      newItem.children = newItem.children.deduplicatingIds()
+      if idCounts[newItem.id, default: 0] > 1 {
+        newItem.id = newItem.ambiguousTestDifferentiator
+      }
+      return newItem
+    }
+  }
+}
+
 extension TestItem {
+  /// A fully qualified name to disambiguate identical TestItem IDs.
+  /// This matches the IDs produced by `swift test list` when there are
+  /// tests that cannot be disambiguated by their simple ID.
+  fileprivate var ambiguousTestDifferentiator: String {
+    let filename = self.location.uri.arbitrarySchemeURL.lastPathComponent
+    let position = location.range.lowerBound
+    // Lines and columns start at 1.
+    // swift-testing tests start from _after_ the @ symbol in @Test, so we need to add an extra column.
+    // see https://github.com/swiftlang/swift-testing/blob/cca6de2be617aded98ecdecb0b3b3a81eec013f3/Sources/TestingMacros/Support/AttributeDiscovery.swift#L153
+    let columnOffset = self.style == TestStyle.swiftTesting ? 2 : 1
+    return "\(self.id)/\(filename):\(position.line + 1):\(position.utf16index + columnOffset)"
+  }
+
   fileprivate func prefixIDWithModuleName(workspace: Workspace) async -> TestItem {
     guard let canonicalTarget = await workspace.buildSystemManager.canonicalTarget(for: self.location.uri),
       let moduleName = await workspace.buildSystemManager.moduleName(for: self.location.uri, in: canonicalTarget)
diff --git a/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift b/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift
@@ -418,6 +418,139 @@ final class DocumentTestDiscoveryTests: XCTestCase {
     )
   }
 
+  func testSwiftTestingTestsWithDuplicateFunctionIdentifiers() async throws {
+    let testClient = try await TestSourceKitLSPClient()
+    let uri = DocumentURI(for: .swift)
+
+    let positions = testClient.openDocument(
+      """
+      import Testing
+
+      1️⃣@Test(arguments: [1, 2, 3])
+      func foo(_ x: Int) {}2️⃣
+      3️⃣@Test(arguments: ["a", "b", "c"])
+      func foo(_ x: String) {}4️⃣
+      """,
+      uri: uri
+    )
+
+    let filename = uri.fileURL?.lastPathComponent ?? ""
+    let tests = try await testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri)))
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "foo(_:)/\(filename):\(positions["1️⃣"].line + 1):\(positions["1️⃣"].utf16index + 2)",
+          label: "foo(_:)",
+          style: TestStyle.swiftTesting,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["2️⃣"])
+        ),
+        TestItem(
+          id: "foo(_:)/\(filename):\(positions["3️⃣"].line + 1):\(positions["3️⃣"].utf16index + 2)",
+          label: "foo(_:)",
+          style: TestStyle.swiftTesting,
+          location: Location(uri: uri, range: positions["3️⃣"]..<positions["4️⃣"])
+        ),
+      ]
+    )
+  }
+
+  func testSwiftTestingTestsWithDuplicateFunctionIdentifiersInSuite() async throws {
+    let testClient = try await TestSourceKitLSPClient()
+    let uri = DocumentURI(for: .swift)
+
+    let positions = testClient.openDocument(
+      """
+      import Testing
+
+      1️⃣struct MySuite {
+        3️⃣@Test(arguments: [1, 2, 3])
+        func foo(_ x: Int) {}4️⃣
+        5️⃣@Test(arguments: ["a", "b", "c"])
+        func foo(_ x: String) {}6️⃣
+      }2️⃣
+      """,
+      uri: uri
+    )
+
+    let filename = uri.fileURL?.lastPathComponent ?? ""
+    let tests = try await testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri)))
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MySuite",
+          label: "MySuite",
+          style: TestStyle.swiftTesting,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["2️⃣"]),
+          children: [
+            TestItem(
+              id: "MySuite/foo(_:)/\(filename):\(positions["3️⃣"].line + 1):\(positions["3️⃣"].utf16index + 2)",
+              label: "foo(_:)",
+              style: TestStyle.swiftTesting,
+              location: Location(uri: uri, range: positions["3️⃣"]..<positions["4️⃣"])
+            ),
+            TestItem(
+              id: "MySuite/foo(_:)/\(filename):\(positions["5️⃣"].line + 1):\(positions["5️⃣"].utf16index + 2)",
+              label: "foo(_:)",
+              style: TestStyle.swiftTesting,
+              location: Location(uri: uri, range: positions["5️⃣"]..<positions["6️⃣"])
+            ),
+          ]
+        )
+      ]
+    )
+  }
+
+  func testSwiftTestingTestsWithDuplicateFunctionIdentifiersInExtension() async throws {
+    let testClient = try await TestSourceKitLSPClient()
+    let uri = DocumentURI(for: .swift)
+
+    let positions = testClient.openDocument(
+      """
+      import Testing
+      1️⃣struct MySuite {
+        3️⃣@Test(arguments: [1, 2, 3])
+        func foo(_ x: Int) {}4️⃣
+      }2️⃣
+
+      extension MySuite {
+        5️⃣@Test(arguments: ["a", "b", "c"])
+        func foo(_ x: String) {}6️⃣
+      }
+      """,
+      uri: uri
+    )
+
+    let filename = uri.fileURL?.lastPathComponent ?? ""
+    let tests = try await testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri)))
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MySuite",
+          label: "MySuite",
+          style: TestStyle.swiftTesting,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["2️⃣"]),
+          children: [
+            TestItem(
+              id: "MySuite/foo(_:)/\(filename):\(positions["3️⃣"].line + 1):\(positions["3️⃣"].utf16index + 2)",
+              label: "foo(_:)",
+              style: TestStyle.swiftTesting,
+              location: Location(uri: uri, range: positions["3️⃣"]..<positions["4️⃣"])
+            ),
+            TestItem(
+              id: "MySuite/foo(_:)/\(filename):\(positions["5️⃣"].line + 1):\(positions["5️⃣"].utf16index + 2)",
+              label: "foo(_:)",
+              style: TestStyle.swiftTesting,
+              location: Location(uri: uri, range: positions["5️⃣"]..<positions["6️⃣"])
+            ),
+          ]
+        )
+      ]
+    )
+  }
+
   func testSwiftTestingSuiteWithNoTests() async throws {
     let testClient = try await TestSourceKitLSPClient()
     let uri = DocumentURI(for: .swift)
diff --git a/Tests/SourceKitLSPTests/WorkspaceTestDiscoveryTests.swift b/Tests/SourceKitLSPTests/WorkspaceTestDiscoveryTests.swift
@@ -245,6 +245,49 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
     )
   }
 
+  func testSwiftTestingTestsWithDuplicateFunctionIdentifiersAcrossDocuments() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Tests/MyLibraryTests/MyTests1.swift": """
+        import Testing
+
+        1️⃣@Test(arguments: [1, 2, 3])
+        private func foo(_ x: Int) {}2️⃣
+        """,
+        "Tests/MyLibraryTests/MyTests2.swift": """
+        import Testing
+
+        3️⃣@Test(arguments: [1, 2, 3])
+        private func foo(_ x: Int) {}4️⃣
+        """,
+      ],
+      manifest: packageManifestWithTestTarget
+    )
+
+    let test1Position = try project.position(of: "1️⃣", in: "MyTests1.swift")
+    let test2Position = try project.position(of: "3️⃣", in: "MyTests2.swift")
+
+    let tests = try await project.testClient.send(WorkspaceTestsRequest())
+
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MyLibraryTests.foo(_:)/MyTests1.swift:\(test1Position.line + 1):\(test1Position.utf16index + 2)",
+          label: "foo(_:)",
+          style: TestStyle.swiftTesting,
+          location: try project.location(from: "1️⃣", to: "2️⃣", in: "MyTests1.swift")
+        ),
+        TestItem(
+          id: "MyLibraryTests.foo(_:)/MyTests2.swift:\(test2Position.line + 1):\(test2Position.utf16index + 2)",
+          label: "foo(_:)",
+          style: TestStyle.swiftTesting,
+          location: try project.location(from: "3️⃣", to: "4️⃣", in: "MyTests2.swift")
+        ),
+      ]
+    )
+  }
+
   func testSwiftTestingAndXCTestInTheSameFile() async throws {
     try SkipUnless.longTestsEnabled()
 
