This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add lit tests for remote index
ClosedPublic

Authored by kbobyrev on Oct 28 2020, 2:34 AM.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 28 2020, 2:34 AM
kbobyrev requested review of this revision.Oct 28 2020, 2:34 AM
kbobyrev updated this revision to Diff 301268.Oct 28 2020, 6:50 AM

Fix test except shutting down part :(

kbobyrev updated this revision to Diff 301298.Oct 28 2020, 8:32 AM

Push the Franken-pytest. Benefits: doesn't fail, is platform independent
(to be determined).

kbobyrev updated this revision to Diff 301303.Oct 28 2020, 9:05 AM

Don't change the existing Python formatting.

kadircet added inline comments.Oct 29 2020, 3:22 AM
clang-tools-extra/clangd/test/remote-index/pipeline.test
2

nit: / shouldn't be needed here.

the difference between %t and %/t is the latter is expressed with forward slashes on every platform.

3

nit: s/.in.dex/.idx/

nit: maybe create a new Inputs directory under test/remote-index and move the files there?

4

use %python instead to enable lit substitution, just in case there are weird build bots without python on the PATH or the default one pointing at a wrong version.

can we use 127.0.0.1:50051 instead? as starting a server on 0.0.0.0 might be blocked on some bots again. And also I would rather use a random port number (greater than 1024, possibly by letting the python script choose the number, and getting rid of the argument completely), to lower the chances of failing accidentally when 50051 is in use. (it might be because bot is running multiple tests in parallel, or bot itself has a grpc server running on default port for whatever reason).

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
26 ↗(On Diff #301303)

can we rename test-directory to project-root and pass it directly from lit ?

this will also contain \\ when run on windows.

32 ↗(On Diff #301303)

do we really need the abspath conversion here ? is it to get rid of .. ? i am uneasy about this as the call is likely to introduce \\.

36 ↗(On Diff #301303)

can we instead wait until server prints Server listening on ... ?

47 ↗(On Diff #301303)

can we just wait until clangd process shuts down ?

ArcsinX added inline comments.
clang-tools-extra/clangd/test/remote-index/pipeline.test
4

can we use 127.0.0.1:50051 instead?

I think we should do this, because we also pass this address into --remote-index-address= clangd parameter, which is illegal. Seems Linux and Mac allow to call connect with 0.0.0.0 as a destination address, but Windows does not and this test fails.

kbobyrev updated this revision to Diff 301606.Oct 29 2020, 7:02 AM
kbobyrev marked 8 inline comments as done.

Resolve review comments.

kbobyrev updated this revision to Diff 301607.Oct 29 2020, 7:02 AM

Rebase on top of master.

kbobyrev added inline comments.Oct 29 2020, 7:39 AM
clang-tools-extra/clangd/test/remote-index/pipeline.test
4

Thanks for checking! Can you try run this test now and see if it works?

ArcsinX added inline comments.Oct 29 2020, 7:51 AM
clang-tools-extra/clangd/test/remote-index/pipeline.test
4

Is there any reason to use %/S in --project-root? We support --project-root with native slashes (at least --project-root=%S works for me on Windows + MS compiler)

It looks more reasonable to use %/S in paths like this %/S/Inputs/Source to avoid mixing slashes on Windows (but it works correctly with mixing slashes anyway)

4

It works ok now.

kbobyrev updated this revision to Diff 301619.Oct 29 2020, 7:54 AM
kbobyrev marked an inline comment as done.

Rebase on top of master, don't force forward slashes in project root,
format Python helper.

kadircet accepted this revision.Oct 30 2020, 2:26 AM

thanks, lgtm!

clang-tools-extra/clangd/test/lit.cfg.py
26

we should rather call lit.llvm.llvm_config.use_default_substitutions(), preferably right after use_clang() on line 4.

sorry for the churn, i thought we were already doing this.

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
19 ↗(On Diff #301619)

nit: maybe perform the magic after arg parsing ?

22 ↗(On Diff #301619)

this still binds the socket to all interfaces, can we specify localhost or 127.0.0.1 explicitly for the interface address.

36 ↗(On Diff #301619)

nit: formatting looks off, is this really what yapf offers?

39 ↗(On Diff #301619)

i would rather scan through all lines and look for Server listening on $server_addres. We should be able to either hit the end of stream (i.e. server encounters a failure and shuts down) or find the log message we are looking for.

just to make test less reliant on log ordering.

51 ↗(On Diff #301619)

can we rather use signal.SIGXXX here instead of 9 ?

Also rather than kill, SIGINT might be more applicable. https://docs.python.org/3/library/signal.html#signal.SIGKILL claims sigkill is not available on windows.

This revision is now accepted and ready to land.Oct 30 2020, 2:26 AM
kbobyrev updated this revision to Diff 302213.Nov 1 2020, 11:37 PM
kbobyrev marked 6 inline comments as done.

Resolve post-LGTM commentts, increase index hot reload frequency.

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
36 ↗(On Diff #301619)

Yep, I'm formatting with it :(

39 ↗(On Diff #301619)

Unfortunately, it's not as easy :( Then I would have to set a time to wait for the server to warm up and read lines then only. The problem here is that the server runs, doesn't shut down but also doesn't print the message. We'll hang the buildbots then.

But I'll go with the timeout option. I don't think it's better than checking the first line as the initialize one but still.

This revision was landed with ongoing or failed builds.Nov 1 2020, 11:40 PM
This revision was automatically updated to reflect the committed changes.
kbobyrev added inline comments.Nov 1 2020, 11:41 PM
clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
51 ↗(On Diff #301619)

The problem with SIGINT is that it needs to wait for the hot reload sync to finish :) I'll add a patch to modify the frequency via CLI to avoid long test terminaiton.

kadircet added inline comments.Nov 1 2020, 11:57 PM
clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
42 ↗(On Diff #302213)

so if we think .readline() is blocking but this is not, can't we just keep calling readline instead?

39 ↗(On Diff #301619)

Unfortunately, it's not as easy :( Then I would have to set a time to wait for the server to warm up and read lines then only

Why? isn't the readline() on stderr blocking ? If not how was this code working without sleep before?

The problem here is that the server runs, doesn't shut down but also doesn't print the message. We'll hang the buildbots then.

If the argument above is correct, this was the case already, i.e. stderr.readline() would block until a line is available or server had shutdown.

To overcome this problem I would suggest having a separate thread that would kill the server if it is still running after N seconds.

hans added a subscriber: hans.Nov 2 2020, 4:46 AM
hans added inline comments.
clang-tools-extra/clangd/test/lit.site.cfg.py.in
11

Could this use PYTHON_EXECUTABLE instead? I see other tests using that, and Python3_EXECUTABLE might not always be set.

kadircet added inline comments.Nov 2 2020, 6:20 AM
clang-tools-extra/clangd/test/lit.site.cfg.py.in
11

i was checking llvm's lit.site.cfg.py.in for reference and it was using python3 too.

any specific reasons for using PYTHON_EXECUTABLE instead of python3? I can't say this was a strong preference, but the python script included might not be python2 compliant, hence the need for 3.

If there is a compelling reason for using a possibly python2 executable, we can try to make the script both 2 and 3 compliant.

hans added inline comments.Nov 2 2020, 6:28 AM
clang-tools-extra/clangd/test/lit.site.cfg.py.in
11

i was checking llvm's lit.site.cfg.py.in for reference and it was using python3 too.

Oh interesting, I hadn't seen that. Well then using it here too makes sense I suppose. Sorry for the noise :)