[PATCH] D72914: [FileCheck] Strengthen error checks in unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:50
   handleAllErrors(std::move(Err), [&](const UndefVarError &E) {
-    ExpectedUndefVarNames.erase(E.getVarName());
+    EXPECT_GE(ExpectedUndefVarNames.erase(E.getVarName()), 1U);
   });
----------------
Are there any cases where you expet to remove more than one name? If not, I think `EXPECT_EQ` would be less confusing.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:65
   Expected<uint64_t> EvalResult = FooVarUse.eval();
-  ASSERT_FALSE(EvalResult);
-  expectUndefError("FOO", EvalResult.takeError());
+  expectUndefErrors({"FOO"}, EvalResult.takeError());
 
----------------
This will not behave like you want, I believe, if `EvalResult` is not a failure. You should `ASSERT_THAT_EXPECTED(EvalResult, Failed())` first, IIRC. Same goes elsewhere.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:161
   ParsedVarResult = Pattern::parseVariable(VarName, SM);
-  EXPECT_TRUE(errorToBool(ParsedVarResult.takeError()));
+  EXPECT_THAT_EXPECTED(ParsedVarResult, Failed());
 
----------------
Ideally, here and below you'd check that the returned error is the error you expect.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:206
+  handleAllErrors(std::move(Err), [&](const ErrorT &E) {
+    EXPECT_NE(E.message().find(ExpectedMsg.str()), std::string::npos);
+  });
----------------
Would `EXPECT_EQ(ExpectedMsg, E.message())`  be more precise?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:301
 
   // Invalid variable name.
+  expectDiagnosticError("invalid operand format '%VAR'",
----------------
You probably don't need some of these comments any more, as the expected error message documents the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72914