This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Strengthen error checks in unit tests
ClosedPublic

Authored by thopre on Jan 17 2020, 3:52 AM.

Details

Summary

This commit adds error checking beyond UndefVarError and fix a number of
Error/Expected related idioms:

  • use (EXPECT|ASSERT)_THAT_(ERROR|EXPECTED) instead of errorToBool or boolean operator
  • ASSERT when a further check require the check to be successful to give a correct result

Event Timeline

thopre created this revision.Jan 17 2020, 3:52 AM
jhenderson added inline comments.Jan 20 2020, 1:56 AM
llvm/unittests/Support/FileCheckTest.cpp
50

Are there any cases where you expet to remove more than one name? If not, I think EXPECT_EQ would be less confusing.

65

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.

175

Ideally, here and below you'd check that the returned error is the error you expect.

220

Would EXPECT_EQ(ExpectedMsg, E.message()) be more precise?

299

You probably don't need some of these comments any more, as the expected error message documents the case.

thopre updated this revision to Diff 239158.Jan 20 2020, 9:57 AM
thopre marked 7 inline comments as done.

Address all review comments but the substring match when expecting errors

llvm/unittests/Support/FileCheckTest.cpp
65

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.

220

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?

299

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)

thopre updated this revision to Diff 239202.Jan 20 2020, 3:07 PM

Add missing error checks

thopre updated this revision to Diff 239203.Jan 20 2020, 3:29 PM

Add still missing error checks...

jhenderson added inline comments.Jan 21 2020, 2:13 AM
llvm/unittests/Support/FileCheckTest.cpp
49

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());

65

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.

137

See my comment above about handleAllErrors

220

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).

thopre updated this revision to Diff 239373.Jan 21 2020, 10:28 AM
thopre marked 5 inline comments as done.

Address all review comments but the location information

thopre added inline comments.Jan 21 2020, 10:31 AM
llvm/unittests/Support/FileCheckTest.cpp
220

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.

jhenderson accepted this revision.Jan 23 2020, 1:27 AM

LGTM.

llvm/unittests/Support/FileCheckTest.cpp
220

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.

This revision is now accepted and ready to land.Jan 23 2020, 1:27 AM
This revision was automatically updated to reflect the committed changes.
thopre marked 2 inline comments as done.