Simplify tests #896
Merged
Simplify tests #896
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove the JSONRPC connection kind between `TestSourceKitServer` and `SourceKitServer` - It wasn’t actually used and the connection abstraction just made things more complicated than they needed to be - Send requests and notifications to `SourceKitServer` by directly calling into `SourceKitServer.handle` instead of going through a `Connection`. This makes the code a lot easier to understand statically - Make `TestSourceKitServer` conform to `MessageHandler` instead of going through `TestClient` - IMO this centralizes all the handling and makes it a lot easier to follow. `TestClient` didn’t do a whole bunch anyway. - Allow async awaiting of next notifications instead of having to register a `handleNextNotification` handler before expecting the notification to be emitted. - This allows us to remove quite a few `XCTExpectation`s in test cases - Change `sendSync` function that sends a request and returns the result to be `async`
Really, what the class was mocking is a SourceKit-LSP client.
…eKitServer.Options` Really, these options had nothing to do with `TestSourceKitLSPClient`, they are just the defaults that are used for all the tests.
It turns out that `XCTestCase.setUp` can indeed be `async` and we thus don’t actually need `awaitTask`
|
@swift-ci Please test |
bnbarham
reviewed
Oct 12, 2023
Comment on lines
+538
to
+541
| // FIXME: (async) We need more granular request handling. Completion requests | ||
| // to the same file depend on each other because we only have one global | ||
| // code completion session in sourcekitd but they don't need to be full | ||
| // barriers to any other request. |
There was a problem hiding this comment.
If there's only the one session, doesn't that mean all completion requests (even across files) have to depend on each other? What happens today if multiple are sent?
We could also just make it an error to request completion when there's already a completion happening.
There was a problem hiding this comment.
Let’s discuss any of the code completion scheduling on #899.
hamishknight
approved these changes
Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies for the large diff but all the changes are fairly straightforward and intertwined so it doesn’t make too much sense to split them up.
The goal of this PR is to simplify the API of
TestSourceKitServerSourceKitServerby directly calling intoSourceKitServer.handleinstead of going through aConnection. This makes the code a lot easier to understand staticallyTestSourceKitServerconform toMessageHandlerinstead of going throughTestClientTestClientdidn’t do a whole bunch anyway.sendSyncfunction that sends a request and returns the result to beasynchandleNextNotificationhandler before expecting the notification to be emitted.XCTExpectations in test casesTestSourceKitServerandSourceKitServerTestSourceKitServertoTestSourceKitClientbecause really, it’s mocking an LSP client and isn’t modeling a server.While doing this refactoring, I discovered a race condition where two code completion requests could be executed concurrently, causing conflicts because
sourcekitdonly has a single, global code completion session. I hot-fixed this for now by making code completion requests barriers in the message handling queue. The real fix is #899.