Page MenuHomePhabricator

[lit] Allow passing extra commands to executeShTest
ClosedPublic

Authored by ldionne on Mar 17 2020, 8:29 AM.

Details

Summary

This allows creating custom test formats on top of executeShTest that inject commands at the beginning of the file being parsed, without requiring these commands to physically appear in the test file itself.

For example, one could define a test format that prints out additional debug information at the beginning of each test. More realistically, this has been used to define custom test formats like one that supports compilation failure tests (e.g. with the extension compile.fail.cpp) by injecting a command that calls the compiler on the file itself and expects it to fail.

Without this change, the only alternative is to create a temporary file with the same content as the original test, then prepend the desired // RUN: lines to that file, and call executeShTest on that file instead. This is both slow and cumbersome to do.

Diff Detail

Event Timeline

ldionne created this revision.Mar 17 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 8:29 AM

I couldn't find existing unit tests for executeShTest that I could have added to, but please LMK if there are some.

ldionne added a reviewer: yln.Mar 18 2020, 3:12 PM
yln accepted this revision.Mar 18 2020, 4:59 PM

I don't understand yet what this enables. Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.
Does this enable deriving new tests from existing source files or running existing tests (.{pass,fail,sh}.cpp) in a different way without needing to modify those dozens/hundreds of files?

The change itself looks innocent enough and I don't want to block you on adding a test if there are currently none at all. LGTM.

This revision is now accepted and ready to land.Mar 18 2020, 4:59 PM
yln added a reviewer: rnk.Mar 18 2020, 5:00 PM
yln added inline comments.
llvm/utils/lit/lit/TestRunner.py
1497

I think Python "truthiness" allows for not script here.

In D76290#1930134, @yln wrote:

I don't understand yet what this enables. Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.
Does this enable deriving new tests from existing source files or running existing tests (.{pass,fail,sh}.cpp) in a different way without needing to modify those dozens/hundreds of files?

The change itself looks innocent enough and I don't want to block you on adding a test if there are currently none at all. LGTM.

I'm a little uncomfortable with declaring bankruptcy on the tests. Doesn't the lit test suite itself use executeShTest? Can we add a test there?

This would also be great to document if there's a place. If there isn't, maybe we can change (maybe just one of) the libcxx tests to use this in the same commit, so that it's obvious in a blame what this is for.

ldionne updated this revision to Diff 251257.Mar 18 2020, 8:40 PM
ldionne marked an inline comment as done.

Add tests, reword commit, use Python truthiness to full extent

In D76290#1930134, @yln wrote:

I don't understand yet what this enables.

It enables injecting commands at the beginning of an arbitrary file, even if there are no commands in that file. That makes the ShTest format a lot more powerful, cause new formats can be built on top of it. For example, this is the new test format I'm working on for libc++, which is both more powerful and *a lot* simpler than the current test format: https://github.com/ldionne/llvm-project/blob/a60ed4470/libcxx/utils/libcxx/test/newformat.py#L79-L138.

Without this change, I have to create a temporary file for each test, copy it, prepend // RUN: XXX commands to it, and then re-create an artificial Lit test that uses that file. It's not great and it's a lot slower.

Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.

I've tried to better describe what the change enables.

I'm a little uncomfortable with declaring bankruptcy on the tests. Doesn't the lit test suite itself use executeShTest? Can we add a test there?

I agree, and you're very good at making me write tests :-). I'm now more familiar with how lit tests work and I've managed to add some.

@dexonsmith @yln
Please let me know if the new revision seems OK to you!

ldionne edited the summary of this revision. (Show Details)Mar 18 2020, 8:41 PM
delcypher added inline comments.Mar 19 2020, 11:56 AM
llvm/utils/lit/lit/TestRunner.py
1492

I don't think extra_commands is a command good name. It's not obvious that these are injected as extra commands at the beginning. Here are a few name suggestions

  • preamble_commands
  • prologue_commands
delcypher requested changes to this revision.Mar 19 2020, 11:56 AM
This revision now requires changes to proceed.Mar 19 2020, 11:56 AM
ldionne updated this revision to Diff 251639.Mar 20 2020, 7:36 AM
ldionne edited the summary of this revision. (Show Details)

Renamed to preamble_commands

ldionne marked an inline comment as done.Mar 20 2020, 7:41 AM

I renamed to preamble_commands, @delcypher LMK if that looks better now!

delcypher added inline comments.Mar 20 2020, 5:25 PM
llvm/utils/lit/tests/shtest-inject.py
2

These tests require an external shell. I don't know if that option is available on all the platforms where this test runs (e.g. Windows). Do you need to put a REQUIRES: <something> on this test?

ldionne marked 2 inline comments as done.Mar 20 2020, 6:00 PM
ldionne added inline comments.
llvm/utils/lit/tests/shtest-inject.py
2

These tests follow the exact format of the other tests in this directory, so this must not be an issue. Can I commit this?

delcypher accepted this revision.Mar 24 2020, 11:57 AM
delcypher added inline comments.
llvm/utils/lit/tests/shtest-inject.py
2

Ok.

This revision is now accepted and ready to land.Mar 24 2020, 11:57 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.