End-to-end LSP cancellation tests #2256
Conversation
There was a problem hiding this comment.
Looks 💯 to me! There are a few tiniest-of-nits comments I made as I was reading, mostly to keep my place as I kept jumping between this PR and looking at the code in context. (I don't care that you actually do anything about any of them unless you feel inspired...I may ask you IRL about them tomorrow...)
| auto SorbetTypecheckRunInfo = makeObject("SorbetTypecheckRunInfo", | ||
| { | ||
| makeField("tookFastPath", JSONBool), | ||
| makeField("status", SorbetTypecheckRunStatus), | ||
| makeField("fastPath", JSONBool), |
There was a problem hiding this comment.
Trivial naming observation: It totally makes sense that tookFastPath => fastPath reflects that if the Status is merely "Started", you don't know which one you'll take. Is filesTypeChecked actually the files that have already been successfully typechecked, or the full list that will be checked? (This name makes me think that it's the former.)
There was a problem hiding this comment.
It's the former. It's not specified at the start of typechecking, because (at least for fast path) we don't know which files you'll pull in yet.
| // Make another edit that fixes syntax error and should take fast path. | ||
| sendAsync(*changeFile("foo.rb", "# typed: true\n\nclass Foo\nend\n", 2, false)); |
There was a problem hiding this comment.
Observation: this reverts foo.rb to the state of the files at line 36. Does this take the fast path because it reverted to a state that has already been previously parsed, or does it work regardless of previous parsings because it fixed the syntax error?
(The comment on line 63 made me think that it's the latter, in which case it would have been slightly clearer to me if line 64 added the end to def noend, rather than removing the method declaration.)
There was a problem hiding this comment.
It takes the fast path because it fixes the syntax error and doesn't make any structural changes to the code (e.g., new methods).
An upcoming change will change the cancellation logic to become a great deal simpler, and I wanted to land these tests (which are agnostic to the implementation) separately to make reviewing easier.
5c2e599 to
e567de0
Compare
|
Rebased, updated the comment, and used a helper function to make the code a bit cleaner. |
Adds two end-to-end LSP cancellation tests that run in multithreaded mode. This is in contrast to the existing tests, which are unit tests that do not test the whole cancellation mechanism.
There are two tests:
I add a new field to edit messages,
sorbetCancellationExpected, which propagates through to slow path typechecking. If it's set, the slow path typechecking routine will block forever (via polling every 1ms if it has been cancelled) until cancellation occurs.Motivation
I'm changing how cancellation is implemented to simplify the mechanism and to make it work better with preemption. However, I'm going to need to land in master at the same time as pre-emption, which will make for a difficult PR review!
To try to lessen the review burden, I broke off these tests into their own PR because they are agnostic to how cancellation is implemented.
These tests are necessary because the old tests will no longer function (and will be deleted) when I change the cancellation implementation.
Test plan
See included automated tests.