Page MenuHomePhabricator

[FileCheck] Clean and improve unit tests

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



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

Diff Detail

Event Timeline

thopre created this revision.Jan 17 2020, 3:51 AM
jhenderson added inline comments.Jan 20 2020, 1:22 AM

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


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


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


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.


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.


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


the variable defined -> a defined variable


return its value -> match their value


What's the 30 here?


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


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.


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


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


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.