Page MenuHomePhabricator

[libc++] Add an alternative Lit test format
ClosedPublic

Authored by ldionne on Apr 2 2020, 2:23 PM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Restricted Project
Commits
rG80a2ddf65ccd: [libc++] Add an alternative Lit test format
Summary

This new test format is simpler and more flexible. It creates Lit ShTests
on the fly that reuse existing substitutions (like %{cxx}) instead of
having complex logic in Python to run the tests. This has the benefit
that virtually no coding is required to customize how the test suite is
run -- one can achieve pretty much anything by defining the appropriate
substitutions in a simple lit.cfg file.

For example, in order to run the tests on an embedded device after
building with a specific SDK, one can set the %{cxx} and %{compile_flags}
substitutions to use that SDK, and the %{exec} substitution to the ssh.py
script currently used for .sh.cpp tests with a remote executor. Dealing with
the SSHExecutor becomes unnecessary, since all tests are treated like ShTests.

As a side effect of this design, configuration files for the test
suite can be as simple as:

config.substitutions.append(('%{cxx}', '<path-to-compiler>'))
config.substitutions.append(('%{compile_flags}', '<flags>'))
config.substitutions.append(('%{link_flags}', '<flags>'))
config.substitutions.append(('%{exec}', '<script-to-execute>'))

This should allow storing lit.cfg files for various configurations
directly in the repository instead of relying on complicated logic
in config.py to set up the right flags. I've found numerous problems
in that logic in the past years, and it seems like having simple and
explicit configuration files for the configurations we support is
going to solve most of these problems. Specifically, I am hoping to
store configuration files for testing other Standard Libraries in
the repository.

Improving the interaction with the test suite configuration is still a
work in progress, so for now this test format reuses the substitutions and
available features that are set up by the current config.py.

This new test format should support pretty much everything that the current
test format supports, however it will not be enabled by default at first to
make sure we're satisfied with it. For a short period of time, the new format
will require --param=use_new_format=True to be enabled, however it is a very
short term goal to replace the current testing format entirely and to simplify
the configuration accordingly.

Diff Detail

Event Timeline

ldionne created this revision.Apr 2 2020, 2:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 2 2020, 2:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

This is mostly a heads-up review. I welcome comments and improvement suggestions. This patch is a no-op unless one opts into the new format, and in particular this doesn't change the interaction between CMake and config.py at all. Because of this, I will not hold off committing this in order to start adopting it more widely right away -- improvements can be made as follow-up patches as I get more feedback from people who try it out.

I have been tweaking the libc++ test suite in the past couple of weeks in order to make this format and the current format essentially equivalent. As of right now, this format allows me to run all tests locally or remotely on both macOS and our CI Docker images (including filesystem tests, which doesn't work with the old format). I especially welcome feedback from people who try it out on Windows or on configurations I'm not aware of.

The only known "problem" with the new format is that the test/libcxx/type_traits/is_pointer.arc.pass.mm test doesn't pass right now. This is actually rather troubling -- the old format is broken and skips .pass.mm tests entirely, so that test simply was never executed since I added it one year ago.

This is awesome, thanks for your work!

libcxx/utils/libcxx/test/newformat.py
117

Can this be simplified?

compilerSupportsVerify = result.code != lit.Test.FAIL
EricWF accepted this revision.Apr 3 2020, 1:59 AM
EricWF added a subscriber: EricWF.

I like tests.
Assuming all of them pass;: LGTM.

This revision is now accepted and ready to land.Apr 3 2020, 1:59 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked 2 inline comments as done.