Fix test discovery for Objective-C XCTests

ahoppen · ahoppen · commit aff3a97ff739 · 2024-04-24T08:54:42.000-07:00
There were two issues with Objective-C XCTest discovery:
1. We were relying on syntactic test discovery after a document is edited. But since we don't have syntactic test discovery for Objective-C tests, this meant that all tests would disappear as a document got edited. Until we get syntactic test discovery for Objective-C, use the semantic index to discover tests, even if they are out-of-date.
2. We were assuming that the `DocumentSymbols` request returned `[DocumentSymbol]` to find the ranges of tests. But clangd returns the alternative `[SymbolInformation]`, which meant that we were only returning the position of a test function’s name instead of the test function’s range.

rdar://126810202
diff --git a/Sources/SourceKitLSP/LanguageService.swift b/Sources/SourceKitLSP/LanguageService.swift
@@ -198,7 +198,9 @@ public protocol LanguageService: AnyObject {
   /// Perform a syntactic scan of the file at the given URI for test cases and test classes.
   ///
   /// This is used as a fallback to show the test cases in a file if the index for a given file is not up-to-date.
-  func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem]
+  ///
+  /// A return value of `nil` indicates that this language service does not support syntactic test discovery.
+  func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem]?
 
   /// Crash the language server. Should be used for crash recovery testing only.
   func _crash() async
