This is an archive of the discontinued LLVM Phabricator instance.

[lit] Allow for tests to have non-parsed requirements
ClosedPublic

Authored by CaseyCarter on Jun 12 2020, 7:05 PM.

Details

Summary

MSVC uses lit for STL testing, running both the libcxx tests and our "native" suite of tests which has feature requirements that are not parsed from the test content, but supplied externally. dac21fd2 broke us by ignoring the initial value of the requires property of tests and using only the result of the REQUIRES: parser.

(This is my first LIT change, apologies if I'm missing some protocol.)

Diff Detail

Event Timeline

CaseyCarter created this revision.Jun 12 2020, 7:05 PM
ldionne requested changes to this revision.Jun 15 2020, 8:25 AM

How do you set test.requires externally? Do you have a custom test format?

This LGTM generally speaking, with some details. Also, can we add some tests? The tests are in llvm/utils/lit/tests.

llvm/utils/lit/lit/TestRunner.py
1465–1466

I think this can always be just test.requires because test.requires is set to [] on construction. Thus you'd end up adding both the requires set programatically on the test and the parsed ones.

1465–1466

I think we should have consistent behaviour for test.xfails and test.unsupported`.

This revision now requires changes to proceed.Jun 15 2020, 8:25 AM

Yes, we have a custom test format which considers the configured target architecture and the set of flags the test will be compiled with.

Yes, we have a custom test format which considers the configured target architecture and the set of flags the test will be compiled with.

Is that test format available on GitHub or somewhere else? I'd love to take a look.

It can be found here. It was based loosely off of the old libcxx test format. I'm probably going to submit a pretty significant re-working of it and/or see if I can adapt the new format to our needs whenever I have a free minute.

It can be found here. It was based loosely off of the old libcxx test format. I'm probably going to submit a pretty significant re-working of it and/or see if I can adapt the new format to our needs whenever I have a free minute.

Nice! I think you should look at https://reviews.llvm.org/D81846 then -- it would allow specifying your own Lit config entirely from libc++'s CMake configuration. This way, you could point Lit to your own configuration made entirely from scratch. I also have examples of how to setup configurations from scratch that I'm going to submit patches for soon.

Partially address review comments.

CaseyCarter marked 2 inline comments as done.

Hack together test.

I attempted to address everything. The test may be terrible - I've used LIT casually but never peeked inside until yesterday - so please don't refrain from telling me how to improve it.

ldionne added inline comments.Jun 15 2020, 2:31 PM
llvm/utils/lit/tests/unparsed-requirements.py
2

Is 2> %t.err necessary?

9

I think this LGTM, but you should also test xfails and unsupported.

16

You could also avoid creating a new class CustomTest and instead use:

test = Test(<args...>)
test.requires = ["meow"]

This might be less brittle in case we decide to change the signature of lit.Test's __init__ method.

  • Remove copy-pasta from RUN line in unparsed-requirements.py.
  • Simplify code with +=
  • Complete test coverage
  • Change line endings to LF in new files
CaseyCarter marked 5 inline comments as done.Jun 15 2020, 3:52 PM
CaseyCarter added inline comments.
llvm/utils/lit/tests/unparsed-requirements.py
2

Nope - it's copy-pasta.

9

I was so shocked to get the test working that I forgot it wasn't complete!

ldionne accepted this revision.Jun 15 2020, 4:37 PM

LGTM.

This revision is now accepted and ready to land.Jun 15 2020, 4:37 PM
CaseyCarter marked 2 inline comments as done.Jun 15 2020, 4:42 PM

Thanks for the help, Louis!

This revision was automatically updated to reflect the committed changes.