Page MenuHomePhabricator

[lit] Introduce setup and teardown routines
AbandonedPublic

Authored by broadwaylamb on Mar 26 2020, 1:24 AM.

Details

Summary

This patch adds a new feature to lit. We can now define setup and teardown routines that will be run before the whole test suite starts and after it completes, respectively.

The primary use case for this feature is running std::filesystem tests in libc++ on a remote target. There, we need to copy test inputs to the target machine via SSH, run the tests on those inputs, and then, after all the tests have been run, we need to remove those test inputs from the target machine.

Diff Detail

Event Timeline

broadwaylamb created this revision.Mar 26 2020, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 1:24 AM

Can't we do that with a custom executor in libc++?

@ldionne I tried to find a way but couldn't. All the code in executor classes is run either at configuration time (when we parse lit.cfg), or while running each test. There's no way to tell it to run "when all tests have completed", AFAICT.

@ldionne I tried to find a way but couldn't. All the code in executor classes is run either at configuration time (when we parse lit.cfg), or while running each test. There's no way to tell it to run "when all tests have completed", AFAICT.

The executor does receive file dependencies. It could copy those and remove them after each test, no? I'm not saying setup/teardown isn't useful in lit, I'm just trying to see what other ways we can solve that problem.

@ldionne I tried to find a way but couldn't. All the code in executor classes is run either at configuration time (when we parse lit.cfg), or while running each test. There's no way to tell it to run "when all tests have completed", AFAICT.

The executor does receive file dependencies. It could copy those and remove them after each test, no? I'm not saying setup/teardown isn't useful in lit, I'm just trying to see what other ways we can solve that problem.

It probably could. However, it would be unnecessary work, leading to slowing down test execution time significantly. Copying the inputs involves tarring them, creating a temporary directory on the target, scp'ing the archive to that temporary directory and untarring it. For me, it can easily take ~10 seconds (yeah, my target machine is on another hemisphere, but still). And we have 134 std::filesystem tests, that's ~22 minutes spent only on copying the inputs to the target. And I haven't even counted the cleanup time.

yln added a comment.Mar 26 2020, 3:59 PM

I can already think of at least one other use case that currently doesn't have a good solution: setting up iOS simulator/devices before and after compiler-rt tests.

The only concern that I have is that in lit we have this concept of "suites" defined by lit configs. These lit config now also register setup/teardown functions, but the functions get executed at the beginning/end of the overall lit command line invocation. This is a mismatch. But maybe this is not important?! Do scenarios where setup matters already equate "lit invocation" with "test suite"?

This is what I am trying to depict:

lit command line invocation - start
  suite 1 setup
  suite 2 setup
  suite 1
    test 1 in suite 1
    test 2 in suite 1
  suite 2
    test 1 in suite 2
    test 2 in suite 2
  suite 1 teardown
  suite 2 teardown
lit command line invocation - end

Ideally, setup/teardown would execute around the suite that defined it. We can't easily accomplish this because after test discovery, a "lit run" (run.py) is just the list of all discovered tests.

Still, from my perspective this is a good feature to have. LGTM, with nits from my side. I would like someone else to agree with me before landing though.

llvm/utils/lit/lit/LitConfig.py
73

I think I can guess why this is required, but please explain it just to make sure I understand.

185

Can we name this suite_setup to drive home the point that this is executed once, and not for every test?

215

Should we provide self (the lit_config) or is this implicitly captured on the caller side?

llvm/utils/lit/lit/run.py
73

My apologies for this. The explicit modeling of serial and parallel runs is my fault. That abstraction has not carried its weight. I want to remove it in the future.

llvm/utils/lit/tests/Inputs/setup-teardown/lit.cfg
11

Can this access the surrounding context?

llvm/utils/lit/tests/setup-teardown.py
2

If you provide -j1, then you can skip the -DAG below.

broadwaylamb marked 2 inline comments as done.
  • Rename setup_callback to suite_setup, teardown_callback to suite_teardown.
  • Add a comment about pickling LitConfig.
  • Update the test to use -j1.
broadwaylamb marked an inline comment as done.Mar 27 2020, 5:44 AM
In D76829#1945166, @yln wrote:

Ideally, setup/teardown would execute around the suite that defined it. We can't easily accomplish this because after test discovery, a "lit run" (run.py) is just the list of all discovered tests.

Yes, I thought about it too and indeed it's not easy to do. Not only because the information about suites is "lost", but also because I think it would significantly complicate the logic in case of parallelism.
I agree that although it would be a natural thing to do, I don't see any issue if we run those routines before and after the whole lit invocation.

llvm/utils/lit/lit/LitConfig.py
215

Yes, it can be captured.

llvm/utils/lit/tests/Inputs/setup-teardown/lit.cfg
11

Yes.

yln added a reviewer: rnk.Mar 27 2020, 10:06 AM

LGTM. @delcypher @rnk: what do you think?

@ldionne I tried to find a way but couldn't. All the code in executor classes is run either at configuration time (when we parse lit.cfg), or while running each test. There's no way to tell it to run "when all tests have completed", AFAICT.

