Page MenuHomePhabricator

[FileCheck] Make FILECHECK_OPTS useful for its test suite
AcceptedPublic

Authored by jdenny on Jul 22 2019, 3:36 PM.

Details

Summary

Without this patch, FILECHECK_OPTS isn't propagated to FileCheck's
test suite so that FILECHECK_OPTS doesn't inadvertently affect test
results by affecting the output of FileCheck calls under test. As a
result, FILECHECK_OPTS is useless for debugging FileCheck's test
suite.

In llvm/test/FileCheck/lit.local.cfg, this patch provides a new
subsitution, %ProtectFileCheckOutput, to address this problem for
both FILECHECK_OPTS and the deprecated
FILECHECK_DUMP_INPUT_ON_FAILURE. The rest of the patch uses
%ProtectFileCheckOutput throughout the test suite

Fixes PR40284.

Diff Detail

Event Timeline

jdenny created this revision.Jul 22 2019, 3:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 3:36 PM
Herald added a subscriber: thopre. · View Herald Transcript

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks. That bit should be its own patch, regardless of how we decide to handle FileCheck itself.

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

That bit should be its own patch, regardless of how we decide to handle FileCheck itself.

Sure, I'll separate out the lit change.

Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.

I have to say my initial reaction was "omg not another layer of indirection in the FileCheck test suite" but rereading PR40284, my own comments there have persuaded me to rethink that position. :-)

Reading the comments in FileCheck/lit.local.cfg, yes at some low level the point is whether varying the output affects the test, but I don't think that's a properly principled way to look at it. Also thinking that way is likely to cause brain damage in anyone coming along later and trying to decipher when to use which spelling of "run this tool."

Better to look at it this way (this is a multi-paragraph argument, sorry): This is a tool, these are its tests, and we want to run the tool and then FileCheck its output. This is how we test llvm-objdump and llvm-dwarfdump and all the rest of those tools. By a bizarre quirk of fate, the command to run the tool itself is spelled the same as FileCheck. So, let's change the spelling of "run this tool" because we'd like to keep the RUN lines following a pattern of toolname | FileCheck. We can call it something like %FileCheckUnderTest or %TestFileCheck or something else that implies "run the FileCheck I'm trying to test." (This would happen to be implemented as "run FileCheck with no environment.") Then your basic test of FileCheck would have a RUN line something like RUN: %TestFileCheck blah | FileCheck %s and it should be clear to the easily-trained eye which invocation of FileCheck is fulfilling which role. Importantly the *choice* of which spelling to use is based solely on the role within the test RUN line, not some more complicated "does it affect the test result" decision.

Sadly there are a few cases where a test actually does want to set an environment variable, and in those cases the above substitution will not do the right thing. Apparently we want (or also want) a substitution that will clean out the environment but not actually run the tool, so that we can set new environment variables before running it. Maybe call it %CleanEnv. Then your basic FileCheck-with-environment test's RUN line would look something like %CleanEnv env FILECHECK_OPTS=-v FileCheck blah | FileCheck %s

