[PATCH] D65525: [clangd] Add new helpers to make tweak tests scale better. Convert most tests. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Neat, thanks for the cleanup! A few suggestions from me, no major comments.



================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.cpp:74
+    return true;
+  llvm::consumeError(PrepareResult.takeError());
+  return false;
----------------
Maybe print the input in case of failure?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:9
+
+#include "TestTU.h"
+#include "gtest/gtest.h"
----------------
NIT: add an include guard


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:44
+    // Snippet is an expression.
+    Expression,
+  };
----------------
I wonder whether we could use `Function` and get rid of 'expression' mode completely? WDYT?
One can easily turn an expression into a statement by adding a semicolon.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:86
+  for (const auto &Case : expandCases(MarkedCode))                             \
+  EXPECT_THAT(Case, isAvailable())
+
----------------
NIT: add a level of indent


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:86
+  for (const auto &Case : expandCases(MarkedCode))                             \
+  EXPECT_THAT(Case, isAvailable())
+
----------------
ilya-biryukov wrote:
> NIT: add a level of indent
NIT: fully-qualify in a macro `::clang::clangd::TweakTest::isAvailable()`


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:90
+  for (const auto &Case : expandCases(MarkedCode))                             \
+  EXPECT_THAT(Case, ::testing::Not(isAvailable()))
+
----------------
NIT: add a level of indent


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:36
 
+// TODO(sammccall): migrate the rest of the tests to use TweakTesting.h and
+// remove these helpers.
----------------
NIT: s/TODO/FIXME
the latter is used more widely in clangd codebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65525