[clangd] Fixes in lit tests

[ArcsinX]

Summary

Changes:

• background-index.test Add Windows support, don't create redundant *-e files on macOS
• did-change-configuration-params.test Replace cat | FileCheck with FileCheck --input-file
• test-uri-windows.test This test did not run on Windows displite REQUIRES: windows-gnu || windows-msvc (replacement: UNSUPPORTED: !(windows-gnu || windows-msvc)).

[ArcsinX]

What do you think of this patch? I'm not sure if Windows is important OS for developers, and that some of these changes might be awkward to maintain, but maybe we can enable background-index.test at least?

[njames93]

In D83759#2163622, @ArcsinX wrote:

What do you think of this patch? I'm not sure if Windows is important OS for developers.

Windows is most certainly an important OS for developers and something the whole llvm project has been trying to catch up on where possible.

[kadircet]

hi, sorry for the late replay :/

We discussed this offline with Sam and are both concerned with the duplications it introduces, haven't really thought about a nice way to address this yet.

As you mentioned the background-index test in itself looks a lot better do, so we can land that one separately and think about the rest in the meantime.

clang-tools-extra/clangd/test/background-index.test

17–18

are you sure these don't need changes on windows bots ? AFAICT this contains both backslashes(coming from %t) and forwards slashes, and command is ls which might not be available on a default windows prompt.

[ArcsinX]

clang-tools-extra/clangd/test/background-index.test

17–18

I am not sure. How I can check it? The only thing I can say, that I tested this on mingw and visual studio builds on my local machine.

this contains both backslashes(coming from %t) and forwards slashes,

For me that was OK, but I agree that ls %t/.cache/clangd/index/foo.cpp.*.idx should be replaced with ls %/t/.cache/clangd/index/foo.cpp.*.idx.

As for ls, it's a part of mingw, so I think ls absence probability is the same as for sed

[kadircet]

clang-tools-extra/clangd/test/background-index.test

17–18

I am not sure. How I can check it? The only thing I can say, that I tested this on mingw and visual studio builds on my local machine.