diff --git a/Sources/SourceKitLSP/TestDiscovery.swift b/Sources/SourceKitLSP/TestDiscovery.swift
@@ -41,22 +41,26 @@ fileprivate extension SymbolOccurrence {
 /// Find the innermost range of a document symbol that contains the given position.
 private func findInnermostSymbolRange(
   containing position: Position,
-  documentSymbols documentSymbolsResponse: DocumentSymbolResponse
+  documentSymbolsResponse: DocumentSymbolResponse
 ) -> Range<Position>? {
-  guard case .documentSymbols(let documentSymbols) = documentSymbolsResponse else {
-    // Both `ClangLanguageService` and `SwiftLanguageService` return `documentSymbols` so we don't need to handle the
-    // .symbolInformation case.
-    logger.fault(
-      """
-      Expected documentSymbols response from language service to resolve test ranges but got \
-      \(documentSymbolsResponse.forLogging)
-      """
-    )
-    return nil
+  switch documentSymbolsResponse {
+  case .documentSymbols(let documentSymbols):
+    return findInnermostSymbolRange(containing: position, documentSymbols: documentSymbols)
+  case .symbolInformation(let symbolInformation):
+    return findInnermostSymbolRange(containing: position, symbolInformation: symbolInformation)
   }
+}
+
+private func findInnermostSymbolRange(
+  containing position: Position,
+  documentSymbols: [DocumentSymbol]
+) -> Range<Position>? {
   for documentSymbol in documentSymbols where documentSymbol.range.contains(position) {
     if let children = documentSymbol.children,
-      let rangeOfChild = findInnermostSymbolRange(containing: position, documentSymbols: .documentSymbols(children))
+      let rangeOfChild = findInnermostSymbolRange(
+        containing: position,
+        documentSymbolsResponse: .documentSymbols(children)
+      )
     {
       // If a child contains the position, prefer that because it's more specific.
       return rangeOfChild
@@ -66,6 +70,21 @@ private func findInnermostSymbolRange(
   return nil
 }
 
+/// Return the smallest range in `symbolInformation` containing `position`.
+private func findInnermostSymbolRange(
+  containing position: Position,
+  symbolInformation symbolInformationArray: [SymbolInformation]
+) -> Range<Position>? {
+  var bestRange: Range<Position>? = nil
+  for symbolInformation in symbolInformationArray where symbolInformation.location.range.contains(position) {
+    let range = symbolInformation.location.range
+    if bestRange == nil || (bestRange!.lowerBound < range.lowerBound && range.upperBound < bestRange!.upperBound) {
+      bestRange = range
+    }
+  }
+  return bestRange
+}
+
 extension SourceKitLSPServer {
   /// Converts a flat list of test symbol occurrences to a hierarchical `TestItem` array, inferring the hierarchical
   /// structure from `childOf` relations between the symbol occurrences.
@@ -263,9 +282,15 @@ extension SourceKitLSPServer {
 
     let syntacticTests = try await languageService.syntacticDocumentTests(for: req.textDocument.uri, in: workspace)
 
-    if let index = workspace.index(checkedFor: .imMemoryModifiedFiles(documentManager)) {
+    // We `syntacticDocumentTests` returns `nil`, it indicates that it doesn't support syntactic test discovery.
+    // In that case, the semantic index is the only source of tests we have and we thus want to show tests from the
+    // semantic index, even if they are out-of-date. The alternative would be showing now tests after an edit to a file.
+    let indexCheckLevel: IndexCheckLevel =
+      syntacticTests == nil ? .deletedFiles : .imMemoryModifiedFiles(documentManager)
+
+    if let index = workspace.index(checkedFor: indexCheckLevel) {
       var syntacticSwiftTestingTests: [TestItem] {
-        syntacticTests.filter { $0.style == TestStyle.swiftTesting }
+        syntacticTests?.filter { $0.style == TestStyle.swiftTesting } ?? []
       }
 
       let testSymbols =
@@ -283,7 +308,7 @@ extension SourceKitLSPServer {
           for: testSymbols,
           resolveLocation: { uri, position in
             if uri == snapshot.uri, let documentSymbols,
-              let range = findInnermostSymbolRange(containing: position, documentSymbols: documentSymbols)
+              let range = findInnermostSymbolRange(containing: position, documentSymbolsResponse: documentSymbols)
             {
               return Location(uri: uri, range: range)
             }
@@ -298,7 +323,7 @@ extension SourceKitLSPServer {
       }
     }
     // We don't have any up-to-date semantic index entries for this file. Syntactically look for tests.
-    return syntacticTests
+    return syntacticTests ?? []
   }
 }
 
@@ -437,7 +462,7 @@ extension TestItem {
 }
 
 extension SwiftLanguageService {
-  public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem] {
+  public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async throws -> [TestItem]? {
     let snapshot = try documentManager.latestSnapshot(uri)
     let semanticSymbols = workspace.index(checkedFor: .deletedFiles)?.symbols(inFilePath: snapshot.uri.pseudoPath)
     let xctestSymbols = await SyntacticSwiftXCTestScanner.findTestSymbols(
@@ -453,7 +478,7 @@ extension SwiftLanguageService {
 }
 
 extension ClangLanguageService {
-  public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async -> [TestItem] {
-    return []
+  public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async -> [TestItem]? {
+    return nil
   }
 }
diff --git a/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift b/Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift
@@ -913,4 +913,135 @@ final class DocumentTestDiscoveryTests: XCTestCase {
     XCTAssertFalse(testsAfterEdit.contains { $0.label == "NotQuiteTest" })
     XCTAssertTrue(testsAfterEdit.contains { $0.label == "OtherNotQuiteTest" })
   }
+
+  func testObjectiveCTestFromSemanticIndex() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Tests/MyLibraryTests/Test.m": """
+        #import <XCTest/XCTest.h>
+
+        @interface MyTests : XCTestCase
+        @end
+
+        1️⃣@implementation MyTests
+        2️⃣- (void)testSomething {
+        }3️⃣
+        @4️⃣end
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.testTarget(name: "MyLibraryTests")]
+        )
+        """,
+      build: true
+    )
+
+    let (uri, positions) = try project.openDocument("Test.m")
+
+    let tests = try await project.testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri)))
+
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MyTests",
+          label: "MyTests",
+          disabled: false,
+          style: TestStyle.xcTest,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["4️⃣"]),
+          children: [
+            TestItem(
+              id: "MyTests/testSomething",
+              label: "testSomething",
+              disabled: false,
+              style: TestStyle.xcTest,
+              location: Location(uri: uri, range: positions["2️⃣"]..<positions["3️⃣"]),
+              children: [],
+              tags: []
+            )
+          ],
+          tags: []
+        )
+      ]
+    )
+  }
+
+  func testObjectiveCTestsAfterInMemoryEdit() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Tests/MyLibraryTests/Test.m": """
+        #import <XCTest/XCTest.h>
+
+        @interface MyTests : XCTestCase
+        @end
+
+        1️⃣@implementation MyTests
+        2️⃣- (void)testSomething {}3️⃣
+        0️⃣
+        @4️⃣end
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.testTarget(name: "MyLibraryTests")]
+        )
+        """,
+      build: true
+    )
+
+    let (uri, positions) = try project.openDocument("Test.m")
+
+    project.testClient.send(
+      DidChangeTextDocumentNotification(
+        textDocument: VersionedTextDocumentIdentifier(uri, version: 2),
+        contentChanges: [
+          TextDocumentContentChangeEvent(
+            range: Range(positions["0️⃣"]),
+            text: """
+              - (void)testSomethingElse {}
+              """
+          )
+        ]
+      )
+    )
+
+    let tests = try await project.testClient.send(DocumentTestsRequest(textDocument: TextDocumentIdentifier(uri)))
+    // Since we don't have syntactic test discovery for clang-languages, we don't discover `testSomethingElse` as a
+    // test method until we perform a build
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MyTests",
+          label: "MyTests",
+          disabled: false,
+          style: TestStyle.xcTest,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["4️⃣"]),
+          children: [
+            TestItem(
+              id: "MyTests/testSomething",
+              label: "testSomething",
+              disabled: false,
+              style: TestStyle.xcTest,
+              location: Location(uri: uri, range: positions["2️⃣"]..<positions["3️⃣"]),
+              children: [],
+              tags: []
+            )
+          ],
+          tags: []
+        )
+      ]
+    )
+  }
 }
diff --git a/Tests/SourceKitLSPTests/WorkspaceTestDiscoveryTests.swift b/Tests/SourceKitLSPTests/WorkspaceTestDiscoveryTests.swift
@@ -846,4 +846,133 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
     XCTAssertFalse(testsAfterEdit.contains { $0.label == "NotQuiteTest" })
     XCTAssertTrue(testsAfterEdit.contains { $0.label == "OtherNotQuiteTest" })
   }
+
+  func testObjectiveCTestFromSemanticIndex() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Tests/MyLibraryTests/Test.m": """
+        #import <XCTest/XCTest.h>
+
+        @interface MyTests : XCTestCase
+        @end
+
+        @implementation 1️⃣MyTests
+        - (void)2️⃣testSomething {
+        }
+        @end
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.testTarget(name: "MyLibraryTests")]
+        )
+        """,
+      build: true
+    )
+
+    let tests = try await project.testClient.send(WorkspaceTestsRequest())
+
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MyTests",
+          label: "MyTests",
+          disabled: false,
+          style: TestStyle.xcTest,
+          location: try project.location(from: "1️⃣", to: "1️⃣", in: "Test.m"),
+          children: [
+            TestItem(
+              id: "MyTests/testSomething",
+              label: "testSomething",
+              disabled: false,
+              style: TestStyle.xcTest,
+              location: try project.location(from: "2️⃣", to: "2️⃣", in: "Test.m"),
+              children: [],
+              tags: []
+            )
+          ],
+          tags: []
+        )
+      ]
+    )
+  }
+
+  func testObjectiveCTestsAfterInMemoryEdit() async throws {
+    let project = try await SwiftPMTestProject(
+      files: [
+        "Tests/MyLibraryTests/Test.m": """
+        #import <XCTest/XCTest.h>
+
+        @interface MyTests : XCTestCase
+        @end
+
+        1️⃣@implementation MyTests
+        2️⃣- (void)testSomething {}3️⃣
+        0️⃣
+        @4️⃣end
+        """
+      ],
+      manifest: """
+        // swift-tools-version: 5.7
+
+        import PackageDescription
+
+        let package = Package(
+          name: "MyLibrary",
+          targets: [.testTarget(name: "MyLibraryTests")]
+        )
+        """,
+      build: true
+    )
+
+    let (uri, positions) = try project.openDocument("Test.m")
+
+    project.testClient.send(
+      DidChangeTextDocumentNotification(
+        textDocument: VersionedTextDocumentIdentifier(uri, version: 2),
+        contentChanges: [
+          TextDocumentContentChangeEvent(
+            range: Range(positions["0️⃣"]),
+            text: """
+              - (void)testSomethingElse {}
+              """
+          )
+        ]
+      )
+    )
+
+    let tests = try await project.testClient.send(WorkspaceTestsRequest())
+    // Since we don't have syntactic test discovery for clang-languages, we don't discover `testSomethingElse` as a
+    // test method until we perform a build
+    XCTAssertEqual(
+      tests,
+      [
+        TestItem(
+          id: "MyTests",
+          label: "MyTests",
+          disabled: false,
+          style: TestStyle.xcTest,
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["4️⃣"]),
+          children: [
+            TestItem(
+              id: "MyTests/testSomething",
+              label: "testSomething",
+              disabled: false,
+              style: TestStyle.xcTest,
+              location: Location(uri: uri, range: positions["2️⃣"]..<positions["3️⃣"]),
+              children: [],
+              tags: []
+            )
+          ],
+          tags: []
+        )
+      ]
+    )
+  }
 }
