[PATCH] D72912: [FileCheck] Clean and improve unit tests

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:329
+  // Invalid rhs operand.
+  EXPECT_TRUE(Tester.parseSubstExpect("@LINE+%VAR"));
+
----------------
jhenderson wrote:
> 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.
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.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:353
   EXPECT_TRUE(Tester.parsePatternExpect("[[#42INVALID]]"));
-  EXPECT_TRUE(Tester.parsePatternExpect("[[#@FOO]]"));
-  EXPECT_TRUE(Tester.parsePatternExpect("[[#@LINE/2]]"));
----------------
jhenderson wrote:
> It's not clear to me how this and the other deleted test cases are covered elsewhere?
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().


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:441
 
-  // Substitutions of defined pseudo and non-pseudo numeric variables return
-  // the right value.
-  NumericVariable LineVar("@LINE", 1);
+  // Substitutions of defined numeric variables return its value.
   NumericVariable NVar("N", 1);
----------------
jhenderson wrote:
> return its value -> match their value
I'm not testing match here, only substitution. I've thus rephrased accordingly.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:445
   auto NVarUse = std::make_unique<NumericVariableUse>("N", &NVar);
-  NumericSubstitution SubstitutionLine(&Context, "@LINE", std::move(LineVarUse),
-                                       12);
   NumericSubstitution SubstitutionN(&Context, "N", std::move(NVarUse), 30);
   SubstValue = SubstitutionN.getResult();
----------------
jhenderson wrote:
> What's the 30 here?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72912