Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid couple partial functions in lsp-test #546

Closed

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Jan 22, 2024

Was anyone able to figure out why there are sometimes timeout messages (apparently unrelated to actual code changes) of the following king in hls test suite?

Test suite tests: RUNNING...
eval
  Property checking:                                            FAIL (66.02s)
    Timed out waiting to receive a message from the server.
    Last message received:
    {
        "jsonrpc": "2.0",
        "method": "$/progress",
        "params": {
            "token": "28",
            "value": {
                "kind": "end",
                "message": "Finished indexing 5 files"
            }
        }
    }
    Use -p '/Property checking/' to rerun this test only.

U briefly checked how lsp-test works and was surprised by the amount of partial functions used.
Here's a few easy to replace occurrences of fromJust + couple hard-to-resist hlint suggestions.

@@ -513,7 +513,7 @@ logMsg t msg = do
shouldColor <- asks $ logColor . config
liftIO $ when shouldLog $ do
when shouldColor $ setSGR [SetColor Foreground Dull color]
putStrLn $ arrow ++ showPretty msg
B.putStrLn $ arrow <> encodePretty msg
Copy link
Collaborator Author

@jhrcek jhrcek Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems pointless to convert pretty-printed json bytestring to String just to print it.
Wouldn't directly printing ByteString be less resource intensive?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, but seems sensible

@@ -421,7 +421,7 @@ sendRequest method params = do
reqMap <- requestMap <$> ask
liftIO $
modifyMVar_ reqMap $
\r -> return $ fromJust $ updateRequestMap r id method
\r -> return $ fromMaybe r $ updateRequestMap r id method
Copy link
Collaborator Author

@jhrcek jhrcek Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateRequestMap returns Nothing when request with given LspId is already in it.
I don't see how it makes sense to crash in such cases as opposed to returning the origial map unmodified. Similarly in the file below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite bad: that means we've somehow reused an ID which means stuff will break. So I think this probably should throw, but maybe not by using fromJust

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I'll play around with it locally to see if I can reproduce the me play around with how this works to see if I can reproduce the Timed out waiting to receive a message from the server. locally and then get back to this or close this.

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I do think it's a good idea to blow up somehow if we have reused ids, though.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Jan 22, 2024

Found this which answers my questions about test flakiness so closing this PR.

@jhrcek jhrcek closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants