[PATCH] D121286: [clangd] Test against path insensitivity

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TestFS.cpp:76
 #ifdef _WIN32
-  return "C:\\clangd-test";
+  // On windows paths are insensitive, simulate that behaviour by toggling
+  // capitalization for drive-letter on each call.
----------------
This is clever, but I don't think it's a good idea.

It's very surprising given the contract and historical behavior of this function, and can lead to spurious test failures in some legitimate uses, e.g in IncludeCleanerTests:
```
  // Note we deduped the names as _number_ of <built-in>s is uninteresting.
  EXPECT_THAT(ReferencedFileNames.keys(),
              UnorderedElementsAre("<built-in>", "<scratch space>",
                                   testPath("macros.h")));
```
(Not sure why this isn't failing, luck? Maybe you'll get more failures by adding more possible case variations?)
I don't think the extra verbosity to make such tests resilient to case changes is an improvement.

The paths coming from testRoot get propagated around a lot, so lots of strings start having unpredictable case (which is the point!). Nevertheless I'm not confident many of them will be effectively tested, because the paths compared need to originate through independent calls to testRoot() rather than e.g. both resulting from relative paths resolved against a CWD set to testRoot() called once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121286/new/

https://reviews.llvm.org/D121286