[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:65
   Expected<uint64_t> EvalResult = FooVarUse.eval();
-  ASSERT_FALSE(EvalResult);
-  expectUndefError("FOO", EvalResult.takeError());
+  expectUndefErrors({"FOO"}, EvalResult.takeError());
 
----------------
thopre wrote:
> 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.
My bad. I was under the impression that takeError() would fail miserably if called for an Expected in a success state (just like trying to access the wrapped value for a failing Expected). I see that isn't the case.


================
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);
+  });
----------------
thopre wrote:
> 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?
Okay, that's fine. No need to add the additional context in most cases (though you might want a single test that shows location information too).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:49
                   Error Err) {
   handleAllErrors(std::move(Err), [&](const UndefVarError &E) {
+    EXPECT_EQ(ExpectedUndefVarNames.erase(E.getVarName()), 1U);
----------------
I just realised that this and similar patterns will trigger a llvm_unreachable, if the error isn't purely a single `UndefVarError` (or equivalent in other places). You probably either want a generic handler that handles all error types (in addition to the error-specific one), or you want to use `handleErrors` and check that the returned `Error` is an `ErrorSuccess()`, e.g. `EXPECT_THAT_ERROR(handleErrors(std::move(Err), /*exisitng handler*/...), Succeeded());`


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:137
+  bool ErrorHandled = false;
+  handleAllErrors(std::move(Err), [&](const ErrorT &E) {
+    EXPECT_NE(E.message().find(ExpectedMsg.str()), std::string::npos);
----------------
See my comment above about `handleAllErrors`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72914