Merge pull request #1195 from ahoppen/ahoppen/filter-syntactic-tests-based-on-outdated-index

Enhance syntactic test discovery with information from the semantic index

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" })
+  }
 }