The executor does receive file dependencies. It could copy those and remove them after each test, no? I'm not saying setup/teardown isn't useful in lit, I'm just trying to see what other ways we can solve that problem.

It probably could. However, it would be unnecessary work, leading to slowing down test execution time significantly. Copying the inputs involves tarring them, creating a temporary directory on the target, scp'ing the archive to that temporary directory and untarring it. For me, it can easily take ~10 seconds (yeah, my target machine is on another hemisphere, but still). And we have 134 std::filesystem tests, that's ~22 minutes spent only on copying the inputs to the target. And I haven't even counted the cleanup time.

FWIW, that's needed for correctness because tests could modify these files. When running locally, we would also need to copy the files to temporary directories, but we don't do it.

I'm curious to know -- what are you tarring up exactly? We have only a few small inputs, don't we?

FWIW, that's needed for correctness because tests could modify these files. When running locally, we would also need to copy the files to temporary directories, but we don't do it.

If such tests are introduced (those that modify their inputs), we'll just copy their inputs only for them. But there are no such tests now.

I'm curious to know -- what are you tarring up exactly? We have only a few small inputs, don't we?

Only this directory: libcxx/test/std/input.output/filesystems/Inputs

rnk added a subscriber: zturner.Mar 27 2020, 1:15 PM

Given the use cases described, I think what you have is probably the best way forward.

However, I'd add that setup steps are not parallelized. Any work that lit does up front slows the test suite down significantly, so in general we should prefer to do any set up with *in* the test, rather than up front. For your use case, the overhead comes from round trip network latency, not CPU time, so up front set up makes sense.

In the past, @zturner wanted to create clean output directories for every test up front, so that %T wouldn't be as dangerous. I forget if it landed, but I remember that one version of the patch did a lot of IO up front (deleting all temp files from the last test run), and he had to rework it to do the FS work during test execution.

FWIW, that's needed for correctness because tests could modify these files. When running locally, we would also need to copy the files to temporary directories, but we don't do it.

If such tests are introduced (those that modify their inputs), we'll just copy their inputs only for them. But there are no such tests now.

Well, those are tests, so if they do the wrong thing (and fail), what tells us they didn't modify the files? We're basically relying on the tests working correctly for the test suite to be correct, which is not very good. The only way of really being correct in libc++'s case is to copy the files and run the test on those copies. The only reason we're not doing it right now is because we're being lazy, but I'm actually working on fixing that right now.

I personally believe that a more useful feature for lit would be to allow batching sets of RUN commands or something like that. For example, imagine if we could compile all the tests locally and only then execute them on the remote host -- you wouldn't only save the cost of scping N times, you would also save on all the ssh's. That would be a major improvement. To me, trying to just save on copying the inputs is taking the easy way for less benefit, and it's not clear to me this will even work with the upcoming libc++ lit configuration, where each test will require its copy of the inputs.

By the way, I don't mean to say that this patch isn't useful -- I'm just saying that in the context of trying to solve the specific problem at hand (libc++'s std::filesystem on a remote host), I'm not convinced this is the right thing.

yln added a comment.Mar 27 2020, 3:20 PM

I personally believe that a more useful feature for lit would be to allow batching sets of RUN commands or something like that. For example, imagine if we could compile all the tests locally and only then execute them on the remote host -- you wouldn't only save the cost of scping N times, you would also save on all the ssh's. That would be a major improvement. To me, trying to just save on copying the inputs is taking the easy way for less benefit, and it's not clear to me this will even work with the upcoming libc++ lit configuration, where each test will require its copy of the inputs.

By the way, I don't mean to say that this patch isn't useful -- I'm just saying that in the context of trying to solve the specific problem at hand (libc++'s std::filesystem on a remote host), I'm not convinced this is the right thing.

Without a strong use case from libc++ I would prefer to hold off on this and see if it is still needed after we made the other improvements.

In D76829#1946422, @yln wrote:

LGTM. @delcypher @rnk: what do you think?

I'd quite like to see the feature land for the reason you mentioned with the iOS simulator.

delcypher added inline comments.Mar 27 2020, 7:48 PM
llvm/utils/lit/lit/LitConfig.py
71

Is suite really the best name here? IIUC a lit invocation can discover multiple test suites but the callbacks in this patch aren't executed when a suite starts/finishes. IIUC the implementation calls the setup calls backs before any tests run and then calls the teardown callbacks after all tests have been executed. In that sense the callbacks are actually global to the entire lit invocation, even though the callbacks originate from a particular suite.

Wouldn't _global_setup_callbacks (or _global_pre_test_callbacks) and _global_teardown_callbacks (or _global_post_test_callbacks) with similar changes elsewhere be a more descriptive name?

yln added a comment.Apr 24 2020, 4:19 PM

I'd quite like to see the feature land for the reason you mentioned with the iOS simulator.

Note: we just solved this issue in a different way (by eliminating the need to boot/shutdown simulator instances).

yln resigned from this revision.Apr 24 2020, 4:19 PM
broadwaylamb abandoned this revision.Apr 24 2020, 4:49 PM