From 7bcbae4e90ed7baa950e09db8cbf83a35f3a4c8f Mon Sep 17 00:00:00 2001
From: Paul LeMarquand <plemarquand@apple.com>
Date: Thu, 12 Sep 2024 09:33:06 -0400
Subject: [PATCH] Fully specify test IDs if they are not unique

It is possible for swift-testing test IDs to be identical in certain
circumstances. For instance if there are two parameterized tests where
only the type of the parameter differs. Another example is having two
identical @Test definitions marked private in two different files in the
same test target.

To overcome this we do the same thing that SwiftPM does when running
`swift test list` in this situation, which is to fully qualify the
duplicate test IDs by appending their filename:line:column.

Issue: #1661
---
 Sources/SourceKitLSP/TestDiscovery.swift      | 41 ++++++++++++++++--
 .../DocumentTestDiscoveryTests.swift          | 37 ++++++++++++++++
 .../WorkspaceTestDiscoveryTests.swift         | 43 +++++++++++++++++++
 3 files changed, 118 insertions(+), 3 deletions(-)

diff --git a/Sources/SourceKitLSP/TestDiscovery.swift b/Sources/SourceKitLSP/TestDiscovery.swift
index ef9ff3dc..2cdf638a 100644
--- a/Sources/SourceKitLSP/TestDiscovery.swift
+++ b/Sources/SourceKitLSP/TestDiscovery.swift
@@ -433,6 +433,7 @@ fileprivate extension Array<AnnotatedTestItem> {
   /// A node's parent is identified by the node's ID with the last component dropped.
   func mergingTestsInExtensions() -> [TestItem] {
     var itemDict: [String: AnnotatedTestItem] = [:]
+    var duplicatedIds: Set<String> = []
     for item in self {
       let id = item.testItem.id
       if var rootItem = itemDict[id] {
@@ -443,6 +444,10 @@ fileprivate extension Array<AnnotatedTestItem> {
           var newItem = item
           newItem.testItem.children += rootItem.testItem.children
           rootItem = newItem
+        } else if (rootItem.testItem.children.isEmpty && item.testItem.children.isEmpty) {
+          duplicatedIds.insert(item.testItem.id)
+          itemDict[item.testItem.fullyQualifiedTestId] = item
+          continue
         } else {
           rootItem.testItem.children += item.testItem.children
         }
@@ -475,10 +480,26 @@ fileprivate extension Array<AnnotatedTestItem> {
       .sorted { ($0.isExtension != $1.isExtension) ? !$0.isExtension : ($0.testItem.location < $1.testItem.location) }
 
     let result = sortedItems.map {
-      guard !$0.testItem.children.isEmpty, mergedIds.contains($0.testItem.id) else {
-        return $0.testItem
-      }
       var newItem = $0.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.
+      if duplicatedIds.contains(newItem.id) {
+        newItem.id = newItem.fullyQualifiedTestId
+      }
+
+      guard !$0.testItem.children.isEmpty, mergedIds.contains($0.testItem.id) else {
+        return newItem
+      }
       newItem.children = newItem.children
         .map { AnnotatedTestItem(testItem: $0, isExtension: false) }
         .mergingTestsInExtensions()
@@ -498,6 +519,20 @@ fileprivate extension Array<AnnotatedTestItem> {
 }
 
 extension TestItem {
+  fileprivate var fullyQualifiedTestId: String {
+    return "\(self.id)/\(self.sourceLocation)"
+  }
+
+  private var sourceLocation: 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 "\(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
index 9f89290a..8ba285a8 100644
--- a/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift
+++ b/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift
@@ -418,6 +418,43 @@ 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 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
index 6126006f..0d1acbc9 100644
--- 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()