feat(zest): support parallel test execution#84
Merged
16bit-ykiko merged 16 commits intomainfrom Mar 30, 2026
Merged
Conversation
Add `bool parallel` to RunnerOptions and `--parallel` CLI flag. When enabled, all test cases run concurrently via std::jthread and results are printed after completion. Also make deferred_cancel_await in when_tests thread-safe via thread_local. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds parallel test execution to Zest with per-test serial attributes, merges suite/case attributes, introduces thread-local state in tests and runtime helpers, adds a tcp acceptor local-port API, and serializes libuv process spawning. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Zest Runner
participant Collector as Test Collector
participant Partitioner as Partitioner
participant WorkerPool as Worker Pool
participant SerialExec as Serial Executor
participant Aggregator as Result Aggregator
Runner->>Collector: collect runnable tests & active_suites
Collector-->>Runner: runnable[]
alt parallel mode
Runner->>Partitioner: split runnable[] by attrs.serial
Partitioner-->>Runner: parallel_tasks[], serial_tasks[]
Runner->>WorkerPool: start workers (parallel_workers)
loop workers fetch tasks
WorkerPool->>WorkerPool: fetch next task (atomic index)
WorkerPool->>WorkerPool: run test, measure duration
WorkerPool-->>Runner: store result at task index
end
WorkerPool-->>Runner: all parallel results computed
Runner->>SerialExec: run serial_tasks sequentially
SerialExec-->>Runner: serial results
else sequential mode
loop for each test in runnable[]
Runner->>SerialExec: run test sequentially
SerialExec-->>Runner: result
end
end
Runner->>Aggregator: print per-test output & update totals
Aggregator-->>Runner: final summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zest/runner.cpp`:
- Around line 368-378: The current loop accumulates per-test durations into
summary.duration which yields CPU-time-like totals in parallel runs; detect
parallel execution (e.g., via options.parallel or the runner's parallel flag)
and, when parallel, replace the summed duration with a measured wall-clock
elapsed time taken for the whole run (measure steady_clock before starting tests
and after all threads join and assign that to summary.duration), while retaining
the per-test sum in a separate variable (e.g., cpu_time_sum) if you want to keep
both metrics; otherwise leave the sequential behavior unchanged (keep the
existing per-test summation in the loop) and update print_run_result/summary
only after choosing wall-clock vs summed duration.
- Around line 344-366: The current parallel branch spawns one std::jthread per
runnable item which can exhaust system resources; change it to use a bounded
worker pool (size = std::max(1u, std::thread::hardware_concurrency()) or a
configurable max) that pulls tasks from a queue/indices for runnable and writes
into results, and ensure worker threads are explicitly joined (or let the vector
go out of scope) instead of calling threads.clear(); update the code around
options.parallel, the creation of threads, and the place currently using
threads.emplace_back(...) and threads.clear(), keeping TestResult population
(results[i] = ...) and timing logic inside the worker task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c1aea95-3f5d-4822-811d-6fee6b5a596f
📒 Files selected for processing (3)
include/eventide/zest/run.hsrc/zest/runner.cpptests/unit/async/when_tests.cpp
- Add tcp::local_port() to query bound port from acceptor, eliminating pick_free_port() TOCTOU race by binding to port 0 directly - Convert global static state to thread_local in when_tests and deco runtime tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace N-thread-per-test with fixed-size thread pool (default: hardware_concurrency) using atomic task counter. Add --parallel-workers flag. Also fix small_vector test global state to thread_local. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zest/runner.cpp`:
- Around line 354-388: The code currently spawns num_workers jthreads regardless
of how many tests exist; clamp the worker count to the amount of work by
computing an actual_worker_count = std::min(num_workers,
static_cast<unsigned>(runnable.size() ? runnable.size() : 1)); then use
actual_worker_count for pool.reserve(...) and the spawn loop (replace
for(unsigned w = 0; w < num_workers; ++w) with for(unsigned w = 0; w <
actual_worker_count; ++w)). Ensure you use the existing symbols num_workers,
runnable.size(), pool.reserve and the jthread spawn loop when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d07666-2294-4bf8-9165-0571224b6c3b
📒 Files selected for processing (7)
include/eventide/async/io/stream.hinclude/eventide/zest/run.hsrc/async/io/acceptor.cppsrc/zest/runner.cpptests/unit/async/stream_tests.cpptests/unit/common/small_vector_tests.cpptests/unit/deco/runtime.cc
✅ Files skipped from review due to trivial changes (1)
- tests/unit/async/stream_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/eventide/zest/run.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/zest/runner.cpp (2)
371-375:⚠️ Potential issue | 🟡 MinorUse elapsed wall time for the parallel summary.
record_result()always adds per-test durations, sosummary.durationturns into a CPU-time-like sum once multiple workers run concurrently. The printed “total” can then exceed the actual elapsed runtime.♻️ Proposed fix
- auto record_result = [&](const TestResult& result) { + auto record_result = [&](const TestResult& result, bool accumulate_duration = true) { const bool failed = is_failure(result.state); print_run_result(result.display_name, failed, result.duration, options.only_failed_output); - summary.duration += result.duration; + if(accumulate_duration) { + summary.duration += result.duration; + } if(failed) { summary.failed += 1; summary.failed_tests.push_back( FailedTest{result.display_name, result.path, result.line}); } @@ if(options.parallel) { + using namespace std::chrono; + auto parallel_begin = steady_clock::now(); + // Partition: parallel-safe tests first, serial tests after. std::vector<std::size_t> parallel_indices; std::vector<std::size_t> serial_indices; @@ // Print all results in original order. for(const auto& result: results) { - record_result(result); + record_result(result, false); } + summary.duration = duration_cast<milliseconds>(steady_clock::now() - parallel_begin); } else {Also applies to: 385-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/runner.cpp` around lines 371 - 375, The summary currently accumulates per-test durations inside the record_result lambda (reference: record_result, summary.duration), which double-counts wall time when tests run in parallel; instead record a single run start timestamp before workers start and, when emitting the parallel summary, compute summary.duration as the elapsed wall time (now - run_start) rather than summing result.duration; update all places that currently add to summary.duration (including the block around print_run_result and the later summary aggregation) to use this elapsed value so the printed “total” reflects real elapsed time.
397-421:⚠️ Potential issue | 🟠 MajorClamp the pool size to the parallel batch.
--parallel-workersis still used as the reserve/spawn count even whenparallel_indicesis tiny or empty. A large value here can exhaust memory or OS threads before any test starts.♻️ Proposed fix
- const auto num_workers = + const auto configured_workers = std::max(1u, options.parallel_workers ? options.parallel_workers : std::thread::hardware_concurrency()); + const auto num_workers = + std::min<std::size_t>(configured_workers, parallel_indices.size()); std::atomic<std::size_t> next_task{0}; @@ - { + if(num_workers > 0) { std::vector<std::jthread> pool; pool.reserve(num_workers); - for(unsigned w = 0; w < num_workers; ++w) { + for(std::size_t w = 0; w < num_workers; ++w) { pool.emplace_back(worker); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/runner.cpp` around lines 397 - 421, The code currently uses num_workers (computed from options.parallel_workers or hardware_concurrency) directly when reserving and spawning the thread pool, which can create many threads even when parallel_indices is small or empty; change the spawn count to clamp to the work size by computing a worker_count = std::min(num_workers, static_cast<unsigned>(parallel_indices.size())); then use worker_count for pool.reserve(...) and the for loop that calls pool.emplace_back(worker) (replace uses of num_workers in reserve/spawn with worker_count) so you never allocate more threads than there are parallel tasks (and allow zero threads when parallel_indices is empty).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zest/runner.cpp`:
- Around line 1-10: This file relies on transitive headers for std::function and
std::max; add direct includes for <functional> (to cover the std::function usage
where callbacks/functional types are declared) and <algorithm> (to cover
std::max usage where maximum values are computed) so the symbols are always
available across toolchains and standard-library implementations.
---
Duplicate comments:
In `@src/zest/runner.cpp`:
- Around line 371-375: The summary currently accumulates per-test durations
inside the record_result lambda (reference: record_result, summary.duration),
which double-counts wall time when tests run in parallel; instead record a
single run start timestamp before workers start and, when emitting the parallel
summary, compute summary.duration as the elapsed wall time (now - run_start)
rather than summing result.duration; update all places that currently add to
summary.duration (including the block around print_run_result and the later
summary aggregation) to use this elapsed value so the printed “total” reflects
real elapsed time.
- Around line 397-421: The code currently uses num_workers (computed from
options.parallel_workers or hardware_concurrency) directly when reserving and
spawning the thread pool, which can create many threads even when
parallel_indices is small or empty; change the spawn count to clamp to the work
size by computing a worker_count = std::min(num_workers,
static_cast<unsigned>(parallel_indices.size())); then use worker_count for
pool.reserve(...) and the for loop that calls pool.emplace_back(worker) (replace
uses of num_workers in reserve/spawn with worker_count) so you never allocate
more threads than there are parallel tasks (and allow zero threads when
parallel_indices is empty).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fcd7e5c-b91b-4f2e-a2a6-54ae8cde3307
📒 Files selected for processing (10)
.github/workflows/cmake.ymlinclude/eventide/zest/detail/registry.hinclude/eventide/zest/detail/suite.hinclude/eventide/zest/macro.hsrc/async/io/fs.cppsrc/async/libuv.hsrc/zest/runner.cpptests/unit/async/cancellation_tests.cpptests/unit/async/process_tests.cpptests/unit/deco/runtime.cc
✅ Files skipped from review due to trivial changes (1)
- include/eventide/zest/detail/registry.h
- Add `serial` attribute to zest test framework (suite-level and case-level) - Fix thread-safety issues: static → thread_local for deco args helper, thread-local libuv sync loop, mutex-guarded uv_spawn - Mark process_io suite and cancellation pool-exhaustion tests as serial - Rewrite query_info_child test with pipe synchronization (no sleep) - Add parallel test run to CI alongside serial run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/parallel-test-runner
branch
from
717169c to
7d11140
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adapt serial attribute support to work with main's inheritance-based fixture pattern. Suite-level attrs are now detected via Derived::suite_attrs static member instead of a template parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/zest/runner.cpp (1)
371-375:⚠️ Potential issue | 🟡 MinorUse elapsed wall time for the parallel summary.
summary.durationis still built by summing per-test runtimes, so the final “ms total” in--parallelmode can be much larger than actual elapsed time. Measure once around the whole parallel branch instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/runner.cpp` around lines 371 - 375, The summary.duration is being accumulated from per-test durations inside the record_result lambda which inflates the total in --parallel mode; instead, record the wall-clock start timestamp before entering the parallel branch and after the parallel work finishes compute summary.duration = elapsed_ms (overwriting the summed value) so the final summary uses real elapsed time; adjust code around the parallel execution block and stop updating summary.duration inside record_result for parallel runs (use options.parallel or the parallel execution scope to detect this) and keep print_run_result/is_failure/record_result behavior unchanged except not summing durations for parallel mode.
🧹 Nitpick comments (2)
.github/workflows/cmake.yml (1)
168-169: If CI time becomes a problem, narrow the extra parallel pass.
tests/CMakeLists.txt:188already runsunit_testsviactest, so these lines execute the suite twice in every matrix job. A smaller representative slice usually keeps the new path covered with much less CI time and flake surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cmake.yml around lines 168 - 169, The workflow is running the entire test suite twice because tests/CMakeLists.txt already runs unit_tests via ctest; remove or narrow the explicit "${build_dir}/unit_tests" --parallel invocation: either delete these two lines to rely on the existing ctest run, or replace them with a targeted, small representative invocation such as running the test binary with a specific filter (e.g., "${build_dir}/unit_tests" --gtest_filter=SomeSmokeTest* --parallel) or invoking ctest with a focused regex (ctest -R smoke -j ...), so the new parallel path covers only a small slice rather than the full suite.src/zest/runner.cpp (1)
53-62: Don’t silently ignore--parallel-workers.
unit_tests --parallel-workers 8still runs sequentially unless--parallelis also set. Either reject that combination here or make a nonzero worker count imply parallel mode.💡 One simple way to tighten this up
auto to_runner_options(ZestCliOptions options) -> std::expected<eventide::zest::RunnerOptions, std::string> { if(options.test_filter_input.has_value() && !options.test_filter->empty()) { return std::unexpected("cannot use both positional filter and --test-filter"); } + if(*options.parallel_workers != 0 && !*options.parallel) { + return std::unexpected("--parallel-workers requires --parallel"); + } eventide::zest::RunnerOptions runner_options;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zest/runner.cpp` around lines 53 - 62, to_runner_options currently allows a nonzero options.parallel_workers to be set while options.parallel is false, causing sequential execution; update to_runner_options to either (a) validate and reject the combination by returning std::unexpected when options.parallel_workers > 0 and !*options.parallel, or (b) infer parallel mode by setting runner_options.parallel = true whenever *options.parallel_workers > 0; implement the chosen behavior in to_runner_options (use symbols options.parallel_workers, options.parallel, to_runner_options, and eventide::zest::RunnerOptions::parallel/parallel_workers to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/async/io/fs.cpp`:
- Around line 626-638: The sync_loop() implementation currently ignores return
values from uv_loop_init and uv_loop_close; change sync_loop_holder to store the
uv_loop_init return code (e.g., an int init_result) and, because sync_loop() is
noexcept, call std::terminate() from the constructor if uv_loop_init(&loop)
returns non-zero, and likewise call uv_loop_close(&loop) in the destructor and
call std::terminate() if uv_loop_close returns non-zero (or UV_EBUSY), so the
code never silently returns an invalid/Leaked loop; reference the
sync_loop_holder struct, its constructor/destructor, uv_loop_init,
uv_loop_close, and holder.loop when making the changes.
In `@src/zest/runner.cpp`:
- Around line 416-424: The current worker pool uses std::vector<std::thread>
pool and manually joins threads, which can call std::terminate if
emplace_back(worker) throws; change the container to std::vector<std::jthread>
pool so each jthread will automatically join on destruction, keep
pool.reserve(num_workers), continue to emplace_back(worker) (jthread is
movable), and remove the manual join loop (for(auto& t: pool) t.join();). Ensure
the code still references num_workers and worker unchanged and that <thread> is
available for std::jthread.
---
Duplicate comments:
In `@src/zest/runner.cpp`:
- Around line 371-375: The summary.duration is being accumulated from per-test
durations inside the record_result lambda which inflates the total in --parallel
mode; instead, record the wall-clock start timestamp before entering the
parallel branch and after the parallel work finishes compute summary.duration =
elapsed_ms (overwriting the summed value) so the final summary uses real elapsed
time; adjust code around the parallel execution block and stop updating
summary.duration inside record_result for parallel runs (use options.parallel or
the parallel execution scope to detect this) and keep
print_run_result/is_failure/record_result behavior unchanged except not summing
durations for parallel mode.
---
Nitpick comments:
In @.github/workflows/cmake.yml:
- Around line 168-169: The workflow is running the entire test suite twice
because tests/CMakeLists.txt already runs unit_tests via ctest; remove or narrow
the explicit "${build_dir}/unit_tests" --parallel invocation: either delete
these two lines to rely on the existing ctest run, or replace them with a
targeted, small representative invocation such as running the test binary with a
specific filter (e.g., "${build_dir}/unit_tests" --gtest_filter=SomeSmokeTest*
--parallel) or invoking ctest with a focused regex (ctest -R smoke -j ...), so
the new parallel path covers only a small slice rather than the full suite.
In `@src/zest/runner.cpp`:
- Around line 53-62: to_runner_options currently allows a nonzero
options.parallel_workers to be set while options.parallel is false, causing
sequential execution; update to_runner_options to either (a) validate and reject
the combination by returning std::unexpected when options.parallel_workers > 0
and !*options.parallel, or (b) infer parallel mode by setting
runner_options.parallel = true whenever *options.parallel_workers > 0; implement
the chosen behavior in to_runner_options (use symbols options.parallel_workers,
options.parallel, to_runner_options, and
eventide::zest::RunnerOptions::parallel/parallel_workers to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18dc8d9a-98b0-41b0-900d-d7fd00f5e1ec
📒 Files selected for processing (9)
.github/workflows/cmake.ymlinclude/eventide/zest/detail/registry.hinclude/eventide/zest/detail/suite.hsrc/async/io/fs.cppsrc/async/libuv.hsrc/zest/runner.cpptests/unit/async/cancellation_tests.cpptests/unit/async/process_tests.cpptests/unit/deco/runtime.cc
✅ Files skipped from review due to trivial changes (2)
- tests/unit/async/cancellation_tests.cpp
- tests/unit/deco/runtime.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- include/eventide/zest/detail/registry.h
- include/eventide/zest/detail/suite.h
- tests/unit/async/process_tests.cpp
pipe.connect_failure and pipe.stop use a hardcoded pipe name on Windows, causing conflicts when run in parallel. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…E usage
TEST_CASE(name, TestAttrs{.serial = true}) instead of
TEST_CASE(name, zest::TestAttrs{.serial = true})
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TEST_CASE(name, serial = true) instead of
TEST_CASE(name, TestAttrs{.serial = true})
Uses the same DECO pattern: __VA_ARGS__ becomes statements inside
a constructor of a struct inheriting from TestAttrs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TEST_SUITE(process_io, loop_fixture) {
TEST_SUITE_ATTRS(serial = true);
...
};
Eliminates the need for a separate fixture struct just to carry attrs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add missing <algorithm> and <functional> includes - Use wall-clock time for parallel mode summary duration - Clamp thread pool size to number of parallel tests - Add error handling for uv_loop_init/uv_loop_close Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use designated initializers instead of local struct in constexpr lambda for TEST_CASE/TEST_SUITE_ATTRS — GCC rejects constructing a local struct within the same constexpr lambda where it's defined. Syntax is now: TEST_CASE(name, .serial = true) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace constexpr local struct (GCC-incompatible) and designated initializers with a ZEST_MAKE_ATTRS helper that declares local references to TestAttrs fields, enabling plain assignment syntax: TEST_CASE(name, serial = true) TEST_SUITE_ATTRS(serial = true) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace individual field references with `auto& [skip, focus, serial] = _a;` to eliminate duplication between TestAttrs fields and macro variables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bool paralleltoRunnerOptionsand--parallelCLI flagstd::jthread, results printed after completiondeferred_cancel_awaitin when_tests thread-safe viathread_localTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Chores