From 92f3c7740d8d7f2412e14e5418c26e04cfd8780a Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 25 Mar 2025 09:09:41 -0700 Subject: [PATCH 1/3] Wait for request to be sent to sourcekitd before cancelling it in `SwiftSourceKitPluginTests.testCancellation` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise, the cancellation could get handled before the request actually gets sent to sourcekitd, which is not what we want to test here. There appears to be a second underlying issue that causes unexpected results when we cancel the fast completion request after sending the slow completion request. I’ll look at that in a follow-up PR. --- .../DynamicallyLoadedSourceKitD.swift | 26 ++++++++++++++++ Sources/SourceKitD/SourceKitD.swift | 7 +++++ .../SourceKitDRegistryTests.swift | 1 + .../SwiftSourceKitPluginTests.swift | 31 +++++++++++-------- 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift b/Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift index eb4f7fe72..459dd5ba7 100644 --- a/Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift +++ b/Sources/SourceKitD/DynamicallyLoadedSourceKitD.swift @@ -108,6 +108,9 @@ package actor DynamicallyLoadedSourceKitD: SourceKitD { /// List of notification handlers that will be called for each notification. private var notificationHandlers: [WeakSKDNotificationHandler] = [] + /// List of hooks that should be executed for every request sent to sourcekitd. + private var requestHandlingHooks: [UUID: (SKDRequestDictionary) -> Void] = [:] + package static func getOrCreate( dylibPath: URL, pluginPaths: PluginPaths? @@ -204,6 +207,29 @@ package actor DynamicallyLoadedSourceKitD: SourceKitD { notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler }) } + /// Execute `body` and invoke `hook` for every sourcekitd request that is sent during the execution time of `body`. + /// + /// Note that `hook` will not only be executed for requests sent *by* body but this may also include sourcekitd + /// requests that were sent by other clients of the same `DynamicallyLoadedSourceKitD` instance that just happen to + /// send a request during that time. + /// + /// This is intended for testing only. + package func withRequestHandlingHook( + body: () async throws -> Void, + hook: @escaping (SKDRequestDictionary) -> Void + ) async rethrows { + let id = UUID() + requestHandlingHooks[id] = hook + defer { requestHandlingHooks[id] = nil } + try await body() + } + + package func didSend(request: SKDRequestDictionary) { + for hook in requestHandlingHooks.values { + hook(request) + } + } + package nonisolated func log(request: SKDRequestDictionary) { logger.info( """ diff --git a/Sources/SourceKitD/SourceKitD.swift b/Sources/SourceKitD/SourceKitD.swift index ca7dd07e1..77e78e106 100644 --- a/Sources/SourceKitD/SourceKitD.swift +++ b/Sources/SourceKitD/SourceKitD.swift @@ -83,6 +83,10 @@ package protocol SourceKitD: AnyObject, Sendable { /// Removes a previously registered notification handler. func removeNotificationHandler(_ handler: SKDNotificationHandler) async + /// A function that gets called after the request has been sent to sourcekitd but but does not wait for results to be + /// received. This can be used by clients to implement hooks that should be executed for every sourcekitd request. + func didSend(request: SKDRequestDictionary) async + /// Log the given request. /// /// This log call is issued during normal operation. It is acceptable for the logger to truncate the log message @@ -146,6 +150,9 @@ extension SourceKitD { self.api.send_request(request.dict, &handle) { response in continuation.resume(returning: SKDResponse(response!, sourcekitd: self)) } + Task { + await self.didSend(request: request) + } if let handle { return SourceKitDRequestHandle(handle: handle) } diff --git a/Tests/SourceKitDTests/SourceKitDRegistryTests.swift b/Tests/SourceKitDTests/SourceKitDRegistryTests.swift index c98f4a9b6..c8cc2614c 100644 --- a/Tests/SourceKitDTests/SourceKitDRegistryTests.swift +++ b/Tests/SourceKitDTests/SourceKitDRegistryTests.swift @@ -74,6 +74,7 @@ final class FakeSourceKitD: SourceKitD { var values: sourcekitd_api_values { fatalError() } func addNotificationHandler(_ handler: SKDNotificationHandler) { fatalError() } func removeNotificationHandler(_ handler: SKDNotificationHandler) { fatalError() } + func didSend(request: SKDRequestDictionary) {} private init() { token = nextToken.fetchAndIncrement() } diff --git a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift index 4bc24113d..c859652d6 100644 --- a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift +++ b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift @@ -311,22 +311,27 @@ final class SwiftSourceKitPluginTests: XCTestCase { compilerArguments: [path] ) + let slowCompletionRequestSent = self.expectation(description: "slow completion result sent") let slowCompletionResultReceived = self.expectation(description: "slow completion") - let slowCompletionTask = Task { - do { - _ = try await sourcekitd.completeOpen( - path: path, - position: positions["1️⃣"], - filter: "" - ) - XCTFail("Expected completion to be cancelled") - } catch { - XCTAssert(error is CancellationError, "Expected completion to be cancelled, failed with \(error)") + let dynamicallyLoadedSourceKitd = try XCTUnwrap(sourcekitd as? DynamicallyLoadedSourceKitD) + try await dynamicallyLoadedSourceKitd.withRequestHandlingHook { + let slowCompletionTask = Task { + await assertThrowsError(try await sourcekitd.completeOpen(path: path, position: positions["1️⃣"], filter: "")) { + XCTAssert($0 is CancellationError, "Expected completion to be cancelled, failed with \($0)") + } + slowCompletionResultReceived.fulfill() } - slowCompletionResultReceived.fulfill() + // Wait for the slow completion request to actually be sent to sourcekitd. Otherwise, we might hit a cancellation + // check somewhere during request sending and we aren't actually sending the completion request to sourcekitd. + try await fulfillmentOfOrThrow(slowCompletionRequestSent) + + slowCompletionTask.cancel() + try await fulfillmentOfOrThrow(slowCompletionResultReceived, timeout: 30) + } hook: { request in + // Check that we aren't matching against a request sent by something else that has handle to the same sourcekitd. + XCTAssert(request.description.contains(path), "Received unexpected request: \(request)") + slowCompletionRequestSent.fulfill() } - slowCompletionTask.cancel() - try await fulfillmentOfOrThrow(slowCompletionResultReceived, timeout: 30) let fastCompletionStarted = Date() let result = try await sourcekitd.completeOpen( From 0f5727f4f0bde9dbcc89e12e3831c0836c5b2397 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 25 Mar 2025 19:12:51 -0700 Subject: [PATCH 2/3] Introduce `assertContains` to check that a sequence/string contains an element/substring This also prints the sequence and expected element on failure, which is useful for debugging. --- Sources/SKTestSupport/Assertions.swift | 20 +++++++++++++++++++ .../BuildServerBuildSystemTests.swift | 6 +++--- .../BuildSystemManagerTests.swift | 2 +- .../SwiftPMBuildSystemTests.swift | 6 +++--- .../CompilationDatabaseTests.swift | 6 +++--- Tests/SourceKitLSPTests/LocalSwiftTests.swift | 2 +- Tests/SourceKitLSPTests/WorkspaceTests.swift | 4 ++-- .../SwiftSourceKitPluginTests.swift | 2 +- 8 files changed, 34 insertions(+), 14 deletions(-) diff --git a/Sources/SKTestSupport/Assertions.swift b/Sources/SKTestSupport/Assertions.swift index 73cb64186..4d6217069 100644 --- a/Sources/SKTestSupport/Assertions.swift +++ b/Sources/SKTestSupport/Assertions.swift @@ -119,6 +119,26 @@ package func assertNotNil( XCTAssertNotNil(expression, message(), file: file, line: line) } +/// Check that the string contains the given substring. +package func assertContains( + _ string: some StringProtocol, + _ substring: some StringProtocol, + file: StaticString = #filePath, + line: UInt = #line +) { + XCTAssert(string.contains(substring), "Expected to contain '\(substring)': \(string)", file: file, line: line) +} + +/// Check that the sequence contains the given element. +package func assertContains( + _ sequence: some Sequence, + _ element: Element, + file: StaticString = #filePath, + line: UInt = #line +) { + XCTAssert(sequence.contains(element), "Expected to contain '\(element)': \(sequence)", file: file, line: line) +} + /// Same as `XCTUnwrap` but doesn't take autoclosures and thus `expression` /// can contain `await`. package func unwrap( diff --git a/Tests/BuildSystemIntegrationTests/BuildServerBuildSystemTests.swift b/Tests/BuildSystemIntegrationTests/BuildServerBuildSystemTests.swift index 71108e9a8..82b27af44 100644 --- a/Tests/BuildSystemIntegrationTests/BuildServerBuildSystemTests.swift +++ b/Tests/BuildSystemIntegrationTests/BuildServerBuildSystemTests.swift @@ -501,7 +501,7 @@ final class BuildServerBuildSystemTests: XCTestCase { allowFallbackSettings: false ) ) - XCTAssert(try XCTUnwrap(firstOptions).compilerArguments.contains("-DFIRST")) + assertContains(try XCTUnwrap(firstOptions).compilerArguments, "-DFIRST") let secondOptions = try await project.testClient.send( SourceKitOptionsRequest( @@ -511,7 +511,7 @@ final class BuildServerBuildSystemTests: XCTestCase { allowFallbackSettings: false ) ) - XCTAssert(try XCTUnwrap(secondOptions).compilerArguments.contains("-DSECOND")) + assertContains(try XCTUnwrap(secondOptions).compilerArguments, "-DSECOND") let optionsWithoutTarget = try await project.testClient.send( SourceKitOptionsRequest( @@ -521,7 +521,7 @@ final class BuildServerBuildSystemTests: XCTestCase { ) ) // We currently pick the canonical target alphabetically, which means that `bsp://first` wins over `bsp://second` - XCTAssert(try XCTUnwrap(optionsWithoutTarget).compilerArguments.contains("-DFIRST")) + assertContains(try XCTUnwrap(optionsWithoutTarget).compilerArguments, "-DFIRST") } func testDontBlockBuildServerInitializationIfBuildSystemIsUnresponsive() async throws { diff --git a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift index 690c5ce94..deb9068cd 100644 --- a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift +++ b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift @@ -116,7 +116,7 @@ final class BuildSystemManagerTests: XCTestCase { await manager.registerForChangeNotifications(for: d, language: .c) await assertEqual(manager.cachedMainFile(for: a), c) let bMain = await manager.cachedMainFile(for: b) - XCTAssert(Set([c, d]).contains(bMain)) + assertContains([c, d], bMain) await assertEqual(manager.cachedMainFile(for: c), c) await assertEqual(manager.cachedMainFile(for: d), d) diff --git a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift index 84c548d2e..23aaf2532 100644 --- a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift +++ b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift @@ -1118,8 +1118,8 @@ final class SwiftPMBuildSystemTests: XCTestCase { fallbackAfterTimeout: false ) let compilerArgs = try XCTUnwrap(settings?.compilerArguments) - XCTAssert(compilerArgs.contains("-package-description-version")) - XCTAssert(compilerArgs.contains(try versionSpecificManifestURL.filePath)) + assertContains(compilerArgs, "-package-description-version") + assertContains(compilerArgs, try versionSpecificManifestURL.filePath) } } @@ -1152,7 +1152,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { ) let compilerArgs = try XCTUnwrap(settings?.compilerArguments) assertArgumentsContain("-package-description-version", "5.1.0", arguments: compilerArgs) - XCTAssert(compilerArgs.contains(try manifestURL.filePath)) + assertContains(compilerArgs, try manifestURL.filePath) } } } diff --git a/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift b/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift index 34ec4cb4b..92a3a9f89 100644 --- a/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift +++ b/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift @@ -151,7 +151,7 @@ final class CompilationDatabaseTests: XCTestCase { XCTFail("Expected ResponseError, got \(error)") return } - XCTAssert(error.message.contains("No language service")) + assertContains(error.message, "No language service") } } @@ -217,7 +217,7 @@ final class CompilationDatabaseTests: XCTestCase { ) ) let hoverContent = try XCTUnwrap(hover?.contents.markupContent?.value) - XCTAssert(hoverContent.contains("void main()")) + assertContains(hoverContent, "void main()") // But for `testFromDummyToolchain.swift`, we can't launch sourcekitd (because it doesn't exist, we just provided a // dummy), so we should receive an error. The exact error here is not super relevant, the important part is that we @@ -235,7 +235,7 @@ final class CompilationDatabaseTests: XCTestCase { XCTFail("Expected ResponseError, got \(error)") return } - XCTAssert(error.message.contains("No language service")) + assertContains(error.message, "No language service") } } } diff --git a/Tests/SourceKitLSPTests/LocalSwiftTests.swift b/Tests/SourceKitLSPTests/LocalSwiftTests.swift index 26be41450..a53728edc 100644 --- a/Tests/SourceKitLSPTests/LocalSwiftTests.swift +++ b/Tests/SourceKitLSPTests/LocalSwiftTests.swift @@ -730,7 +730,7 @@ final class LocalSwiftTests: XCTestCase { XCTAssertEqual(fixit.title, "Use 'new(_:hotness:)' instead") XCTAssertEqual(fixit.diagnostics?.count, 1) - XCTAssert(fixit.diagnostics?.first?.message.contains("is deprecated") == true) + assertContains(try XCTUnwrap(fixit.diagnostics?.first?.message), "is deprecated") XCTAssertEqual( fixit.edit?.changes?[uri], [ diff --git a/Tests/SourceKitLSPTests/WorkspaceTests.swift b/Tests/SourceKitLSPTests/WorkspaceTests.swift index 7157b6442..c2c76e562 100644 --- a/Tests/SourceKitLSPTests/WorkspaceTests.swift +++ b/Tests/SourceKitLSPTests/WorkspaceTests.swift @@ -1181,7 +1181,7 @@ final class WorkspaceTests: XCTestCase { ) ) let options = try XCTUnwrap(optionsOptional) - XCTAssert(options.compilerArguments.contains("-module-name")) + assertContains(options.compilerArguments, "-module-name") XCTAssertEqual(options.kind, .normal) XCTAssertNil(options.didPrepareTarget) } @@ -1366,7 +1366,7 @@ final class WorkspaceTests: XCTestCase { allowFallbackSettings: false ) ) - XCTAssert(options.compilerArguments.contains("-DHAVE_SETTINGS")) + assertContains(options.compilerArguments, "-DHAVE_SETTINGS") } func testOutputPaths() async throws { diff --git a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift index c859652d6..1043cbc8b 100644 --- a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift +++ b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift @@ -329,7 +329,7 @@ final class SwiftSourceKitPluginTests: XCTestCase { try await fulfillmentOfOrThrow(slowCompletionResultReceived, timeout: 30) } hook: { request in // Check that we aren't matching against a request sent by something else that has handle to the same sourcekitd. - XCTAssert(request.description.contains(path), "Received unexpected request: \(request)") + assertContains(request.description, path) slowCompletionRequestSent.fulfill() } From 4a4c114af808a520001feeabbce89729100d9f4d Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 25 Mar 2025 19:13:44 -0700 Subject: [PATCH 3/3] Replace `\\` by `\` when comparing file paths `testCancellation` Undo the escaping of `\` in paths on Windows. --- Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift index 1043cbc8b..816adc4dd 100644 --- a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift +++ b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift @@ -329,7 +329,7 @@ final class SwiftSourceKitPluginTests: XCTestCase { try await fulfillmentOfOrThrow(slowCompletionResultReceived, timeout: 30) } hook: { request in // Check that we aren't matching against a request sent by something else that has handle to the same sourcekitd. - assertContains(request.description, path) + assertContains(request.description.replacing(#"\\"#, with: #"\"#), path) slowCompletionRequestSent.fulfill() }