Hello,
When someone breaks a FileCheck-based test on some buildbot, sometimes
it may not be obvious *why* did it fail. If the failure can not be
reproduced locally, it can be very hard to fix.
I propose adding a "very verbose" mode to FileCheck. In this mode
FileCheck will dump the input file in case of failure. This mode will
be enabled by an environment variable "FILECHECK_VERY_VERBOSE". If we
chose a command line option, we would have to edit all FileCheck-based
tests to use %FileCheck.
Attached is the proposed patch. A patch for zorg will follow if this
is accepted. Normal output for 'make check' is unchanged.
I am not attached to the name "FILECHECK_VERY_VERBOSE", suggestions
for better names are welcome.
Dmitri
filecheck-very-verbose-v1.patch (3.6 KB)
Could you describe a situation where this helps in more detail? It's
quite easy to reproduce the input file to FileCheck locally.
Eli
Imagine a situation when you have opt | FileCheck. And 'opt' produces
something weird only on a buildbot, while on your machine the output
is correct.
See, for example, this thread
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130114/162207.html
Dmitri
I think that this idea is good, but I'd prefer it be implemented a different way:
- Filecheck should take a new flag -dump-input-on-error that causes it to... dump the input file on error.
- Lit should be the thing that checks the environment (or perhaps add a new option to lit), and adds the flag to FileCheck invocations.
I don't like it when the behavior of such a low-level tool like this changes based on environment variables. It isn't discoverable in --help. If for some reason, it is bad for lit to implicitly pass the option, I'd rather have a standard FILECHECK_COMMANDLINE environment variable, and have filecheck parse arbitrary options out of it using the cl::ParseEnvironmentOptions function.
-Chris
I agree that a command line option would be better. But in that case
all tests should be updated. It is not an issue for me -- it is
mostly mechanical. So should I change tests to use %FileCheck?
Dmitri
Here's a third attempt.
The new behavior is as follows:
1. In case of errors we always dump output to a temporary file and print
Saving input file "<stdin>" to "/tmp/filecheck.txt-Jno73y"
2. If --dump-input-on-error option is passed, FileCheck will also dump
input to stderr.
3. If FILECHECK_DUMP_INPUT_ON_ERROR env var is set, lit will replace
"%FileCheck" with "FileCheck --dump-input-on-error".
I will fix all tests in LLVM and Clang if we decide this is the way to go.
Dmitri
filecheck-very-verbose-v3.patch (4.75 KB)
Hello,
When someone breaks a FileCheck-based test on some buildbot, sometimes
it may not be obvious *why* did it fail. If the failure can not be
reproduced locally, it can be very hard to fix.
I propose adding a "very verbose" mode to FileCheck. In this mode
FileCheck will dump the input file in case of failure. This mode will
be enabled by an environment variable "FILECHECK_VERY_VERBOSE". If we
chose a command line option, we would have to edit all FileCheck-based
tests to use %FileCheck.
I think that this idea is good, but I'd prefer it be implemented a different way:
- Filecheck should take a new flag -dump-input-on-error that causes it to... dump the input file on error.
- Lit should be the thing that checks the environment (or perhaps add a new option to lit), and adds the flag to FileCheck invocations.
I don't like it when the behavior of such a low-level tool like this changes based on environment variables. It isn't discoverable in --help. If for some reason, it is bad for lit to implicitly pass the option, I'd rather have a standard FILECHECK_COMMANDLINE environment variable, and have filecheck parse arbitrary options out of it using the cl::ParseEnvironmentOptions function.
I agree that a command line option would be better. But in that case
all tests should be updated. It is not an issue for me -- it is
mostly mechanical. So should I change tests to use %FileCheck?
Here's a third attempt.
The new behavior is as follows:
1. In case of errors we always dump output to a temporary file and print
Does it mean we get one more file in /tmp every time a test fails, and
it is not cleaned up automatically? I don't think this should happen
in the "default" mode of the tool.
Other than that, the idea sounds awesome, and I'd be happy to see any
variation of it on the bots ASAP.
Well, yes. David requested that and I agreed that it is a good idea.
Are you strongly opposed to it?
Dmitri
Not really, I doubt it can break something for someone...
But if we only need this behaviour on the buildbot, and you are going
to update all tests anyway, why not make it conditional on a flag?
Buildbots don't need to create files. Richard requested this because
it is just generally useful -- usually the first thing you do after
seeing a failed FileCheck test is running the command manually to see
the output.
Dmitri
I agree that a command line option would be better. But in that case
all tests should be updated. It is not an issue for me -- it is
mostly mechanical. So should I change tests to use %FileCheck?
Here's a third attempt.
Thanks in advance for driving this forward. I'm sorry that such a simple thing is so complicated.
The new behavior is as follows:
1. In case of errors we always dump output to a temporary file and print
Saving input file "<stdin>" to "/tmp/filecheck.txt-Jno73y"
This doesn't make sense to me. It's really common in a normal development scenario to do something, test and have stuff fail. It doesn't make sense to dump things into /tmp in this case.
2. If --dump-input-on-error option is passed, FileCheck will also dump
input to stderr.
This is fine.
3. If FILECHECK_DUMP_INPUT_ON_ERROR env var is set, lit will replace
"%FileCheck" with "FileCheck --dump-input-on-error".
Sounds good for lit.
I will fix all tests in LLVM and Clang if we decide this is the way to go.
What tests need to be fixed? FileCheck -> %FileCheck? You should check with Daniel, but would it make sense to have lit just "know" FileCheck?
-Chris
I agree that a command line option would be better. But in that case
all tests should be updated. It is not an issue for me -- it is
mostly mechanical. So should I change tests to use %FileCheck?
Here's a third attempt.
Thanks in advance for driving this forward. I'm sorry that such a simple thing is so complicated.
The new behavior is as follows:
1. In case of errors we always dump output to a temporary file and print
Saving input file "<stdin>" to "/tmp/filecheck.txt-Jno73y"
This doesn't make sense to me. It's really common in a normal development scenario to do something, test and have stuff fail. It doesn't make sense to dump things into /tmp in this case.
That was a suggestion/request from me - my thinking was that, pretty
much whenever that happens, you end up wanting to look at the output
anyway. My usual process involves copy/pasting the command (if I can
tell which one it is, since lit dumps all the RUN lines out, I think)
& running it to look at the output to see what went wrong. It seems
having that output already available would skip that step - is this
less common for other developers? Is there some other solution that
might address this?
(I suggested this in part in the hopes that we could find a unified
solution for local and buildbot execution, but on consideration I
couldn't find a particularly easy way to gather the extra log file
back to the build master so Dmitri's solution still seemed
necessary/convenient - making my suggestion perhaps orthogonal to his
current proposal)
Hello Daniel & llvm-commits,
We are implementing a "very verbose" for FileCheck, so that FileCheck
will dump the input to stderr in case of mismatch. This will make it
easier to debug failures on remote buildbots.
This will be a separate mode in FileCheck, activated by "FileCheck
--dump-input-on-error". lit should add that option to FileCheck
invocations on buildbots (lit can see if it is running on a buildbot
by checking an environment variable).
We have to options:
(a) replace 'FileCheck' with '%FileCheck' in all tests, and teach
'lit' to replace '%FileCheck' with 'FileCheck --dump-input-on-error';
(b) teach 'lit' to replace a plain 'FileCheck'.
The first approach seems cleaner to developers who read and write
tests (it suggests that they are invoking some "macro" -- but does
that matter?) The second approach is much easier to implement since
tests will be unchanged.
Dmitri
IMO the biggest issue with (a) is that developers will continue to use
`FileCheck` instead of `%FileCheck`. So IMO (a) should only be
implemented if simultaneously there is a change that makes just plain
`FileCheck` an error.
Another approach would be to have a script or symlink
`FileCheck-dump-input-on-error` which lives in build/bin/ alongside
FileCheck, and which just invokes FileCheck in a way that causes it to
go into the "dump input on error" mode (e.g. it checks argv[0], or a
magic environment variable, or whatever). On the build bots,
FileCheck-dump-input-on-error and regular FileCheck could be swapped
by a configure option. That's kind of ugly, but some variation of it
may be more palatable.
-- Sean Silva
I think that within a month this knowledge will be propagated to all developers.
Dmitri
We have to options:
(a) replace 'FileCheck' with '%FileCheck' in all tests, and teach
'lit' to replace '%FileCheck' with 'FileCheck --dump-input-on-error';
(b) teach 'lit' to replace a plain 'FileCheck'.
The first approach seems cleaner to developers who read and write
tests (it suggests that they are invoking some "macro" -- but does
that matter?) The second approach is much easier to implement since
tests will be unchanged.
IMO the biggest issue with (a) is that developers will continue to use
`FileCheck` instead of `%FileCheck`. So IMO (a) should only be
implemented if simultaneously there is a change that makes just plain
`FileCheck` an error.
I think that within a month this knowledge will be propagated to all developers.
I'd like to think so, too, but we still get patches that write tests using 'grep' instead of FileCheck.
-Jim
This is unfortunate. Last month I tweaked TestingGuide.rst to
discourage grep in favor of FileCheck. It now says:
"The recommended way to examine output to figure out if the test
passes it using the FileCheck tool. The usage of grep in RUN lines is
discouraged."
However, perhaps it's time to remove any mention of grep from that document?
Eli
"Usage of grep in RUN lines will result in your patch being reverted." 
If you can think of a less snarky way of saying something along those lines, it'd be great. I'm more than a bit surprised the docs still talk about it at all, honestly. FileCheck has been the One True Way(™) for quite a while now.
-Jim
How about "Please do not submit/commit test patches that use grep" and
wipe out all other mention of grep (so that it's ungreppable, *sigh*)
from that document?
Eli
