Page MenuHomePhabricator

Add lit config for dir with standalone tests
ClosedPublic

Authored by thopre on Jan 15 2021, 4:52 AM.

Details

Summary

Some test systems do not use lit for test discovery but only for its
substitution and test selection because they use another way of managing
test collections, e.g. CTest. This forces those tests to be invoked with
lit --no-indirectly-run-check. When a mix of lit version is in use, it
requires to detect the availability of that option.

This commit provides a new config option standalone_tests to signal a
directory made of tests meant to run as standalone. When this option is
set, lit skips test discovery and the indirectly run check. It also adds
the missing documentation for --no-indirectly-run-check.

Diff Detail

Event Timeline

thopre created this revision.Jan 15 2021, 4:52 AM
thopre requested review of this revision.Jan 15 2021, 4:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 4:52 AM

Can you explain the use case a bit more? I don't find myself moving between old and new versions of lit very often. Perhaps I'm naive, but it seems like it should be easy enough to just not specify --no-indirectly-run-check when using the old version.

Can you explain the use case a bit more? I don't find myself moving between old and new versions of lit very often. Perhaps I'm naive, but it seems like it should be easy enough to just not specify --no-indirectly-run-check when using the old version.

Our systems use a mix of LLVM versions (whatever is available on that OS). This causes problem for this option because tests are run individually using lit (tests are managed by CTest which runs lit) and thus require the --no-indirect-run-check option to not fail due to warning but then systems with an old lit fail because the option is not accepted. Having a variable would allow to support a nice mix. It also allows to upgrade to a newer lit without having to update lit and the tests in lockstep.

The problem of --no-indirectly-run-check is it changes the behavior of lit. This is fine in LLVM but lit is used outside of LLVM and I think we should acknowledge that and aim to maintain compatibility to some degree (if cost is low, it should not become a burden to LLVM). The environment variable allow a smooth migration from one version to the next (and a mix of version in our case). A lit configuration option would work as well.

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

yln added a comment.Jan 21 2021, 11:09 AM

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

+1

I feel the same as Joel. Not opposed, but my default stance is to try to avoid collecting and codifying workarounds. They almost never get removed once obsolete.

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option. But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users. Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option.

Yes, that's what I had in mind. It seems simple, but maybe I'm missing something.

A fake test is probably the most robust way to check. But an even simpler check that should work for now is: llvm-lit --help | grep -q -- --no-indirectly-run-check.

But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

What makes it specific to your setup? Feature tests when building/testing a project are common.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users.

Are there many external users suffering from this? If so, hopefully they can chime in here.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option.

Yes, that's what I had in mind. It seems simple, but maybe I'm missing something.

A fake test is probably the most robust way to check. But an even simpler check that should work for now is: llvm-lit --help | grep -q -- --no-indirectly-run-check.

But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users.

Are there many external users suffering from this? If so, hopefully they can chime in here.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Is it actually useful to implement that for any other command-line option?

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

No because the project in question is built before LLVM. Only at test time is lit guaranteed to be there. But as I said, no need to talk about my specifics, I can find a workaround if needed.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Yes that's what I'm thinking about.

Is it actually useful to implement that for any other command-line option?

Just looking at lit usage message, I could see most option making sense as a config option. I don't personally need them though. I guess a config option of --no-indirect-run-check is enough and can be generalized later.

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

But as I said, no need to talk about my specifics, I can find a workaround if needed.

Well, it's the only example I have so far. But we seem to be converging on another solution now.

Just looking at lit usage message, I could see most option making sense as a config option.

For me, it's just the opposite. Most seem like they are not properties of a test suite that belong in the config. Most seem like they are specific to a run of the test suite.

I don't personally need them though. I guess a config option of --no-indirect-run-check is enough and can be generalized later.

That's fine. But I am curious to hear more about the general use case. If it's useful, it will probably come up again.

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Before you invest time in this, it might be good to hear from someone else. @yln, does this solution seem reasonable to you?

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Good question, I would leave it because it could allow for quickly testing a new test with a new suffix without updating lit and it's a small amount of code but YMMV. No strong preference.

yln added a comment.Jan 25 2021, 12:04 PM

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Before you invest time in this, it might be good to hear from someone else. @yln, does this solution seem reasonable to you?

Sounds good to me, thanks! :)

thopre updated this revision to Diff 323161.Feb 11 2021, 2:40 PM
thopre retitled this revision from Add lit env variable to disable indirect checks to Add lit config for dir with standalone tests.
thopre edited the summary of this revision. (Show Details)

Rework into a config option

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Is it actually useful to implement that for any other command-line option?

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

How's this new approach? Feel free to suggest a better config name.

jdenny added inline comments.Feb 12 2021, 2:08 PM
llvm/docs/CommandGuide/lit.rst
381

I assume then that there's no known use case where a test directory sometimes relies on test discovery but also has standalone tests. It sounds like it's one or the other.

Then should standalone_tests complain if suffixes or excludes is set?

Actually, if suffixes isn't set (or is empty or None), should that be enough to enable --no-indirectly-run-check? Do we need standalone_tests?

llvm/utils/lit/lit/discovery.py
162–163

This comment should probably mention standalone_tests as well.

178–179

It would be more helpful if this pointed to the relevant config/command-line option(s).

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

I think the above two should check that a test actually ran.

thopre updated this revision to Diff 323744.Feb 15 2021, 7:20 AM
thopre marked 4 inline comments as done.

Address review comments

thopre added inline comments.Feb 15 2021, 2:23 PM
llvm/docs/CommandGuide/lit.rst
381

I think a mix of discoverable tests and non discoverable ones is indeed unlikely and also non desirable because then when adding a new tests if it is not discoverable you cannot know whether it was intentional or an error (e.g. suffixes should have been modified to add a new suffix).

I'm reluctant to use suffixes being unset to disable the indirect check because not setting it might have been an error (i.e. someone creates a new directory, adds tests and a config name but forgets the suffixes). Besides suffixes is not used for all lit format (googletest ignores it).

jdenny accepted this revision.Feb 16 2021, 8:55 AM

LGTM except for the one minor change to the diagnostic. Thanks for all the work.

llvm/docs/CommandGuide/lit.rst
381

Besides suffixes is not used for all lit format (googletest ignores it).

Ah, makes sense.

llvm/utils/lit/lit/discovery.py
178–179

Please also mention standalone_tests as it's a lesser known way past this error.

This revision is now accepted and ready to land.Feb 16 2021, 8:55 AM
jdenny added inline comments.Feb 16 2021, 9:00 AM
llvm/docs/CommandGuide/lit.rst
380

One last request: can we indicate what values this accepts? I understand you're mimicking other documentation, but I find "Mark a" to be unclear.

thopre updated this revision to Diff 324031.Feb 16 2021, 9:13 AM
thopre marked 2 inline comments as done.

Address review comments

This revision was landed with ongoing or failed builds.Feb 17 2021, 2:39 AM
This revision was automatically updated to reflect the committed changes.