Synchronous progress reporting in tests

[pepeiborra]

The WorkDoneBegin and WorkDoneEnd notifications are now sent synchronously for tests, to avoid timing issues reported in #1749

[ndmitchell]

I think this does what you are describing, but it's really hard to see with the retries and the running of the reporter async with a kill. Is there a more direct encoding? Like when we get a progressUpdate Started we sleep, then spawn the thread, and when we get Completed we kill it? I spent a long time trying to find the loop, before realising the loop isn't even in progressLoop, it's in lspShakeProgress.

[pepeiborra]

I'm confused too. There's actually two loops, outer one in the mutually recursive functions forming the state machine, and inner one in lspShakeProgress.

I think the code works fine and I don't see how to make it less confusing, so I've just added more comments.

[ndmitchell]

I was expecting the code to read something along the lines of:

state <- newVar Nothing
let progressUpdate KickCompleted =
        join $ modifyVar state $ \case
             Nothing -> pure (Nothing, pure ())
             Just thread -> pure (Nothing, cancel thread)
    progressUpdate KickStarted =
        modifyVar_ state $ \case
             Nothing -> Just <$> async (mRunLspT ...)
             Just thread -> pure $ Just thread

So you don't need the outer loop. You'd also make progressStop be the same as KickCompleted, simplifying things further.

[pepeiborra]

That code is pretty much equivalent to directProgressReporting introduced in this PR for tests. And this is a good thing, because in hindsight it seems a terrible idea to have one implementation for testing and a different one for production. So the path is clear - adapt directProgressReporting to be used for both and remove the old version.

[pepeiborra]

Thank you both for the careful review.

[pepeiborra]

This PR introduces a couple of new bugs:

1. I moved the sleep call in lspShakeProgress into the loop, so now it no longer delays the Begin message and it happens unconditionally. I believe our tests will not catch this.
2. I added a barrier to wait for the Begin response, but it doesn't look like lsp-test sends this response. It breaks a few tests, and if we use this in prod it could make the IDE less responsive unless done in a background thread.

Having one ProgressReporting for testing and another one for production is not really a good idea. I'll need to take another look, so moving to draft for now.

[wz1000]

I added a barrier to wait for the Begin response, but it doesn't look like lsp-test sends this response. It breaks a few tests, and if we use this in prod it could make the IDE less responsive unless done in a background thread.

I'm pretty sure I fixed this in haskell/lsp@5fa33f8#diff-8e90cf08428164fe5d7ef53f4d8e7900169c97415b3198f3040e75b96803e009

[pepeiborra]

I added a barrier to wait for the Begin response, but it doesn't look like lsp-test sends this response. It breaks a few tests, and if we use this in prod it could make the IDE less responsive unless done in a background thread.

I'm pretty sure I fixed this in haskell/lsp@5fa33f8#diff-8e90cf08428164fe5d7ef53f4d8e7900169c97415b3198f3040e75b96803e009

My bad, it's not lsp-test fault - it was a division by zero bug in the progress loop when todo is 0. This was likely the real issue identified in #1749 - I'll send a minimal fix in a separate PR

[pepeiborra]

Replaced by #1784