Enhance syntactic test discovery with information from the semantic index

When the semantic index is out-of-date, we currently purely rely on the syntactic index to discover tests and completely ignore data from the semantic index. This may lead to confusing behavior. For example if you have

```
class MightInheritFromXCTestCaseOrNot {}

class MyClass: MightInheritFromXCTestCaseOrNot {
  func testStuff() {}
}
```

Then we don’t return any tests when the semantic index is up-to-date. But once the file is modified (either on disk or in-memory), we purely rely on the syntactic index, which reports `testStuff` as a test method. After a build / background indexing finishes, the test method disappears again.

We can mitigate this problem as follows: If we have stale semantic index data for the test file, for every test method found by the syntactic index, check if we have an entry for this method in the semantic index. If we do, but that entry is not marked as a test class/method, we know that the semantic index knows about this method but decided that it’s not a test method for some reason. So we should ignore it.

rdar://126492948

Author: ahoppen

Sources/LanguageServerProtocol/SupportTypes/TestItem.swift

@@ -12,7 +12,7 @@
 
 public struct TestTag: Codable, Equatable, Sendable {
   /// ID of the test tag. `TestTag` instances with the same ID are considered to be identical.
-  public let id: String
+  public var id: String
 
   public init(id: String) {
     self.id = id
@@ -26,35 +26,35 @@ public struct TestItem: ResponseType, Equatable {
   /// Identifier for the `TestItem`.
   ///
   /// This identifier uniquely identifies the test case or test suite. It can be used to run an individual test (suite).
-  public let id: String
+  public var id: String
 
   /// Display name describing the test.
-  public let label: String
+  public var label: String
 
   /// Optional description that appears next to the label.
-  public let description: String?
+  public var description: String?
 
   /// A string that should be used when comparing this item with other items.
   ///
   /// When `nil` the `label` is used.
-  public let sortText: String?
+  public var sortText: String?
 
   /// Whether the test is disabled.
-  public let disabled: Bool
+  public var disabled: Bool
 
   /// The type of test, eg. the testing framework that was used to declare the test.
-  public let style: String
+  public var style: String
 
   /// The location of the test item in the source code.
-  public let location: Location
+  public var location: Location
 
   /// The children of this test item.
   ///
   /// For a test suite, this may contain the individual test cases or nested suites.
-  public let children: [TestItem]
+  public var children: [TestItem]
 
   /// Tags associated with this test item.
-  public let tags: [TestTag]
+  public var tags: [TestTag]
 
   public init(
     id: String,

Sources/SourceKitLSP/CheckedIndex.swift

@@ -95,6 +95,13 @@ public class CheckedIndex {
     return index.symbolProvider(for: sourceFilePath)
   }
 
+  public func symbols(inFilePath path: String) -> [Symbol] {
+    guard self.hasUpToDateUnit(for: URL(fileURLWithPath: path, isDirectory: false)) else {
+      return []
+    }
+    return index.symbols(inFilePath: path)
+  }
+
   /// Returns all unit test symbol in unit files that reference one of the main files in `mainFilePaths`.
   public func unitTests(referencedByMainFiles mainFilePaths: [String]) -> [SymbolOccurrence] {
     return index.unitTests(referencedByMainFiles: mainFilePaths).filter { checker.isUpToDate($0.location) }

Sources/SourceKitLSP/LanguageService.swift

@@ -198,7 +198,7 @@ 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) async throws -> [TestItem]
+  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

Sources/SourceKitLSP/Swift/SyntacticTestIndex.swift

@@ -142,6 +142,8 @@ actor SyntacticTestIndex {
 
   /// Called when a list of files was updated. Re-scans those files
   private func rescanFiles(_ uris: [DocumentURI]) {
+    logger.info("Syntactically scanning files for tests: \(uris)")
+
     // If we scan a file again, it might have been added after being removed before. Remove it from the list of removed
     // files.
     removedFiles.subtract(uris)

Sources/SourceKitLSP/TestDiscovery.swift

@@ -194,30 +194,39 @@ extension SourceKitLSPServer {
     )
     let filesWithTestsFromSemanticIndex = Set(testsFromSemanticIndex.map(\.location.uri))
 
+    let indexOnlyDiscardingDeletedFiles = workspace.index(checkedFor: .deletedFiles)
+
     let syntacticTestsToInclude =
       testsFromSyntacticIndex
-      .filter { testItem in
+      .compactMap { (testItem) -> TestItem? in
         if testItem.style == TestStyle.swiftTesting {
           // Swift-testing tests aren't part of the semantic index. Always include them.
-          return true
+          return testItem
         }
         if filesWithTestsFromSemanticIndex.contains(testItem.location.uri) {
           // If we have an semantic tests from this file, then the semantic index is up-to-date for this file. We thus
           // don't need to include results from the syntactic index.
-          return false
+          return nil
         }
         if filesWithInMemoryState.contains(testItem.location.uri) {
           // If the file has been modified in the editor, the syntactic index (which indexes on-disk files) is no longer
           // up-to-date. Include the tests from `testsFromFilesWithInMemoryState`.
-          return false
+          return nil
         }
         if let fileUrl = testItem.location.uri.fileURL, index?.hasUpToDateUnit(for: fileUrl) ?? false {
           // We don't have a test for this file in the semantic index but an up-to-date unit file. This means that the
           // index is up-to-date and has more knowledge that identifies a `TestItem` as not actually being a test, eg.
           // because it starts with `test` but doesn't appear in a class inheriting from `XCTestCase`.
-          return false
+          return nil
         }
-        return true
+        // Filter out any test items that we know aren't actually tests based on the semantic index.
+        // This might call `symbols(inFilePath:)` multiple times if there are multiple top-level test items (ie.
+        // XCTestCase subclasses, swift-testing handled above) for the same file. In practice test files usually contain
+        // a single XCTestCase subclass, so caching doesn't make sense here.
+        // Also, this is only called for files containing test cases but for which the semantic index is out-of-date.
+        return testItem.filterUsing(
+          semanticSymbols: indexOnlyDiscardingDeletedFiles?.symbols(inFilePath: testItem.location.uri.pseudoPath)
+        )
       }
 
     // We don't need to sort the tests here because they will get
@@ -252,7 +261,7 @@ extension SourceKitLSPServer {
       language: snapshot.language
     )
 
-    let syntacticTests = try await languageService.syntacticDocumentTests(for: req.textDocument.uri)
+    let syntacticTests = try await languageService.syntacticDocumentTests(for: req.textDocument.uri, in: workspace)
 
     if let index = workspace.index(checkedFor: .inMemoryModifiedFiles(documentManager)) {
       var syntacticSwiftTestingTests: [TestItem] {
@@ -398,13 +407,43 @@ final class SyntacticSwiftXCTestScanner: SyntaxVisitor {
   }
 }
 
+extension TestItem {
+  /// Use out-of-date semantic information to filter syntactic symbols.
+  ///
+  /// If the syntactic index found a test item, check if the semantic index knows about a symbol with that name. If it
+  /// does and that item is not marked as a test symbol, we can reasonably assume that this item still looks like a test
+  /// but is semantically known to not be a test. It will thus get filtered out.
+  ///
+  /// `semanticSymbols` should be all the symbols in the source file that this `TestItem` occurs in, retrieved using
+  /// `symbols(inFilePath:)` from the index.
+  fileprivate func filterUsing(semanticSymbols: [Symbol]?) -> TestItem? {
+    guard let semanticSymbols else {
+      return self
+    }
+    // We only check if we know of any symbol with the test item's name in this file. We could try to incorporate
+    // structure here (ie. look for a method within a class) but that makes the index lookup more difficult and in
+    // practice it is very unlikely that a test file will have two symbols with the same name, one of which is marked
+    // as a unit test while the other one is not.
+    let semanticSymbolsWithName = semanticSymbols.filter { $0.name == self.label }
+    if !semanticSymbolsWithName.isEmpty,
+      semanticSymbolsWithName.allSatisfy({ !$0.properties.contains(.unitTest) })
+    {
+      return nil
+    }
+    var test = self
+    test.children = test.children.compactMap { $0.filterUsing(semanticSymbols: semanticSymbols) }
+    return test
+  }
+}
+
 extension SwiftLanguageService {
-  public func syntacticDocumentTests(for uri: DocumentURI) 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(
       in: snapshot,
       syntaxTreeManager: syntaxTreeManager
-    )
+    ).compactMap { $0.filterUsing(semanticSymbols: semanticSymbols) }
     let swiftTestingSymbols = await SyntacticSwiftTestingTestScanner.findTestSymbols(
       in: snapshot,
       syntaxTreeManager: syntaxTreeManager
@@ -414,7 +453,7 @@ extension SwiftLanguageService {
 }
 
 extension ClangLanguageService {
-  public func syntacticDocumentTests(for uri: DocumentURI) async -> [TestItem] {
+  public func syntacticDocumentTests(for uri: DocumentURI, in workspace: Workspace) async -> [TestItem] {
     return []
   }
 }

Tests/SourceKitLSPTests/DocumentTestDiscoveryTests.swift

@@ -993,4 +993,76 @@ final class DocumentTestDiscoveryTests: XCTestCase {
       ]
     )
   }
+
+  func testAddNewMethodToNotQuiteTestCase() async throws {
+    let project = try await IndexedSingleSwiftFileTestProject(
+      """
+      import XCTest
+
+      class NotQuiteTest: SomeClass {
+        func testMyLibrary() {}
+      2️⃣
+      }
+      """,
+      allowBuildFailure: true
+    )
+
+    let testsBeforeEdit = try await project.testClient.send(
+      DocumentTestsRequest(textDocument: TextDocumentIdentifier(project.fileURI))
+    )
+    XCTAssertEqual(testsBeforeEdit, [])
+    project.testClient.send(
+      DidChangeTextDocumentNotification(
+        textDocument: VersionedTextDocumentIdentifier(project.fileURI, version: 2),
+        contentChanges: [
+          TextDocumentContentChangeEvent(range: Range(project.positions["2️⃣"]), text: "func testSomethingElse() {}")
+        ]
+      )
+    )
+    let testsAfterEdit = try await project.testClient.send(
+      DocumentTestsRequest(textDocument: TextDocumentIdentifier(project.fileURI))
+    )
+    XCTAssertEqual(testsAfterEdit, [])
+  }
+
+  func testAddNewClassToNotQuiteTestCase() async throws {
+    let project = try await IndexedSingleSwiftFileTestProject(
+      """
+      import XCTest
+
+      class NotQuiteTest: SomeClass {
+        func testMyLibrary() {}
+      }
+      2️⃣
+      """,
+      allowBuildFailure: true
+    )
+
+    let testsBeforeEdit = try await project.testClient.send(
+      DocumentTestsRequest(textDocument: TextDocumentIdentifier(project.fileURI))
+    )
+    XCTAssertEqual(testsBeforeEdit, [])
+    project.testClient.send(
+      DidChangeTextDocumentNotification(
+        textDocument: VersionedTextDocumentIdentifier(project.fileURI, version: 2),
+        contentChanges: [
+          TextDocumentContentChangeEvent(
+            range: Range(project.positions["2️⃣"]),
+            text: """
+              class OtherNotQuiteTest: SomeClass {
+                func testSomethingElse() {}
+              }
+              """
+          )
+        ]
+      )
+    )
+    let testsAfterEdit = try await project.testClient.send(
+      DocumentTestsRequest(textDocument: TextDocumentIdentifier(project.fileURI))
+    )
+    // We know from the semantic index that NotQuiteTest does not inherit from XCTestCase, so we should not include it.
+    // We don't have any semantic knowledge about `OtherNotQuiteTest`, so we are conservative and should include it.
+    XCTAssertFalse(testsAfterEdit.contains { $0.label == "NotQuiteTest" })
+    XCTAssertTrue(testsAfterEdit.contains { $0.label == "OtherNotQuiteTest" })
+  }
 }

Tests/SourceKitLSPTests/WorkspaceTestDiscoveryTests.swift

@@ -362,14 +362,12 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
         "Tests/MyLibraryTests/MyTests.swift": """
         import XCTest
 
-        class MayInheritFromXCTestCase {}
-
-        1️⃣class MyTests: MayInheritFromXCTestCase {
-          2️⃣func testMyLibrary0️⃣() {
-          }3️⃣
+        1️⃣class 2️⃣MyTests: XCTestCase {
+          3️⃣func 4️⃣testMyLibrary0️⃣() {
+          }5️⃣
           func unrelatedFunc() {}
           var testVariable: Int = 0
-        }4️⃣
+        }6️⃣
         """
       ],
       manifest: packageManifestWithTestTarget,
@@ -381,7 +379,30 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
     // If the document has been opened but not modified in-memory, we can still use the semantic index and detect that
     // `MyTests` does not inherit from `XCTestCase`.
     let testsAfterDocumentOpen = try await project.testClient.send(WorkspaceTestsRequest())
-    XCTAssertEqual(testsAfterDocumentOpen, [])
+    XCTAssertEqual(
+      testsAfterDocumentOpen,
+      [
+        TestItem(
+          id: "MyTests",
+          label: "MyTests",
+          disabled: false,
+          style: TestStyle.xcTest,
+          location: Location(uri: uri, range: positions["2️⃣"]..<positions["2️⃣"]),
+          children: [
+            TestItem(
+              id: "MyTests/testMyLibrary()",
+              label: "testMyLibrary()",
+              disabled: false,
+              style: TestStyle.xcTest,
+              location: Location(uri: uri, range: positions["4️⃣"]..<positions["4️⃣"]),
+              children: [],
+              tags: []
+            )
+          ],
+          tags: []
+        )
+      ]
+    )
 
     // After we have an in-memory change to the file, we can't use the semantic index to discover the tests anymore.
     // Use the syntactic index instead.
@@ -403,14 +424,14 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
           label: "MyTests",
           disabled: false,
           style: TestStyle.xcTest,
-          location: Location(uri: uri, range: positions["1️⃣"]..<positions["4️⃣"]),
+          location: Location(uri: uri, range: positions["1️⃣"]..<positions["6️⃣"]),
           children: [
             TestItem(
               id: "MyTests/testMyLibraryUpdated()",
               label: "testMyLibraryUpdated()",
               disabled: false,
               style: TestStyle.xcTest,
-              location: Location(uri: uri, range: positions["2️⃣"]..<positions["3️⃣"]),
+              location: Location(uri: uri, range: positions["3️⃣"]..<positions["5️⃣"]),
               children: [],
               tags: []
             )
@@ -848,4 +869,43 @@ final class WorkspaceTestDiscoveryTests: XCTestCase {
     let tests = try await project.testClient.send(WorkspaceTestsRequest())
     XCTAssertEqual(tests, [])
   }
+
+  func testAddNewClassToNotQuiteTestCase() async throws {
+    let originalContents = """
+      import XCTest
+
+      class NotQuiteTest: SomeClass {
+        func testMyLibrary() {}
+      }
+
+      """
+
+    let project = try await IndexedSingleSwiftFileTestProject(originalContents, allowBuildFailure: true)
+    // Close the file so we don't have an in-memory version of it.
+    project.testClient.send(DidCloseTextDocumentNotification(textDocument: TextDocumentIdentifier(project.fileURI)))
+
+    let testsBeforeEdit = try await project.testClient.send(WorkspaceTestsRequest())
+    XCTAssertEqual(testsBeforeEdit, [])
+
+    let addedTest = """
+      class OtherNotQuiteTest: SomeClass {
+        func testSomethingElse() {}
+      }
+      """
+
+    let uri = try XCTUnwrap(project.fileURI.fileURL)
+
+    try (originalContents + addedTest).write(to: uri, atomically: true, encoding: .utf8)
+
+    project.testClient.send(
+      DidChangeWatchedFilesNotification(changes: [FileEvent(uri: project.fileURI, type: .changed)])
+    )
+
+    let testsAfterEdit = try await project.testClient.send(WorkspaceTestsRequest())
+    print(testsAfterEdit)
+    // We know from the semantic index that NotQuiteTest does not inherit from XCTestCase, so we should not include it.
+    // We don't have any semantic knowledge about `OtherNotQuiteTest`, so we are conservative and should include it.
+    XCTAssertFalse(testsAfterEdit.contains { $0.label == "NotQuiteTest" })
+    XCTAssertTrue(testsAfterEdit.contains { $0.label == "OtherNotQuiteTest" })
+  }
 }