Page MenuHomePhabricator

[LIT] error if directly named test won't be run indirectly
ClosedPublic

Authored by bd1976llvm on Jul 2 2020, 11:45 AM.

Details

Summary

Currently, a LIT test named directly (on the command line) will be run even if the name of the test file does not meet the rules to be considered a test in the LIT test configuration files for its test suite. For example, if the test does not have a recognised file extension.

This makes it relatively easy to write a LIT test that won't actually be run. I did in: https://reviews.llvm.org/D82567

This patch adds an error to avoid users doing that. There is a small performance overhead for this check. A command line option has been added so that users can opt into the old behaviour.

Note to anyone reading this review in the future: The patch was initially to warn not error. I changed to error later and adjusted the Title and Summary.

Diff Detail

Event Timeline

bd1976llvm created this revision.Jul 2 2020, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 11:45 AM

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

That was an earlier version (where I was creating a set of all possible tests and then checking that the specified test wasn't in it). Performance seems acceptable now.

thakis added inline comments.Jul 11 2020, 7:29 PM
llvm/utils/lit/lit/discovery.py
163

This looks potentially slow. Did you do any perf measurements for this patch? What's lit test discovery time for some target (say, check-llvm) for this test vs without? (min-of-5, or ministat)

bd1976llvm marked an inline comment as done.Jul 20 2020, 12:05 PM
bd1976llvm added inline comments.
llvm/utils/lit/lit/discovery.py
163

Sorry for the slow reply.

I didn't measure any speed difference with check-llvm; that was because check-llvm does not invoke single tests directly (which is the only code path that this change impacts). Instead, I picked a directory with a large number of tests (llvm/test/CodeGen/X86/) and added a new test that would trigger the warning. I recorded these numbers running that test:

min-of-five without this change:

~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

real    0m0.295s
user    0m0.233s
sys 0m0.035s

min-of-five with this change:

~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
llvm-lit: discovery.py:169: warning: 'LLVM :: CodeGen/X86/zlib-longest-match.not' would not be run indirectly: change name or LIT config
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

1 warning(s) in tests

real    0m0.335s
user    0m0.274s
sys 0m0.030s

I also tested on windows (where filesystem operations are often more expensive) and recorded similar numbers.

So, there is a performance impact. Personally, I think this is acceptable *unless* users are listing large numbers of tests individually on their LIT command lines. That seems unlikely (IMO). However, if that is an important use-case then I think that emitting this warning only if a single test has been specified would be reasonable.

Adding @jdenny and @thopre who I think have been messing with lit recently.

bd1976llvm updated this revision to Diff 297914.EditedOct 13 2020, 11:01 AM
bd1976llvm marked an inline comment as done.
bd1976llvm edited the summary of this revision. (Show Details)

Improvements:

  • Error rather than warn - lit tests are regression tests!
  • Add option to supress check (there is no performance cost if this option is specified).
  • Standardized on the terms "direct" and "indirect" in comments and diagnostics.
  • Added missing test file.

LGTM but I'm not familiar with lit to tell whether there would be a more idiomatic way to write this. I also agree with the assumption that invoking lit with a large number of tests arguments rather than a directory is unlikely.

llvm/utils/lit/tests/discovery.py
144

Why not test the full error message?

bd1976llvm retitled this revision from [lit] warn if explicitly specified test won't be run indirectly to [LIT] error if directly named test won't be run indirectly.Oct 19 2020, 4:45 AM
bd1976llvm edited the summary of this revision. (Show Details)
bd1976llvm edited the summary of this revision. (Show Details)Oct 19 2020, 4:51 AM
bd1976llvm marked an inline comment as done.Oct 19 2020, 4:41 PM
bd1976llvm added inline comments.
llvm/utils/lit/tests/discovery.py
144

I try to keep negative test cases from being too specific to try to prevent them from "failing for the wrong reason" (e.g. if in the future the error message is altered slightly then if we were checking the full error message the check would pass even though the, slightly altered, message is being emitted). However, looking at this again I don't think checking the output is worthwhile so I have removed it.

bd1976llvm marked an inline comment as done.

Removed check for error message not being emitted.

LGTM but I'm not familiar with lit to tell whether there would be a more idiomatic way to write this. I also agree with the assumption that invoking lit with a large number of tests arguments rather than a directory is unlikely.

Thanks for looking at this. If you're happy could you accept the change so that I can commit.

thopre accepted this revision.Oct 20 2020, 2:05 AM

LGTM

This revision is now accepted and ready to land.Oct 20 2020, 2:05 AM

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

Hi @thopre,

Sorry - I missed this after the Christmas break somehow. Reading https://reviews.llvm.org/D94766 it seems that you have decided that a config option would be better than a command line option to control this check? +1 to the config option. However, I think that we might still need --no-indirectly-run-check. My scenario would be to allow someone to add and run experimental tests within a teststuite that has the config option set without having to rename them or adjust the testsuite rules. I'm assuming that it is easier to add a command line option than to try to find and disable the relevant config option. Not sure if this is plausible as a scenario but I wanted to raise it. Having raised that scenario, maybe it's still better to add a config option and remove the command line option and then wait to see if there are any usability reports raised?

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

Hi @thopre,

Sorry - I missed this after the Christmas break somehow. Reading https://reviews.llvm.org/D94766 it seems that you have decided that a config option would be better than a command line option to control this check? +1 to the config option. However, I think that we might still need --no-indirectly-run-check. My scenario would be to allow someone to add and run experimental tests within a teststuite that has the config option set without having to rename them or adjust the testsuite rules. I'm assuming that it is easier to add a command line option than to try to find and disable the relevant config option. Not sure if this is plausible as a scenario but I wanted to raise it. Having raised that scenario, maybe it's still better to add a config option and remove the command line option and then wait to see if there are any usability reports raised?

I'm not looking at removing the option. All I want is a mechanism to disable the check in a way that is ignored by old lit versions. So config + command-line is fine by me