At that point, %TestFileCheck can be considered shorthand for %CleanEnv FileCheck (although likely the substitution wouldn't actually be implemented that way).

WDYT?

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

Then every test that had this issue would have to be modified to add the option. I think it would have to be an environment variable that suppresses the other environment variables: FILECHECK_IGNORE_ENV=1 or something. Then test suites could add that in their config files, and tests would be written as normal.

Luckily, so far we don't seem to need that level of confusion.

Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.

I have to say my initial reaction was "omg not another layer of indirection in the FileCheck test suite"

I've been sitting on this patch for a few weeks in part because I had the same concern. :-) But I ultimately convinced myself that this is worth it for debugging FileCheck's test suite, which is challenging.

We can call it something like %FileCheckUnderTest or %TestFileCheck or something else that implies "run the FileCheck I'm trying to test."

This is actually what I originally talked about too in some phab reviews somewhere. One of them led to PR40284. I even started to implement that approach and then became convinced it's actually more complicated than the approach in this patch. See below.

Importantly the *choice* of which spelling to use is based solely on the role within the test RUN line, not some more complicated "does it affect the test result" decision.

This is an interesting point. At first, I thought the same. Then I realized that I cannot easily look at a FileCheck call in a RUN line and determine which role it serves. I can easily determine whether FileCheck's stdout or stderr is redirected/piped for further examination, and then I should surely use %TestFileCheckOut.

Now you might argue that the person originally writing a test surely knows which FileCheck call serves which role. Moreover, if he uses your %FileCheckUnderTest correctly, then everyone in the future will know, and that might be helpful for understanding tests besides just protecting from environment variables.

I'm not optimistic that approach will always be followed correctly and avoid rot over time. Actually, I think my simple check is much easier to follow when looking back at old tests: if the stdout/stderr is examined, then use %TestFileCheckOut. (Hmm, maybe the documentation I wrote could be simpler.) And then I can even use FILECHECK_OPTS to debug some FileCheck calls that are under test.

Sadly there are a few cases where a test actually does want to set an environment variable, and in those cases the above substitution will not do the right thing. Apparently we want (or also want) a substitution that will clean out the environment but not actually run the tool, so that we can set new environment variables before running it. Maybe call it %CleanEnv. Then your basic FileCheck-with-environment test's RUN line would look something like %CleanEnv env FILECHECK_OPTS=-v FileCheck blah | FileCheck %s

I went down this path at one point too... even with my does-the-output-matter approach. That is, %TestFileCheckOut originally included the FileCheck call, so I had a second substitution too.

Then I tried to document it, and I thought about people trying to maintain these in tests. Ugh. Yet another quirky substitution to confuse people. That's when I decided one simple substitution with a simple rule that handles all cases is easier to understand. Anyone who knows how env works can figure out how to set FILECHECK_OPTS just by glancing at the %TestFileCheckOut implementation, and there are examples in the documentation for everyone else.

WDYT?

In summary, I understand every point you made as I thought all the same things as I was working through this. It was my attempt to implement and document that led me to believe my current approach is actually simpler and more useful. Hopefully I didn't become too close to the implementation along the way to see it clearly. :-/

In a moment, I'll update the patch so you can see the mass changes throughout FileCheck's test suite, and I'll simplify the documentation at little. Maybe that'll help.

jdenny updated this revision to Diff 211322.Jul 23 2019, 9:51 AM
jdenny retitled this revision from [FileCheck][lit] Make FILECHECK_OPTS useful for their test suites to [FileCheck] Make FILECHECK_OPTS useful for its test suite.
jdenny edited the summary of this revision. (Show Details)
  • Remove changes for lit's test suite (will post soon in another review).
  • Simplify %TestFileCheckOut documentation some.
  • Propagate %TestFileCheckOut throughout FileCheck's test suite.

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

Then every test that had this issue would have to be modified to add the option.

I was thinking of a command-line option that prints something like -u FILECHECK_OPTS -u FILECHECK_DUMP_INPUT_ON_FAILURE. The idea is that lit/test/lit.cfg wouldn't need to maintain a list of variables.

I think it would have to be an environment variable that suppresses the other environment variables: `FILECHECK_IGNORE_ENV=1` or something. Then test suites could add that in their config files, and tests would be written as normal.

Makes sense too.

Luckily, so far we don't seem to need that level of confusion.

Agreed. It's just one external test suite, and there's no evidence the list of variables is going to evolve much further. FILECHECK_OPTS is intended to be a catch-all. FILECHECK_DUMP_INPUT_ON_FAILURE is deprecated.

Hi,

Could you add a section FileCheck-specific substitutions section in llvm/docs/TestingGuide.rst to describe %TestFileCheckOut?

Hi,

Hi Thomas.

Could you add a section FileCheck-specific substitutions section in llvm/docs/TestingGuide.rst to describe %TestFileCheckOut?

That seems like a good idea. We'll need to decide what goes there and what stays in llvm/test/FileCheck/lit.local.cfg.

I'll make a note to work on this after we reach consensus about the actual behavior.

Getting back to this after a long month on first-responder-to-bugs duty. I know you're trying to get the lit/env issue sorted out, but I had a half hour available to look at this.

It makes me very sad to see
RUN: not FileCheck blahblah | FileCheck yadayada
become

RUN: not %TestFileCheckOut \
RUN: FileCheck blahblah | FileCheck yadayada

Wouldn't it work just as well to say

RUN: %TestFileCheckOut \
RUN: not FileCheck blahblah | FileCheck yadayada

which keeps the not adjacent to the FileCheck (and incidentally sidesteps the "not runs env as an external command" problem).

llvm/test/FileCheck/lit.local.cfg
3

Okay, that seems pretty straightforward and easy to understand.

51

This will clearly have to be changed to solve the problems found after D65156. But it can wait until that is sorted out.

Wouldn't it work just as well to say

RUN: %TestFileCheckOut \
RUN: not FileCheck blahblah | FileCheck yadayada

which keeps the not adjacent to the FileCheck (and incidentally sidesteps the "not runs env as an external command" problem).

Yes, I like that better too. Thanks!

llvm/test/FileCheck/check-count.txt
7–9

Instead of that, just add a preceding line with

; RUN: %TestFileCheckOut \

Likewise everywhere else.

See https://reviews.llvm.org/D65121#1610682.

jdenny updated this revision to Diff 213205.Aug 3 2019, 2:37 PM

Rebase. Specify %TestFileCheckOut before not.

Still need to write the suggested documentation and deal with the "env -u" issue.

jdenny marked 2 inline comments as done.Aug 3 2019, 2:40 PM
jdenny updated this revision to Diff 213211.Aug 3 2019, 3:28 PM

Forgot to git add a change for that last update.

More bikeshedding: The new substitution could be called %TestFileCheckOutput (thus not abbreviating Output to Out) which helps it to be more self-documenting.
Or, it could be called %TestFileCheckEnv because its function is to set the environment, and it can optionally take an environment-variable definition as an argument. (Sorry if I already suggested this and we decided against.)
Everything else about this looks fine, LGTM once the substitution's name is settled.

probinson accepted this revision.Aug 8 2019, 11:49 AM
This revision is now accepted and ready to land.Aug 8 2019, 11:49 AM

More bikeshedding: The new substitution could be called %TestFileCheckOutput (thus not abbreviating Output to Out) which helps it to be more self-documenting.

That works for me, and I'll go with that if there's no further discussion.

Or, it could be called %TestFileCheckEnv because its function is to set the environment, and it can optionally take an environment-variable definition as an argument. (Sorry if I already suggested this and we decided against.)

I like seeing Env there. I don't like dropping Output because I want the reminder that FileCheck output is the reason to use this substitution.

I want to say %SetEnvForTestingFileCheckOutput, but that's too much. Can we cram all that into something shorter?

Another possibility: %ProtectFileCheckOutput

Everything else about this looks fine, LGTM once the substitution's name is settled.

We have time. I want to see how things go with my related lit test suite work first, and there was a suggestion about documentation here that I haven't worked on.

Thanks again.

Another possibility: %ProtectFileCheckOutput

Oh, I like that better.

jdenny updated this revision to Diff 216111.Aug 20 2019, 5:56 AM
jdenny set the repository for this revision to rG LLVM Github Monorepo.
  • Rebased.
  • Renamed %TestFileCheckOut to %ProtectFileCheckOutput.
  • Added documentation to llvm/docs/TestingGuide.rst.

This patch isn't ready to be pushed yet. FileCheck's test suite doesn't use lit's internal shell, and this patch introduces calls to env -u, which D65335 explains is not portable across external env implementations. One solution is to remove the use of env -u here. Another solution is to change FileCheck's test suite to use lit's internal shell. The latter solution is probably generally better for portability, but it won't help this patch without the patches I'm working on for lit's internal shell in order to push D65156 again. I'm inclined to wait on that anyway to be sure we've actually fully understood the trouble there.

jdenny updated this revision to Diff 228468.Fri, Nov 8, 8:45 AM
jdenny edited the summary of this revision. (Show Details)

Now that D65156 and its many dependencies seem to have survived the bots, I'm finally returning to this patch.

This update rebases onto a recent master and adds %ProtectFileCheckOutput to a few new RUN lines that have since appeared in FileCheck's test suite.

As discussed at D65121#1637145, we need to decide whether to eliminate env -u uses in this patch or to switch FileCheck's test suite to lit's internal shell. Either looks easy to do, but I vote for the latter as it seems better for portability in the long term. It appears to just require this in llvm/test/FileCheck/lit.local.cfg (llvm/utils/lit/tests/lit.cfg already has this):

import lit
config.test_format = lit.formats.ShTest(execute_external=False)

What do people think? I'll probably make that a separate review if there are no immediate objections to the idea.

jdenny edited the summary of this revision. (Show Details)Fri, Nov 8, 8:47 AM
jdenny added a comment.Fri, Nov 8, 8:52 AM

It appears the last update before today did not add documentation to llvm/docs/TestingGuide.rst as I intended, but today's update did, so that's another new change to look at.