Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run test builds in parallel with tests, for faster test results #12327

Open
kpreid opened this issue Jul 4, 2023 · 4 comments
Open

Run test builds in parallel with tests, for faster test results #12327

kpreid opened this issue Jul 4, 2023 · 4 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test E-hard Experience: Hard S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@kpreid
Copy link
Contributor

kpreid commented Jul 4, 2023

Problem

Right now, cargo test will compile all test binaries that are to be run, and then run them.

Overall performance in any multi-crate project (workspace or lib & bin) could be improved by concurrently compiling and running tests, because:

  • While both builds and libtest tests try to use all available parallelism, they may not have enough tasks to succeed; for example, a crate's test suite might have one or two very long-running #[test] functions.

    • As long as test binaries are being executed serially, there will be some periods of low parallelism as one test suite wraps up its last tests before the next one starts.
  • Suppose that crate B depends on crate A, crate B takes significant time to build, and the developer has just modified crate A in a way which introduces a bug. In this case, running the test suite for crate A while crate B is being built will allow the developer to see the failure faster, canceling the build of crate B instead of waiting for it unnecessarily. This is the way in which this proposal provides benefits beyond cargo test --all should run tests in parallel #5609.

    (In --no-fail-fast mode, B would not be canceled, but the overall process still produces useful information quicker.)

Proposed Solution

Perform the build the same as currently, but

  • As soon as one test binary has been built, if no test binaries are being run, start it running.
  • While the test binary is being run, compilation should behave like --quiet mode, producing no output.
  • After the test binary exits,
    • any diagnostics that were produced in the meantime should be printed, possibly resulting in aborting the build,
    • and progress reporting should resume until there is another test binary to run.

Another description of the scheduling behavior would be that “run test” becomes just another node in the build dependency graph, except that only one can run at a time (for now, unless and until #5609 happens).

Note that tests/ still must wait for all binaries of the package to be built since they are allowed to run the binaries, but lib unittests need not.

Notes

This has similar goals to #5609cargo test --all should run tests in parallel”, but it has fewer implications for the tests and may be easier to implement.

  • It proposes mixing building and testing, not (just) testing and testing.
  • it does not break tests that assume they will run serially; it should not break any tests unless they are inspecting the target/ directory and expecting it to be the result of a complete package/workspace build.
  • It does not require capturing output of tests; the “foreground” of the terminal is always a single test binary. This also means that it will not interfere with noticing that a test is slow by watching the speed of the output.

Even if this does not happen first, I would like to see a world in which both forms of parallelism are supported; this would be the same as I describe above, except that there is no rule that a single test binary at a time should be run, and output capturing is necessary.

@kpreid kpreid added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Jul 4, 2023
@epage
Copy link
Contributor

epage commented Jul 4, 2023

So if I'm understanding this correctly, nothing changes with libtest, we just kick off tests sooner, right?

In a way I was thinking of that with #5609 but I can see value in tracking these two halves separately.

it does not break tests that assume they will run serially; it should not break any tests unless they are inspecting the target/ directory and expecting it to be the result of a complete package/workspace build.

imo this issue would be blocked on #5609 rather than being completely independent as there wouldn't be a way for us to globally control the level of parallel work being done.

@epage
Copy link
Contributor

epage commented Jul 4, 2023

Also, something we'd need to keep in mind that would make this more complex is that we guarantee for integration tests that the relevant binaries have been built.

@kpreid
Copy link
Contributor Author

kpreid commented Jul 4, 2023

So if I'm understanding this correctly, nothing changes with libtest, we just kick off tests sooner, right?

Correct; this is purely a change to cargo and it works equally well with harness = false as with whatever libtest chooses to do.

imo this issue would be blocked on #5609 rather than being completely independent as there wouldn't be a way for us to globally control the level of parallel work being done.

You mean, it's not feasible for cargo to notice that it's currently already running a run-test-binary job and block others on it?

we guarantee for integration tests that the relevant binaries have been built.

Already mentioned in the Proposed Solution section. I was hoping that this could be straightforwardly expressed by more edges in the dependency graph, but I admit I don't know how Cargo works internally.

@epage
Copy link
Contributor

epage commented Jul 5, 2023

You mean, it's not feasible for cargo to notice that it's currently already running a run-test-binary job and block others on it?

What I mean is that right now, you can specify how many build jobs and test jobs can run concurrently but right now those are sequenced to not overlap. Users controlling the total number of jobs would not be able to. By default, we'd be building with nearly all cores for builds and testing with all cores for tests.

Already mentioned in the Proposed Solution section. I was hoping that this could be straightforwardly expressed by more edges in the dependency graph, but I admit I don't know how Cargo works internally.

Running of tests is likely to be independent such that it wouldn't be "just add more edges".

Overall, I feel like anything we do for this will require a significant reworking and then be thrown out when we do #5609 which again makes me feel like this should be blocked on #5609

In considering the relative priority, it also doesn't help that I don't see as much gain from this. From a performance perspective, running test binaries in parallel can offer big improvements because we have so many gates where all threads have to wind down before we start them up again while the build -> test transition only has a single gate. Independent of comparison with others, I have a hard time seeing us get much benefit from that one gate.

On the other hand, another area of major cost for this feature is on the reporting side. We'd have to put in a lot of work to tune the mixing of test failures and compilation failures and most likely we'd want the test UX changes that would be unblocked by #5609 and some of the console output reporting changes I want for #8889 to have a satisfactory UX.

@weihanglo weihanglo added E-hard Experience: Hard S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test E-hard Experience: Hard S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
Status: No status
Development

No branches or pull requests

3 participants