Clean Tests Slightly, Add lspURI to URL type.

thecoolwinter · thecoolwinter · commit 04da78f9ee05 · 2025-03-24T15:59:38.000-05:00
diff --git a/CodeEdit/Features/Documents/CodeFileDocument/CodeFileDocument.swift b/CodeEdit/Features/Documents/CodeFileDocument/CodeFileDocument.swift
@@ -231,10 +231,6 @@ extension CodeFileDocument: LanguageServerDocument {
     /// A stable string to use when identifying documents with language servers.
     /// Needs to be a valid URI, so always returns with the `file://` prefix to indicate it's a file URI.
     var languageServerURI: String? {
-        if let path = fileURL?.absolutePath {
-            return "file://" + path
-        } else {
-            return nil
-        }
+        fileURL?.lspURI
     }
 }
diff --git a/CodeEdit/Features/LSP/Service/LSPService.swift b/CodeEdit/Features/LSP/Service/LSPService.swift
@@ -42,7 +42,7 @@ import CodeEditLanguages
 ///     do {
 ///         guard var languageClient = self.languageClient(for: .python) else {
 ///             print("Failed to get client")
-///             throw ServerManagerError.languageClientNotFound
+///             throw LSPServiceError.languageClientNotFound
 ///         }
 ///
 ///         let testFilePathStr = ""
@@ -54,7 +54,7 @@ import CodeEditLanguages
 ///         // Completion example
 ///         let textPosition = Position(line: 32, character: 18)  // Lines and characters start at 0
 ///         let completions = try await languageClient.requestCompletion(
-///             document: testFileURL.absoluteString,
+///             document: testFileURL.lspURI,
 ///             position: textPosition
 ///         )
 ///         switch completions {
@@ -168,6 +168,12 @@ final class LSPService: ObservableObject {
         return languageClients[ClientKey(languageId, workspacePath)]
     }
 
+    func languageClient(forDocument url: URL) -> LanguageServerType? {
+        languageClients.values.first(where: { $0.openFiles.document(for: url.lspURI) != nil })
+    }
+
+    // MARK: - Start Server
+
     /// Given a language and workspace path, will attempt to start the language server
     /// - Parameters:
     ///   - languageId: The ID of the language server to start.
@@ -195,6 +201,8 @@ final class LSPService: ObservableObject {
         return server
     }
 
+    // MARK: - Document Management
+
     /// Notify all relevant language clients that a document was opened.
     /// - Note: Must be invoked after the contents of the file are available.
     /// - Parameter document: The code document that was opened.
@@ -230,21 +238,19 @@ final class LSPService: ObservableObject {
     /// Notify all relevant language clients that a document was closed.
     /// - Parameter url: The url of the document that was closed
     func closeDocument(_ url: URL) {
-        guard let languageClient = languageClients.first(where: {
-                  $0.value.openFiles.document(for: url.absolutePath) != nil
-              })?.value else {
-            return
-        }
+        guard let languageClient = languageClient(forDocument: url) else { return }
         Task {
             do {
-                try await languageClient.closeDocument(url.absolutePath)
+                try await languageClient.closeDocument(url.lspURI)
             } catch {
                 // swiftlint:disable:next line_length
-                logger.error("Failed to close document: \(url.absolutePath, privacy: .private), language: \(languageClient.languageId.rawValue). Error \(error)")
+                logger.error("Failed to close document: \(url.lspURI, privacy: .private), language: \(languageClient.languageId.rawValue). Error \(error)")
             }
         }
     }
 
+    // MARK: - Close Workspace
+
     /// Close all language clients for a workspace.
     ///
     /// This is intentionally synchronous so we can exit from the workspace document's ``WorkspaceDocument/close()``
@@ -268,6 +274,8 @@ final class LSPService: ObservableObject {
         }
     }
 
+    // MARK: - Stop Servers
+
     /// Attempts to stop a running language server. Throws an error if the server is not found
     /// or if the language server throws an error while trying to shutdown.
     /// - Parameters:
