[PATCH] D52999: [FileCheck] Annotate input dump (1/7)

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:80-82
+.. option:: --dump-input <mode>
+
+  Dump annotated original input either 'always', on 'fail', or 'never'.
----------------
zturner wrote:
> I haven't been following this too closely, but I'm wondering, are the 3 modes actually necessary?  It sounds like the main use case here is that the user has a failure and wants to get more info.  So they will set it to either `fail` or `always`.  But do they care which?  Basically, what I'm wondering is why not just make this be a binary on flag?
> 
> It just seems simpler to say that if you want to dump the input, pass `--dump-input`, and if you don't want to dump the input, pass nothing.  
-dump-input=always is helpful when you're debugging individual tests and encounter FileCheck successes you don't understand.

I was thinking -dump-input=fail (via an env var) is better when running full test suites, perhaps from a bot or IDE.  Imagine a test with many successful FileCheck commands before the failed one.  -dump-input=always might produce massive output that will be logged and that must be scrolled/grepped through to find the failure, depending on how you like to interact with test suite logs.

In any case, -dump-input=fail was inspired by the existing -dump-input-on-failure, which I believe @george.karpenkov said is used in bots.  Is that right, George?


================
Comment at: llvm/test/FileCheck/match-full-lines.txt:4-7
+// RUN: not FileCheck -dump-input=never -match-full-lines -input-file %s %s  2>&1 \
 // RUN:   | FileCheck --check-prefix=ERROR --implicit-check-not=error: %s
-// RUN: not FileCheck -match-full-lines -strict-whitespace -input-file %s %s  2>&1 \
+// RUN: not FileCheck -dump-input=never -match-full-lines -strict-whitespace -input-file %s %s  2>&1 \
 // RUN:   | FileCheck --check-prefix=ERROR-STRICT --check-prefix=ERROR --implicit-check-not=error: %s
----------------
zturner wrote:
> You could also achieve this without the command line flag by writing `env FILECHECK_DUMP_INPUT= not FileCheck ...`.  Pretty sure you can unset a variable and run a command this way.
I don't recall why I did it that way, and your way does seem more obvious.  I'll change it.

While we're talking about this, I'd eventually like to handle this issue in another way entirely.  In FileCheck's own test suite, I'd like to ban direct FileCheck calls.  Instead, I'd like to require test authors to choose one of the following for each call:

* %FileCheckee: Used for FileCheck calls whose exit status and output are being checked.  Normal FILECHECK_* environment variables are cleared within these calls.  This avoids problems where people have these variables set for general testing purposes and thus change the FileCheck output being tested, creating spurious fails or passes.  If these calls need to test env vars, then we could just have an alternate set of environment variables (FILECHECKEE_* maybe) that %FileCheckee copies to the normal variables.

* %FileChecker: Used for FileCheck calls that check the output of %FileCheckee calls (or any other output under test).  FILECHECK_* environment variables are obeyed just as they are obeyed for FileCheck calls in other test suites.

Of course, this would be another patch, which I don't have time for right now.  Do you think the idea has merit?



Repository:
  rL LLVM

https://reviews.llvm.org/D52999