[PATCH] D65121: [FileCheck][lit] Make FILECHECK_OPTS useful for their test suites

Tue Jul 23 09:47:45 PDT 2019

jdenny added a comment.

In D65121#1597540 <https://reviews.llvm.org/D65121#1597540>, @probinson wrote:

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65121