@@ -276,7 +284,7 @@ final class LSPService: ObservableObject {
     func stopServer(forLanguage languageId: LanguageIdentifier, workspacePath: String) async throws {
         guard let server = server(for: languageId, workspacePath: workspacePath) else {
             logger.error("Server not found for language \(languageId.rawValue) during stop operation")
-            throw ServerManagerError.serverNotFound
+            throw LSPServiceError.serverNotFound
         }
         do {
             try await server.shutdownAndExit()
@@ -311,12 +319,3 @@ final class LSPService: ObservableObject {
         eventListeningTasks.removeAll()
     }
 }
-
-// MARK: - Errors
-
-enum ServerManagerError: Error {
-    case serverNotFound
-    case serverStartFailed
-    case serverStopFailed
-    case languageClientNotFound
-}
diff --git a/CodeEdit/Features/LSP/Service/LSPServiceError.swift b/CodeEdit/Features/LSP/Service/LSPServiceError.swift
@@ -0,0 +1,13 @@
+//
+//  LSPServiceError.swift
+//  CodeEdit
+//
+//  Created by Khan Winter on 3/24/25.
+//
+
+enum LSPServiceError: Error {
+    case serverNotFound
+    case serverStartFailed
+    case serverStopFailed
+    case languageClientNotFound
+}
diff --git a/CodeEdit/Utils/Extensions/URL/URL+LSPURI.swift b/CodeEdit/Utils/Extensions/URL/URL+LSPURI.swift
@@ -0,0 +1,18 @@
+//
+//  URL+LSPURI.swift
+//  CodeEdit
+//
+//  Created by Khan Winter on 3/24/25.
+//
+
+import Foundation
+
+extension URL {
+    /// A stable string to use when identifying documents with language servers.
+    /// Needs to be a valid URI, so always returns with the `file://` prefix to indicate it's a file URI.
+    ///
+    /// Use this whenever possible when using USLs in LSP processing if not using the ``LanguageServerDocument`` type.
+    var lspURI: String {
+        return "file://" + absolutePath
+    }
+}
diff --git a/CodeEditTests/Features/LSP/LanguageServer+CodeFileDocument.swift b/CodeEditTests/Features/LSP/LanguageServer+CodeFileDocument.swift
@@ -13,6 +13,10 @@ import LanguageServerProtocol
 
 @testable import CodeEdit
 
+/// This is an integration test for notifications relating to the ``CodeFileDocument`` class.
+/// 
+/// For *unit* tests with the language server class, add tests to the `LanguageServer+DocumentObjects` test class as
+/// it's cleaner and makes correct use of the mock document type.
 final class LanguageServerCodeFileDocumentTests: XCTestCase {
     // Test opening documents in CodeEdit triggers creating a language server,
     // further opened documents don't create new servers
@@ -97,8 +101,11 @@ final class LanguageServerCodeFileDocumentTests: XCTestCase {
         // This is usually sent from the LSPService
         try await server.openDocument(codeFile)
 
-        await waitForClientEventCount(
-            3,
+        await waitForClientState(
+            (
+                [.initialize],
+                [.initialized, .textDocumentDidOpen]
+            ),
             connection: connection,
             description: "Initialized (2) and opened (1) notification count"
         )
@@ -110,22 +117,27 @@ final class LanguageServerCodeFileDocumentTests: XCTestCase {
         return codeFile
     }
 
-    func waitForClientEventCount(_ count: Int, connection: BufferingServerConnection, description: String) async {
+    func waitForClientState(
+        _ expectedValue: ([ClientRequest.Method], [ClientNotification.Method]),
+        connection: BufferingServerConnection,
+        description: String
+    ) async {
         let expectation = expectation(description: description)
 
         await withTaskGroup(of: Void.self) { group in
+            group.addTask { await self.fulfillment(of: [expectation], timeout: 2) }
             group.addTask {
-                await self.fulfillment(of: [expectation], timeout: 2)
-            }
-            group.addTask {
-                for await events in connection.clientEventSequence where events.0.count + events.1.count == count {
+                for await events in connection.clientEventSequence
+                where events.0.map(\.method) == expectedValue.0 && events.1.map(\.method) == expectedValue.1 {
                     expectation.fulfill()
                     return
                 }
             }
         }
     }
 
+    // MARK: - Open Close
+
     @MainActor
     func testOpenCloseFileNotifications() async throws {
         // Set up test server
@@ -155,40 +167,38 @@ final class LanguageServerCodeFileDocumentTests: XCTestCase {
         file.fileDocument = codeFile
         CodeEditDocumentController.shared.addDocument(codeFile)
 
-        await waitForClientEventCount(3, connection: connection, description: "Pre-close event count")
+        await waitForClientState(
+            (
+                [.initialize],
+                [.initialized, .textDocumentDidOpen]
+            ),
+            connection: connection,
+            description: "Pre-close event count"
+        )
 
         // This should then trigger a documentDidClose event
         codeFile.close()
 
-        await waitForClientEventCount(4, connection: connection, description: "Post-close event count")
-
-        XCTAssertEqual(
-            connection.clientRequests.map { $0.method },
-            [
-                ClientRequest.Method.initialize,
-            ]
-        )
-
-        XCTAssertEqual(
-            connection.clientNotifications.map { $0.method },
-            [
-                ClientNotification.Method.initialized,
-                ClientNotification.Method.textDocumentDidOpen,
-                ClientNotification.Method.textDocumentDidClose
-            ]
+        await waitForClientState(
+            (
+                [.initialize],
+                [.initialized, .textDocumentDidOpen, .textDocumentDidClose]
+            ),
+            connection: connection,
+            description: "Post-close event count"
         )
     }
 
+    // MARK: - Test Document Edit
+
     /// Assert the changed contents received by the buffered connection
     func assertExpectedContentChanges(connection: BufferingServerConnection, changes: [String]) {
         var foundChangeContents: [String] = []
 
         for notification in connection.clientNotifications {
             switch notification {
             case let .textDocumentDidChange(params):
-                foundChangeContents.append(contentsOf: params.contentChanges.map { event in
-                    event.text
-                })
+                foundChangeContents.append(contentsOf: params.contentChanges.map(\.text))
             default:
                 continue
             }
@@ -233,18 +243,17 @@ final class LanguageServerCodeFileDocumentTests: XCTestCase {
             textView.replaceCharacters(in: NSRange(location: 39, length: 0), with: "World")
 
             // Added one notification
-            await waitForClientEventCount(4, connection: connection, description: "Edited notification count")
+            await waitForClientState(
+                (
+                    [.initialize],
+                    [.initialized, .textDocumentDidOpen, .textDocumentDidChange]
+                ),
+                connection: connection,
+                description: "Edited notification count"
+            )
 
             // Make sure our text view is intact
             XCTAssertEqual(textView.string, #"func testFunction() -> String { "Hello World" }"#)
-            XCTAssertEqual(
-                [
-                    ClientNotification.Method.initialized,
-                    ClientNotification.Method.textDocumentDidOpen,
-                    ClientNotification.Method.textDocumentDidChange
-                ],
-                connection.clientNotifications.map { $0.method }
-            )
 
             // Expect only one change due to throttling.
             assertExpectedContentChanges(
@@ -291,18 +300,17 @@ final class LanguageServerCodeFileDocumentTests: XCTestCase {
             textView.replaceCharacters(in: NSRange(location: 39, length: 0), with: "World")
 
             // Throttling means we should receive one edited notification + init notification + didOpen + init request
-            await waitForClientEventCount(4, connection: connection, description: "Edited notification count")
+            await waitForClientState(
+                (
+                    [.initialize],
+                    [.initialized, .textDocumentDidOpen, .textDocumentDidChange]
+                ),
+                connection: connection,
+                description: "Edited notification count"
+            )
 
             // Make sure our text view is intact
             XCTAssertEqual(textView.string, #"func testFunction() -> String { "Hello World" }"#)
-            XCTAssertEqual(
-                [
-                    ClientNotification.Method.initialized,
-                    ClientNotification.Method.textDocumentDidOpen,
-                    ClientNotification.Method.textDocumentDidChange
-                ],
-                connection.clientNotifications.map { $0.method }
-            )
 
             // Expect three content changes.
             assertExpectedContentChanges(

Original file line number	Diff line number	Diff line change
`@@ -231,10 +231,6 @@ extension CodeFileDocument: LanguageServerDocument {`
`231`	`231`	`/// A stable string to use when identifying documents with language servers.`
`232`	`232`	/// Needs to be a valid URI, so always returns with the `file://` prefix to indicate it's a file URI.
`233`	`233`	`var languageServerURI: String? {`
`234`		`- if let path = fileURL?.absolutePath {`
`235`		`- return "file://" + path`
`236`		`- } else {`
`237`		`- return nil`
`238`		`- }`
	`234`	`+ fileURL?.lspURI`
`239`	`235`	`}`
`240`	`236`	`}`