[PATCH] D90631: [FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: thopre.
jhenderson added inline comments.


================
Comment at: llvm/test/FileCheck/allow-unused-prefixes.txt:1-2
-; RUN: not FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-ONE %s
-; RUN: not FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2,P3 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-MORE %s
-; RUN: FileCheck --allow-unused-prefixes=true  --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
+; RUN: not %ProtectFileCheckOutput FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-ONE %s
+; RUN: not %ProtectFileCheckOutput FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2,P3 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-MORE %s
+; RUN: %ProtectFileCheckOutput FileCheck --allow-unused-prefixes=true  --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
----------------
The common pattern I've seen is to put `%ProtectFileCheckOutput` first before anything else like `not`. There are some good examples in a number of other FileCheck tests, like `numeric-defines.txt`.


================
Comment at: llvm/test/FileCheck/allow-unused-prefixes.txt:3-7
+; RUN: %ProtectFileCheckOutput FileCheck --allow-unused-prefixes=true  --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
 
 ;; Note: the default will be changed to 'false', at which time this run line
 ;; should be changed accordingly.
+; RUN: %ProtectFileCheckOutput FileCheck --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
----------------
dblaikie wrote:
> mtrofin wrote:
> > dblaikie wrote:
> > > mtrofin wrote:
> > > > dblaikie wrote:
> > > > > I /think/ these two don't need it, since their output doesn't feed into another FileCheck command (so I assume it's intended to be seen/understood by a human) - but I haven't looked closely at the test. Is there some reason these two would need stabilized/non-customizable output?
> > > > What if the user sets --allow-unused-prefixes in their env?
> > > My guess would be that that wouldn't be reasonably supported through `FILECHECK_OPTS` - in the same way that passing `--check-prefixes=SOMETHING` via `FILECHECK_OPTS` wouldn't be usefully supported either. `FILECHECK_OPTS` is for things like `--color` and `--dump-*` flags, I believe.
> > The 2 tests are that FileCheck passes - i.e. it returns 0. So I'd argue they are similar to the other cases, in that we rely on the user not accidentally messing with the options.
> That property (that FileCheck passes) is true of every test that uses FileCheck (ie: lots of LLVM tests), not only the ones intended to check FileCheck's behavior.
> 
> Perhaps @jhenderson has some perspective here.
It is my belief that @dblaikie is correct. @thopre is probably more up-to-speed on the details of this, but based on a test he relatively recently wrote (see again `numeric-defines.txt`), you only need it where the first FileCheck call is fed into another FileCheck call. In those cases, we are testing the output of FileCheck, so need it constrained. In other cases, like here, we are only testing the matching behaviour.

Using the general environment to change what prefixes are specified seems like a terrible idea, as other people wouldn't have the environment set up in that way and therefore the test behaviour would be different. I can see a niche case for using the environment variable within a limited scope of tests that were explicitly written to support that, so that a user could rapidly test several different variations of behaviour, but that isn't the case for this test, or indeed any of FileCheck's tests. The same basic argument would go for other commands like `--allow-unused-prefixes` which change FileCheck's behaviour in a way that would cause the test to pass/fail under different environments.

Another point here is that setting %ProtectFileCheckOutput explicitly prevents using `--dump-*` to get FileCheck to dump more output when this test fails, which defeats the point of the environment variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90631