Page MenuHomePhabricator

[FileCheck] Given multiple -dump-input, prefer most verbose
ClosedPublic

Authored by jdenny on Nov 27 2019, 7:25 AM.

Details

Summary

Problem: FILECHECK_OPTS was implemented so that a test runner, such
as a bot, can specify FileCheck debugging options, such as
-dump-input=fail. However, some existing test suites have FileCheck
calls that already specify -dump-input=fail or -dump-input=always.
Without this patch, such tests fail under such a test runner because
FileCheck doesn't accept multiple occurrences of -dump-input.

Solution: This patch permits multiple occurrences of -dump-input by
assigning precedence to its values in the following descending order:
help, always, fail, and never. That is, any occurrence of
help always obtains help, and otherwise the behavior is similar to
-v vs. -vv in that the option specifying the greatest verbosity
has precedence.

Rationale: My justification for the new behavior is as follows. I
have not experienced use cases where, either as a test runner or as a
test author, I want to limit the permitted debugging verbosity
(except as a test author in FileCheck's or lit's test suites where the
FileCheck debugging output itself is under test, but the solution
there is env FILECHECK_OPTS=, and I imagine we should use the same
solution anywhere else this need might occur). Of course, as either a
test runner or test author, it is useful to increase debugging
verbosity.

Diff Detail

Event Timeline

jdenny created this revision.Nov 27 2019, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2019, 7:25 AM

I know we've had previous differences of opinion on how many combinations to test, and I'll bring it up again. I think we really need:

  • only one case where the two options are the same; that demonstrates a repeated option is not an error.
  • minimum cases showing the higher one gets picked: never/fail, fail/always, always/help. By transitivity we conclude the others are fine.
  • only one case for the other order, showing it's not first (or last) always wins.
  • only two cases of environment/command, showing it's not one or the always wins.

So a total of 7 cases, rather than the 16 you have here.

The code is fine, the tests are fine, I think we just don't need to spend as much resource on the test here.

jdenny updated this revision to Diff 231328.Nov 27 2019, 2:55 PM

@probinson : I followed your testing strategy, which I agree is better. Thanks for the quick review!

This revision is now accepted and ready to land.Dec 2 2019, 9:58 AM
This revision was automatically updated to reflect the committed changes.