Page MenuHomePhabricator

[clangd] Add a TestWorkspace utility

Authored by nridge on Oct 12 2020, 11:37 PM.



TestWorkspace allows easily writing tests involving multiple
files that can have inclusion relationships between them.

BackgroundIndexTest.RelationsMultiFile is refactored to use
TestWorkspace, and moved to FileIndexTest as it no longer
depends on BackgroundIndex.

Diff Detail

Unit TestsFailed

4,130 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.AddFiles
Note: Google Test filter = DirectoryWatcherTest.AddFiles [==========] Running 1 test from 1 test case.
3,850 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.DeleteFile
Note: Google Test filter = DirectoryWatcherTest.DeleteFile [==========] Running 1 test from 1 test case.
4,040 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.ModifyFile
Note: Google Test filter = DirectoryWatcherTest.ModifyFile [==========] Running 1 test from 1 test case.

Event Timeline

nridge created this revision.Oct 12 2020, 11:37 PM
nridge requested review of this revision.Oct 12 2020, 11:37 PM

This patch is a spinoff from my call hierarchy work (which I haven't posted for review yet, though I posted a draft at D89296).

I'd like to be able to write tests where I can:

  • specify the contents of multiple header and source files, which may have #include relationships among them
  • index the files
  • create an AST for one or more of these files
  • run operations that require an AST + the index and make assertions about their results.

The TestWorkspace utility in this patch is my attempt to allow writing tests of this form without a lot of boilerplate. I also use it in one existing test, BackgroundIndexTest.RelationsMultiFile, though this use is not the most interesting as it does not involve ASTs. There is a more interesting use in D89298 for writing a call hierarchy test that does use ASTs.

I am seeking guidance on the following:

  • Is this a useful utility to have? I've certainly found it useful for writing call hierarchy tests.
  • Is it OK that it depends on BackgroundIndex? In principle, it should be possible to make the same interface work with FileIndex under the hood, but I haven't figured out how. (Specifically, I haven't figured out how to get FileIndex to resolve #includes, because you can't pass a MockFS to it the way you can to BackgroundIndex.)
  • If we keep the dependency on BackgroundIndex, is it OK to move the related dependencies (like MemoryShardStorage) to a header, so that TestWorkspace itself can live in a header and tests from different files (not just BackgroundIndexTest.cpp) can use it?

In principle, it should be possible to make the same interface work with FileIndex under the hood, but I haven't figured out how. (Specifically, I haven't figured out how to get FileIndex to resolve #includes, because you can't pass a MockFS to it the way you can to BackgroundIndex.)

I should mention for completeness that the way we did tests like this in Eclipse CDT is... we actually wrote the test files to disk :) But I'm guessing, given the efforts that have been made to use MockFS in the test suite so far, that we don't want to do this for clangd.

This looks like a useful infra to have indeed, we currently don't have an easy way to setup a testcase that would require interactions between multiple files (e.g. a workspace), as you've noticed while working on callhierarchy tests (sorry for that).

A couple of suggestions from my side:

  • Rather than enforcing files to come in header/source pairs, why not have a isTU flag. That way we can use the infra in a more flexible way.
  • Instead of having a MockFS, maybe have a single TestTU with all the workspace files put into AdditionalFiles. Later on when building an AST, you can just replace the TestTU::Code and build the source as if it is in the workspace.
  • Background or FileIndex is not the point handling include resolution (or AST build process). In theory during the construction time, after populating all the AdditionalFiles in a TestTU, you can have a single FileIndex and populated it for each TU using the AST coming from TestTU::build() and preamble from TestTU::preamble(). Currently TestTU::preamble() doesn't take a callback, but you can change that to populate the FileIndex using preamble information.
  • In addition to having a constructor that takes all the files(or just some base files) at once, it might be better to have an interface like:
struct WorkspaceTest {
  void addSource(FilePath, Code);  // These are just sourcefiles, ASTs can be built for them, but they won't be MainFiles during indexing.
  void addMainFile(FilePath, Code); // These are entrypoints, will be marked as TU, and would be used for index builds.
  std::unique_ptr<SymbolIndex> index(); // Builds the index for whole forkspace, by indexing all the MainFiles and merging them under a single FileIndex.
  ParsedAST ast(FilePath);
nridge updated this revision to Diff 298926.Oct 18 2020, 10:09 PM

Rework utility as suggested

Thanks for the suggestions! I implemented them and it seems to be working well.

thanks this mostly looks good.

can you move implementations from TestWorkspace.h to TestWorkspace.cpp ?

431 ↗(On Diff #298926)

nit: if you decide to keep the constructor can you prepend /*IsMainFile=*/ to last parameters?

12 ↗(On Diff #298926)

s/AST-based/AST based/

32 ↗(On Diff #298926)

i think it would be better to move this struct to private, and only have addSoruce/addMainFile helpers with comments explaining only TUs rooted at mainfiles will be indexed.

if you prefer to keep this struct then we should probably have comments about it too. especially the IsMainFile bit.

46 ↗(On Diff #298926)

nit: Filename.str() (same for Code and usages in addMainFile below.

56 ↗(On Diff #298926)

nit: prefer early exit, if(!MainFile) continue;

75 ↗(On Diff #298926)

should this be a failure instead? e.g. ADD_FAILURE() << "Accessing non-existing file: " << Filename;

77 ↗(On Diff #298926)

nit: Input->Filename

82 ↗(On Diff #298926)

nit: maybe move filename out of the struct, and keep a map here instead?

nridge edited the summary of this revision. (Show Details)Oct 20 2020, 5:23 PM
nridge updated this revision to Diff 299526.Oct 20 2020, 5:49 PM
nridge marked 8 inline comments as done.

Address review comments

Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 5:49 PM
kadircet accepted this revision.Oct 21 2020, 3:42 AM

thanks, lgtm!

17 ↗(On Diff #299526)

nit: redundant braces

32 ↗(On Diff #299526)

nit: I would move this near the stringmap.

38 ↗(On Diff #299526)

drop this.

99 ↗(On Diff #299526)

these are usually picked up by gn-build-bots and auto-updated. but it shouldn't hurt to do that manually as well (i hope)

This revision is now accepted and ready to land.Oct 21 2020, 3:42 AM
nridge updated this revision to Diff 300513.Oct 24 2020, 5:14 PM
nridge marked 4 inline comments as done.

address final review comments

This revision was landed with ongoing or failed builds.Oct 24 2020, 5:15 PM
This revision was automatically updated to reflect the committed changes.