[clangd][test] Fix exit messages in tests

[jkorous]

Summary

There's a small typo in tests - causing that we aren't sending exit LSP message to clangd but crashing it instead with the last message not being valid JSON and clangd hitting unexpected EOF in runLanguageServerLoop() right after. Seems like this bug even managed to spawn some offsprings via copy-pasting. I just found it by accident since I am not closing stdin after the last message when using XPC adapter and my test was hanging - waiting for clangd to exit.

It's not a big deal but I got two ideas.

1. Maybe we should somehow check stderr from clangd - in this case there was JSON parse error present but tests didn't notice.
2. When fixing that I got surprised by shutdown-without-exit.test implementation. It seems more like exit-without-shutdown to me and makes sense that we still want to exit (with non-success return value) in such case. Was this the intention?

What do you think?

[arphaman]

Thanks for fixing this!

You're right we should try to fix it properly to avoid such mistakes in the future. Checking for stderr from Clangd might work, but I don't think it's the most optimal solution. What do you think about Clangd exiting with failure on malformed JSON input when running in -lit mode?

[jkorous]

I think that by introducing different codepath for testing purposes we might end up with fewer bugs in tests but the actual production code could become less tested. Actually, even the -lit-test itself might be not the theoretically most correct approach but I do see the practical reason for it. In general I'd rather keep the testing specific code to a minimum though.

What we might be able to do specifically for this case is to return false iff we hit unexpected EOF in clangd::runLanguageServerLoop() (currently void) and || it with ShutdownRequestReceived in ClangdLSPServer::run().

I still see some value in monitoring clangd's stderr in tests in general though. I can imagine something simple like redirection of clangd's stderr to a file and just grepping the log for lines starting with "E[" and in case of some set of errors being expected/allowed for specific test case then grepping for negation (grep -v expected_error). There might be some work necessary to either recalibrate log levels or add allowed errors to large part of tests. For example clangd currently logs this error for most tests:

E[11:24:49.910] Failed to get status for RootPath clangd: No such file or directory

[ilya-biryukov]

LGTM. Many thanks for fixing this.

Adding some failure monitoring seems like a nice idea. On the other hand, polluting every test with stderr redirection doesn't look like a nice idea.
As a middle ground, I could imagine some extra checking being done if -lit-test is passed, e.g. returning a non-zero error code on request parsing errors would trigger test failures in these particular cases, right?

[ilya-biryukov]

Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code

[jkorous]

I can see the value of getting more information in case of unexpected test behaviour but I still don't really like to have separate codepath for testing purposes.

Anyway, it's not a big deal and it looks like you guys are all in agreement about this.

I created a patch that I will put for review (rebuilding and running tests overnight).

clangd/extra-flags.test

 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
 
 

clangd/formatting.test

 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        }
 # CHECK-NEXT:      }
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","id":6,"method":"shutdown"}
 ---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}

clangd/initialize-params.test

 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": null
 ---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}

clangd/shutdown-with-exit.test

 # RUN: clangd -lit-test < %s
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}

clangd/shutdown-without-exit.test

 # RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}

clangd/unsupported-method.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":""}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/jumpInTheAirLikeYouJustDontCare","params":{}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32601,
 # CHECK-NEXT:    "message": "method not found"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0"
 ---
 {"jsonrpc":"2.0","id":2,"method":"shutdown"}
 ---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}