Page MenuHomePhabricator

[FileCheck] Clean and improve unit tests
ClosedPublic

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

Details

Summary

Clean redundant unit test checks (codepath already tested elsewhere) and
add a few missing checks for existing numeric substitution and match
logic.

Diff Detail

Event Timeline

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

It's not clear to me exactly what the phrase "for the variable defined to become visible" means. What does it mean to be "visible"? Also, I suspect you mean "the defined variable" or even just "the variable".

308–309

Again, what does "visible" mean? Also "make it visible" -> "makes it visible".

330

Acronyms like this should generally be capitalized, i.e. "RHS".

334

This case feels like it's covering two bases, making it unclear what the actual cause of failure is. "%VAR" is not a valid name, right, plus I thought @LINE didn't support variables. Should this be "@LINE+FOO"?

Of course, looking below makes me think I'm wrong, which suggests the comment needs updating to say why the RHS is invalid.

342–344

I'm concerned that these are getting mixed up with the @LINE test cases. The first at least should probably be separate and earlier. The latter might belong somewhere around here however.

353

It's not clear to me how this and the other deleted test cases are covered elsewhere?

401

the variable defined -> a defined variable

443–444

return its value -> match their value

447–449

What's the 30 here?

552

Should this 2 and the 2 below in the parseNumericSubstitution block match? If so, consider pulling them both out into a separate variable. Same applies below.

thopre updated this revision to Diff 239068.Jan 20 2020, 4:15 AM
thopre marked 14 inline comments as done.

Address review comments

llvm/unittests/Support/FileCheckTest.cpp
334

For backward compability legacy @LINE expression (i.e. @LINE used in a [[]] substitution block that doesn't start by "[[#") can only be @LINE, @LINE+offset or @LINE-offset where offset is a decimal literal. Numeric substitution block can use @LINE in any way.

353

For the unit tests I'm doing white box testing. Since anything starting with [[# gets handed over to parseNumericSubstitutionBlock I keep all testing for such expression in the ParseNumericSubstitutionBlock test. I think so far I've done a mix of black box and white box testing and would try to test the interface but then you get a combinatorial explosion. For consistency you would also need to tests again the same patterns in defineCmdlineVariables().

443–444

I'm not testing match here, only substitution. I've thus rephrased accordingly.

447–449

Some arbitrarily chosen insertIdx value. This holds the index of the substitution block in the CHECK string to insert the substituted value at the right place in the string.

jhenderson accepted this revision.Jan 20 2020, 5:38 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jan 20 2020, 5:38 AM
This revision was automatically updated to reflect the committed changes.