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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
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:
> > 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).
> Is the intent of a single location check to verify that a given type of error provide location information or to check for a given error that the location is correct?
> 
> If the former I can perhaps use some regex check in expectError and check for a location info for all messages. I'm not sure what we gain if the intent is the latter. Besides, location information in error message is generally already tested in the standard LIT testsuite.
I think if there's sensible lit testing, then that's fine. We should have testing that shows location information is correct somewhere (and depending on how the code path works, it might be appropriate to test each error separately), but there's no need for both lit and unit testing for it, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72914