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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
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());
 
----------------
jhenderson wrote:
> 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.
This is already caught because expectUndefErrors checks that all the expects undefined variable have been found. I've just tested it on a success value and it fails in the line:

EXPECT_TRUE(ExpectedUndefVarNames.empty()) << toString(ExpectedUndefVarNames);

I've added logic in expectError to achieve the same thing. This avoids one check before every call to expectError.


================
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);
+  });
----------------
jhenderson wrote:
> Would `EXPECT_EQ(ExpectedMsg, E.message())`  be more precise?
A lot of the diagnostics span several lines because they come with a location information. Matching the exact message means you'd have to provide all this in the string to check against. I felt that the error message itself was enough to check which requires a substring match.

What do you think?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:301
 
   // Invalid variable name.
+  expectDiagnosticError("invalid operand format '%VAR'",
----------------
jhenderson wrote:
> You probably don't need some of these comments any more, as the expected error message documents the case.
I've removed many but kept that specific one because the error message is the same for several different errors related to operand. I want to be clear on what error specifically I'm testing (i.e. what code path I'm checking is taken)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72914