unfortunately there is no good way (that I know of) for checking these, apart from landing the patch and monitoring the buildbots(http://lab.llvm.org:8011/) for breakages and reverting/fixing forward accordingly.

[ArcsinX]

Revert tests split; Replace '\' with '/' in background-index.test.

[kadircet]

thanks, LGTM!

as i mentioned please watch out for buildbot breakages after landing this. (and again let me know if I should land this for you)

[ArcsinX]

In D83759#2164296, @kadircet wrote:

as i mentioned please watch out for buildbot breakages after landing this. (and again let me know if I should land this for you)

Thanks for review.

I got commit access recently and will try to commit by myself.

[thakis]

Looks like this breaks tests on macOS (and probably with non-gnu greps): http://45.33.8.238/mac/17611/step_9.txt

Please take a look, and revert while you investigate if it takes a while to fix.

[ArcsinX]

In D83759#2165116, @thakis wrote:

Looks like this breaks tests on macOS (and probably with non-gnu greps): http://45.33.8.238/mac/17611/step_9.txt

Please take a look, and revert while you investigate if it takes a while to fix.

Reverted.

Seems the problem is in sed without gnu extensions, but I wonder how other tests with similar sed command could pass (e.g. compile-commands-path-in-initialize.test)

[ArcsinX]

Seem the problem is in -i option.
According with OSX man:
sed [-Ealn] command [file ... ]
sed [-Ealn] [-e command] [-f command_file] [-i extension] [file ...].

Seems on macOS -E is treated as an argument for -i in command like sed -i -E -e ... .

Also, seems commands like sed -i -e ... are unsafe on macOS. Such commands will create backup with name <filename>-e, but I do not have macOS to check it.
@thakis could you please verify that after tests you have extra <filename>-e files in /Users/thakis/src/llvm-project/out/gn/obj/clang-tools-extra/clangd/test/Output/background-index.test.tmp/ ?

I think we could avoid -i usage to fix this problem.

[kadircet]

In D83759#2165974, @ArcsinX wrote:

Seem the problem is in -i option.
According with OSX man:
sed [-Ealn] command [file ... ]
sed [-Ealn] [-e command] [-f command_file] [-i extension] [file ...].

Seems on macOS -E is treated as an argument for -i in command like sed -i -E -e ... .

Also, seems commands like sed -i -e ... are unsafe on macOS. Such commands will create backup with name <filename>-e, but I do not have macOS to check it.
@thakis could you please verify that after tests you have extra <filename>-e files in /Users/thakis/src/llvm-project/out/gn/obj/clang-tools-extra/clangd/test/Output/background-index.test.tmp/ ?

I think we could avoid -i usage to fix this problem.

your diagnosis seems correct. in addition to avoiding -i altogether, you can try passing a value to it. e.g. sed -i.bak then both linux and macos should hopefully pick it up and the following -e should be interpreted correctly.

[ArcsinX]

[macOS] Fix background-index.test failure, don't create *-e files during background-index.test

[ArcsinX]

I believe it's okay now on macOS.

[kadircet]

clang-tools-extra/clangd/test/background-index.test

6

i don't think there's much point in dropping -i from only some commands.

I would suggest either:

• dropping it from all, by changing the original files to have a .tmpl suffix, and dropping the suffix in sed'd versions
• keeping it for all comments by adding a -i.bak to all

[ArcsinX]

clang-tools-extra/clangd/test/background-index.test

6

For definition.jsonrpc we have two sed commands and we can keep the file name unchanged without -i option.
But for compile_commands.json we have only one sed command and we need -i option to keep the file name unchanged.

changing the original files to have a .tmpl suffix

In other tests .1 suffix is used, that's why I use this suffix here.
E.g. compile-commands-path-in-initialize.test

# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
...
# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %t.test.1 > %t.test

[kadircet]

clang-tools-extra/clangd/test/background-index.test

6

For definition.jsonrpc we have two sed commands and we can keep the file name unchanged without -i option.

yes for that one we could still move into a .1 (or .tmp) suffixed version.

In other tests .1 suffix is used, that's why I use this suffix here.

right, I was talking about renaming clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc to clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc.tmpl (likewise the compile_commands.json). That way you can always do a sed, and write to a different file (i.e. the version without the suffix).

[ArcsinX]

Get rid of -i sed option

[ArcsinX]

clang-tools-extra/clangd/test/background-index.test

6

Got it, fixed, thanks.

[kadircet]

thanks, LGTM.

clang-tools-extra/clangd/test/background-index.test

-# We need to splice paths into file:// URIs for this test.
-# UNSUPPORTED: windows-msvc
-
 # Use a copy of inputs, as we'll mutate it (as will the background index).
-# RUN: rm -rf %t
-# RUN: cp -r %S/Inputs/background-index %t
+# RUN: rm -rf %/t
+# RUN: cp -r %/S/Inputs/background-index %/t
 # Need to embed the correct temp path in the actual JSON-RPC requests.
-# RUN: sed -i -e "s|DIRECTORY|%t|" %t/definition.jsonrpc
-# RUN: sed -i -e "s|DIRECTORY|%t|" %t/compile_commands.json
+# RUN: sed -e "s|DIRECTORY|%/t|" %/t/definition.jsonrpc.tmpl > %/t/definition.jsonrpc.1
+# RUN: sed -e "s|DIRECTORY|%/t|" %/t/compile_commands.json.tmpl > %/t/compile_commands.json
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.1 > %/t/definition.jsonrpc
 
 # We're editing bar.cpp, which includes foo.h.
 # foo() is declared in foo.h and defined in foo.cpp.
 # The background index should allow us to go-to-definition on foo().
 # We should also see indexing progress notifications.
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,BUILD
+# RUN: clangd -background-index -lit-test < %/t/definition.jsonrpc | FileCheck %/t/definition.jsonrpc --check-prefixes=CHECK,BUILD
 
 # Test that the index is writing files in the expected location.
-# RUN: ls %t/.cache/clangd/index/foo.cpp.*.idx
-# RUN: ls %t/sub_dir/.cache/clangd/index/foo.h.*.idx
+# RUN: ls %/t/.cache/clangd/index/foo.cpp.*.idx
+# RUN: ls %/t/sub_dir/.cache/clangd/index/foo.h.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,USE
+# RUN: rm %/t/foo.cpp
+# RUN: clangd -background-index -lit-test < %/t/definition.jsonrpc | FileCheck %/t/definition.jsonrpc --check-prefixes=CHECK,USE

clang-tools-extra/clangd/test/did-change-configuration-params.test

 # RUN: clangd -compile_args_from=lsp -lit-test < %s 2> %t | FileCheck -strict-whitespace %s
-# RUN: cat %t | FileCheck --check-prefix=ERR %s
+# RUN: FileCheck --check-prefix=ERR --input-file=%t %s
 # UNSUPPORTED: windows-gnu,windows-msvc
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test", "compilationCommand": ["clang", "-c", "foo.c"]}}}}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main() { int i; return i; }"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {

clang-tools-extra/clangd/test/test-uri-windows.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-# REQUIRES: windows-gnu || windows-msvc
+# UNSUPPORTED: !(windows-gnu || windows-msvc)
 # Test authority-less URI
 {"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:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 # CHECK:    "uri": "file:///C:/clangd-test/foo.c"
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}