[PATCH] D50641: [clangd][test] Fix exit messages in tests
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339781: [clangd][tests] Fix typo in tests - invalid LSP exit
message (authored by jkorous, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50641?vs=160367&id=160819#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50641
Files:
clang-tools-extra/trunk/test/clangd/extra-flags.test
clang-tools-extra/trunk/test/clangd/formatting.test
clang-tools-extra/trunk/test/clangd/initialize-params.test
clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
clang-tools-extra/trunk/test/clangd/unsupported-method.test
Index: clang-tools-extra/trunk/test/clangd/extra-flags.test
===
--- clang-tools-extra/trunk/test/clangd/extra-flags.test
+++ clang-tools-extra/trunk/test/clangd/extra-flags.test
@@ -49,6 +49,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/formatting.test
===
--- clang-tools-extra/trunk/test/clangd/formatting.test
+++ clang-tools-extra/trunk/test/clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/unsupported-method.test
===
--- clang-tools-extra/trunk/test/clangd/unsupported-method.test
+++ clang-tools-extra/trunk/test/clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/extra-flags.test
===
--- clang-tools-extra/trunk/test/clangd/extra-flags.test
+++ clang-tools-extra/trunk/test/clangd/extra-flags.test
@@ -49,6 +49,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/formatting.test
===
--- clang-tools-extra/trunk/test/clangd/formatting.test
+++ clang-tools-extra/trunk/test/clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/unsupported-method.test
===
--- clang-tools-extra/trunk/test/clangd/unsupported-method.test
+++ clang-tools-extra/trunk/test/clangd/
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous added a comment. 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). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
ilya-biryukov added a comment. Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. 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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous added a comment. 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
arphaman added a comment. 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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous created this revision.
jkorous added reviewers: sammccall, ilya-biryukov.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, arphaman, dexonsmith, MaskRay, ioeric.
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?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50641
Files:
clangd/extra-flags.test
clangd/formatting.test
clangd/initialize-params.test
clangd/shutdown-with-exit.test
clangd/shutdown-without-exit.test
clangd/unsupported-method.test
Index: clangd/unsupported-method.test
===
--- clangd/unsupported-method.test
+++ clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-without-exit.test
===
--- clangd/shutdown-without-exit.test
+++ clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-with-exit.test
===
--- clangd/shutdown-with-exit.test
+++ clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/initialize-params.test
===
--- clangd/initialize-params.test
+++ clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/formatting.test
===
--- clangd/formatting.test
+++ clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/extra-flags.test
===
--- clangd/extra-flags.test
+++ clangd/extra-flags.test
@@ -47,6 +47,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/unsupported-method.test
===
--- clangd/unsupported-method.test
+++ clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-without-exit.test
===
--- clangd/shutdown-without-exit.test
+++ clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-with-exit.test
===
--- clangd/shutdown-with-exit.test
+++ clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/initialize-params.test
===
--- clangd/initialize-params.test
+++ clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/formatting.test
===
--- clangd/formatting.test
+++ clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/extra-flags.test
===
--- clangd/extra-flags.t
