Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I think only the FileCheck commands that are themselves FileChecked need the protection. The other ones are intended to be rendered to users (humans running the tests) so would benefit from being able to be customized with FILECHECK_OPTS, I think?
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 3–7 | 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? | |
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 3–7 | What if the user sets --allow-unused-prefixes in their env? | |
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 3–7 | 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. | |
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 3–7 | 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. | |
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 3–7 | 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. | |
| llvm/test/FileCheck/allow-unused-prefixes.txt | ||
|---|---|---|
| 1–4 | 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. | |
| 3–7 | 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. | |
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.