[flang-dev] RFC: Port test_modfile.sh tests in Flang to use FileCheck

[Hal Finkel via flang-dev]

On 7/9/20 10:01 AM, Richard Barton wrote:
> Hi all
>
> Thanks Tim and Hal for your replies.
>
> In my original post, I didn’t do a great job of explaining why I think we need to change away from test_modfile.sh and why I think my ported test behaviour is better.
>
> The reason I think we need to change away from test_modfile.sh eventually so we can enable Windows support in Flang. I also think we should consider the principle of least surprise with these tests. I think these tests should be written to optimise for the common use case and I think that use case is someone examining a test to determine the root cause of a test failure. I think this use case is going to be more common over time than a developer making a broad change across all tests of a similar type or a developer adding a new test of this type. I think that my common user will appreciate explicitness over brevity in both the test specification and test output and would prefer that the test use familiar tools where possible over something custom and specific to particular tests.
>
> In the ported tests that I have shared, the flang execution line is clearly written in the test file and clearly reported on the case of error (or verbose output) by lit. This makes reproducing a test failure with a compiler invocation outside of any infrastructure (lit, test_modfile or FileCheck) a very simple copy/paste job. This will help my typical user.
>
> The test_modfile.sh failure output is certainly shorter and more succinct than the ported tests output and I agree that it is clear and does a good job. The test_modfile.sh output does assume tacit knowledge of what test_modfile.sh does to understand it completely. Taking Tim's first example, which way around is the diff here? Did flang generate i but the test expect ii or the other way around? Where is m.mod to be found? How was it generated? With the ported tests, all of this is explicit in the output i.e. "lit ran flang like this generating this file here. FileCheck looked at it and expected to see this inside it, but actually got this inside it." I think that my casual user would find the FileCheck output an improvement over test_modfile.sh's already good output. I think that the fact it is standard FileCheck output will also help my typical user. (Note, this is for the first of Tim's examples only, I totally agree that his second example is worse and I'll come back to that later).
>
> In terms of test specification, I agree that test_modfile.sh makes that shorter and cleaner, but I don't agree this is a good thing. My typical user would rather read exactly what is being run in the test file itself rather than have to understand test_modfile.sh in tandem with the test file and reconstitute the run line themselves.
>
> Tim's point on test_modfile.sh making the tests maintainabilie only holds if the new test that one wants to add works the same way as all the existing tests or if you want to change something common to all tests, as per his example. If one wanted to add a new test that needed to drive flang differently or run different tests over the resulting module files then test_modfile.sh would need to change to accommodate it, increasing complexity and decreasing transparency of the script. This would make the job of figuring out what ran and how to reproduce the a failure harder for my typical user.
>
> I make the same point as before on familiarity. I think multiple RUN lines and CHECK lines with different prefixes would not cause a problem for a typical reader investigating a regression.
>
> For these reasons, I think the ported tests are better than what went before them with one exception…
>
> I totally agree that the NO_MODFILES command is not good and certainly would represent a regression in experience for my typical user (and everyone else!). We can aid my typical user by adding comments in the test itself and perhaps by picking a better name for the CHECK prefix for this check, but clearly the custom diagnostic message that test_modfiles.sh gives is always going to be clearer in meaning. The NO_MODFILES run line perhaps poses a portability question to Windows too given that it uses wc and ls directly. I think this would mean we should add a REQUIRES: shell to the tests which would restrict the testing to Unix systems only, which rather defeats the point of the change.
>
> If we were to agree that for the rest of the tests, the FileCheck port is better, then we should remove the NO_MODFILES check from every module file test and find some other way to test flang against this requirement. This could perhaps be a unit test, a custom testing utility or perhaps with some platform-specific lit tests that use ls | wc on Unix and whatever the Windows equivalent of that is on Windows.
>
> Clearly a few different opinions here - more comments and opinions would be very helpful.
>
> As a next step, I will have a look at what a python port of test_modfiles.sh might look like and report back so we can consider that option too.


Thanks, Rich. A couple additional thoughts:

  1. It seems like a good idea, if we have a separate Python script, for 
the script to collect and print the command line necessary to reproduce 
the failure when there's a failure. I certainly agree that's an 
important ability to be able to reproduce the commands necessary outside 
of the script.

  2. One thing that we might consider, if we use FileCheck directly, is 
to use a lit expansion variable in order to limit the amount of 
boilerplate in the RUN lines (i.e., add something convenient by calling 
config.substitutions.append as we do in 
/compiler-rt/test/lit.common.cfg.py).

  -Hal


>
> Ta
> Rich
>
> PS. @Tim: Good catch on the temp file deletion - I mistakenly believed that lit guaranteed a unique or clean temp dir with %t. The correct fix for that would be to clean the temp dir at the start of each test execution.
>
>> ...
>> --
>> Hal Finkel
>> Lead, Compiler Technology and Programming Languages
>> Leadership Computing Facility
>> Argonne National Laboratory

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory