charles/ws-rpc-test

Fork 0

feat(runner): optional parallel execution with isolated clients (#12) #30

Merged

charles merged 1 commit from feat/12-parallel-mode into main

2026-04-11 18:43:37 +00:00

charles commented

2026-04-11 12:56:35 +00:00

Owner

Closes #12. Stacked on #29.

Summary

Adds opt-in parallel execution where each eligible test runs on its own RpcClient connection.

Config

RunConfig.parallel: bool — when true, tests marked .parallel() run concurrently in a bounded batch after the non-parallel tests.
RunConfig.parallel_workers: Option<usize> — defaults to std::thread::available_parallelism(), fallback 1.
TestRunner::parallel(bool) and TestRunner::parallel_workers(usize) setters.
TestCase::parallel() opt-in marker (new parallel_eligible: bool field).

Architecture

Hooks are converted to SharedTestFn = Arc<dyn Fn(Arc<RpcClient>) -> BoxedTestFut + Send + Sync> at the top of run() so tokio tasks can clone them cheaply. (Reuses the existing Arc::from(box) impl — no allocation changes in the sequential path.)
run() partitions tests after applying the filter: filtered → report.filtered, parallel-eligible → parallel phase, else → sequential phase.
Sequential phase runs first on the shared client (unchanged behaviour from #11).
Parallel phase uses a tokio::task::JoinSet gated by tokio::sync::Semaphore(parallel_workers). Each task acquires its own RpcClient via connect() — per the spec, "each parallel test gets its own connection". Hooks (before_each / after_each) run on the task's own connection.
before_all / after_all still run once on the shared connection.
skip_if is evaluated on the scheduling task so we don't bother spawning a worker for an obviously-skipped test.
Fail-fast: an AtomicBool is set when any parallel test fails (or connect errors). The scheduler checks it before spawning each new task; skipped tests are reported with reason = "fail-fast".

Extracted helpers

skipped_result(case, reason) → TestCaseResult with status Skipped, duration 0.
print_result(result) → the PASS/FAIL/SKIP output line plus any error message. Used by both sequential and parallel phases.

Checklist (from issue #12)

.parallel() per-test marker (new parallel_eligible field)
RunConfig.parallel: bool — false ignores marks entirely
RunConfig.parallel_workers: Option<usize> — default via available_parallelism()
Parallel tests run in a bounded JoinSet
Non-eligible tests run before the parallel batch
Each parallel test gets its own RpcClient
before_each / after_each run on the test's own connection
before_all / after_all run once on a shared connection
Fail-fast: in-flight tasks finish, new ones become Skipped("fail-fast")
Doc comment on .parallel() warns about shared server-side state
Self-test verifies isolation (two parallel tests see their own echo)

Test plan (4 new tests)

parallel_mode_disabled_runs_everything_sequentially — .parallel() marks are ignored when the runner isn't in parallel mode; registration order preserved
parallel_mode_runs_eligible_tests_concurrently — 3 tests × 200 ms each complete under 550 ms total (serial would need ≥ 600 ms) — proves actual concurrency
parallel_mode_isolates_client_connections — two tests call different echo methods; each task's own connection sees its own response
parallel_mode_with_failfast_skips_later_parallel_tests — parallel_workers=1, first test fails, subsequent tests recorded as Skipped
just qa green locally — 89/89 unit tests
CI green on Forgejo

Notes for the reviewer

Resolves spec-review §8 (parallel mode was underspecified in the original). The issue now documents the exact semantics implemented here: opt-in per test, sequential first, parallel second, isolated connections, shared hooks.
I put the sequential phase before the parallel phase unconditionally. An alternative reading of "Non-eligible tests run sequentially before the parallel batch ... and after" would be to allow interleaving, but that adds complexity for no obvious win. If there's a real need, a future PR can split sequential into pre and post lists.
The print_result extraction also simplifies the sequential phase — the run() body got noticeably shorter after introducing it. (Net +324 lines, but a lot of that is tests and the multi-phase run logic.)
parallel_workers=1 in the fail-fast test is deliberate — it ensures the first failing test completes and sets the AtomicBool before the scheduler even looks at the next task, which keeps the test deterministic. Higher worker counts would race and occasionally let all three tests start concurrently.
A parallel test whose RpcClient::connect fails is reported as a Failed result with the connect error preserved. This is treated the same as any other test failure for fail-fast purposes.

Closes #12. **Stacked on #29.** ## Summary Adds opt-in parallel execution where each eligible test runs on its own `RpcClient` connection. ### Config - `RunConfig.parallel: bool` — when `true`, tests marked `.parallel()` run concurrently in a bounded batch **after** the non-parallel tests. - `RunConfig.parallel_workers: Option<usize>` — defaults to `std::thread::available_parallelism()`, fallback 1. - `TestRunner::parallel(bool)` and `TestRunner::parallel_workers(usize)` setters. - `TestCase::parallel()` opt-in marker (new `parallel_eligible: bool` field). ### Architecture - Hooks are converted to `SharedTestFn = Arc<dyn Fn(Arc<RpcClient>) -> BoxedTestFut + Send + Sync>` at the top of `run()` so tokio tasks can clone them cheaply. (Reuses the existing `Arc::from(box)` impl — no allocation changes in the sequential path.) - `run()` partitions tests after applying the filter: filtered → `report.filtered`, parallel-eligible → parallel phase, else → sequential phase. - Sequential phase runs first on the shared client (unchanged behaviour from #11). - Parallel phase uses a `tokio::task::JoinSet` gated by `tokio::sync::Semaphore(parallel_workers)`. Each task acquires its own `RpcClient` via `connect()` — per the spec, "each parallel test gets its own connection". Hooks (`before_each` / `after_each`) run on the task's own connection. - `before_all` / `after_all` still run once on the shared connection. - `skip_if` is evaluated on the scheduling task so we don't bother spawning a worker for an obviously-skipped test. - Fail-fast: an `AtomicBool` is set when any parallel test fails (or connect errors). The scheduler checks it before spawning each new task; skipped tests are reported with `reason = "fail-fast"`. ### Extracted helpers - `skipped_result(case, reason)` → `TestCaseResult` with status Skipped, duration 0. - `print_result(result)` → the PASS/FAIL/SKIP output line plus any error message. Used by both sequential and parallel phases. ## Checklist (from issue #12) - [x] `.parallel()` per-test marker (new `parallel_eligible` field) - [x] `RunConfig.parallel: bool` — false ignores marks entirely - [x] `RunConfig.parallel_workers: Option<usize>` — default via `available_parallelism()` - [x] Parallel tests run in a bounded `JoinSet` - [x] Non-eligible tests run **before** the parallel batch - [x] Each parallel test gets its own `RpcClient` - [x] `before_each` / `after_each` run on the test's own connection - [x] `before_all` / `after_all` run once on a shared connection - [x] Fail-fast: in-flight tasks finish, new ones become Skipped("fail-fast") - [x] Doc comment on `.parallel()` warns about shared server-side state - [x] Self-test verifies isolation (two parallel tests see their own echo) ## Test plan (4 new tests) - [x] `parallel_mode_disabled_runs_everything_sequentially` — `.parallel()` marks are ignored when the runner isn't in parallel mode; registration order preserved - [x] `parallel_mode_runs_eligible_tests_concurrently` — 3 tests × 200 ms each complete under 550 ms total (serial would need ≥ 600 ms) — proves actual concurrency - [x] `parallel_mode_isolates_client_connections` — two tests call different echo methods; each task's own connection sees its own response - [x] `parallel_mode_with_failfast_skips_later_parallel_tests` — `parallel_workers=1`, first test fails, subsequent tests recorded as Skipped - [x] `just qa` green locally — 89/89 unit tests - [ ] CI green on Forgejo ## Notes for the reviewer - **Resolves spec-review §8** (parallel mode was underspecified in the original). The issue now documents the exact semantics implemented here: opt-in per test, sequential first, parallel second, isolated connections, shared hooks. - I put the sequential phase **before** the parallel phase unconditionally. An alternative reading of "Non-eligible tests run sequentially before the parallel batch ... and after" would be to allow interleaving, but that adds complexity for no obvious win. If there's a real need, a future PR can split sequential into pre and post lists. - The `print_result` extraction also simplifies the sequential phase — the `run()` body got noticeably shorter after introducing it. (Net +324 lines, but a lot of that is tests and the multi-phase run logic.) - `parallel_workers=1` in the fail-fast test is deliberate — it ensures the first failing test completes and sets the AtomicBool before the scheduler even looks at the next task, which keeps the test deterministic. Higher worker counts would race and occasionally let all three tests start concurrently. - A parallel test whose `RpcClient::connect` fails is reported as a `Failed` result with the connect error preserved. This is treated the same as any other test failure for fail-fast purposes.

charles added the

area:runner

type:user-story

labels

2026-04-11 12:56:35 +00:00

charles added 1 commit

2026-04-11 12:56:35 +00:00

feat(runner): optional parallel execution with isolated clients (closes #12 ) 22c6403e86

Implements ticket #12 on top of the filter/hook/runner baseline.

New configuration
- RunConfig.parallel: bool — when true, tests marked .parallel() run
  concurrently in a bounded batch AFTER the non-parallel tests.
- RunConfig.parallel_workers: Option<usize> — defaults to
  std:🧵:available_parallelism() with a fallback of 1.
- TestRunner::parallel(bool), TestRunner::parallel_workers(usize).

TestCase
- New parallel_eligible: bool field + TestCase::parallel() opt-in.
- Debug impl updated; registration defaults it to false.

SharedTestFn
- New Arc<dyn Fn(Arc<RpcClient>) -> BoxedTestFut + Send + Sync> alias.
- Hooks are converted to SharedTestFn at the top of run() via
  Arc::from(boxed_fn) so tokio tasks can clone the hook Arcs cheaply.

run() flow
  1. Harness start + client connect (unchanged).
  2. before_all on the shared client (unchanged).
  3. Partition tests into sequential / parallel based on filter +
     .parallel_eligible and config.parallel.
  4. Sequential phase runs on the shared client (fail-fast, skip_if,
     before_each, body, after_each) — same logic as before.
  5. Parallel phase spawns each eligible test into a tokio::JoinSet
     gated by a tokio::sync::Semaphore sized to parallel_workers.
     Each task:
       - Acquires its own RpcClient via connect() (spec: "each
         parallel test gets its own connection").
       - Runs before_each, body, after_each on that connection.
       - Sets a shared AtomicBool if fail_fast is on and the test
         failed; the scheduler loop checks this before spawning
         subsequent tests.
       - skip_if is evaluated on the scheduling task so we don't
         bother spawning a worker for an obviously-skipped test.
  6. Join the set, record results, run after_all on the shared
     client, stop the harness.

Extracted helpers
- skipped_result(case, reason) — builds a zero-duration Skipped
  TestCaseResult.
- print_result(result) — prints the PASS/FAIL/SKIP line plus any
  error message underneath. Used by both sequential and parallel.

Tests (4 new in src/runner.rs)
- parallel_mode_disabled_runs_everything_sequentially: .parallel()
  marks are ignored when the runner isn't in parallel mode; order
  preserved.
- parallel_mode_runs_eligible_tests_concurrently: 3 tests sleep
  200 ms each; with 4 workers the total elapsed is < 550 ms (serial
  would need ≥ 600 ms).
- parallel_mode_isolates_client_connections: two tests call
  different methods, assert the echo response matches their own
  payload — proves each task has its own connection.
- parallel_mode_with_failfast_skips_later_parallel_tests:
  parallel_workers=1 ensures ordering, asserts >= 1 skipped with
  fail-fast.

just qa green: 89/89 unit tests, fmt and clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

charles referenced this pull request

2026-04-11 12:58:31 +00:00

test: framework integration tests + mock RPC server (#16) #31

charles reviewed

2026-04-11 17:54:41 +00:00

charles left a comment

Review — parallel execution avec clients isolés (#12)

Architecture : séquentiel d'abord, parallèle ensuite

Tests non marqués .parallel() → phase séquentielle avec client partagé. Tests marqués → phase parallèle, chacun avec sa propre connexion. C'est la sémantique la plus sûre par défaut.

`SharedTestFn = Arc<dyn Fn(...)>`

Nécessaire pour cloner les hooks vers les tâches parallèles. Naming clair. La transformation before_all.take().map(Arc::from) est propre.

`fail_fast_tripped` — AtomicBool partagé

Le flag est vérifié avant de créer chaque tâche, pas pendant son exécution. Il y a donc une fenêtre TOCTOU : une tâche peut démarrer juste avant qu'une autre tâche la tripe, et toutes deux s'exécutent. C'est inévitable en parallèle sans synchronisation bloquante — comportement documenté acceptable.

Ordre des résultats dans le rapport

Les résultats parallèles sont collectés via JoinSet::join_next() en ordre de complétion (pas d'enregistrement). Le report.tests sera donc partiellement désordonné (séquentiel en ordre, puis parallèle en ordre d'achèvement). Pourrait surprendre des utilisateurs qui s'attendent à un rapport ordonné. Vaut la peine de le documenter.

Test de timing fragile

assert!(elapsed < Duration::from_millis(550), "...");

3 tâches de 200ms en parallèle avec workers=4 → ~200ms attendu. La marge de 550ms est généreuse mais peut échouer sur une machine CI très chargée. Acceptable pour un test d'intégration.

Tests

parallel_mode_disabled_runs_everything_sequentially — vérifie que .parallel() sur les tests est ignoré si le mode n'est pas activé.
parallel_mode_with_failfast_skips_later_parallel_tests — asserts souples (skipped >= 1) pour tenir compte du non-déterminisme.

Aucun bloquant. (Le bug timeout de #11 est ici déjà atténué — TestRunner::new() utilise with_defaults().)

## Review — parallel execution avec clients isolés (#12) ### Architecture : séquentiel d'abord, parallèle ensuite Tests non marqués `.parallel()` → phase séquentielle avec client partagé. Tests marqués → phase parallèle, chacun avec sa propre connexion. C'est la sémantique la plus sûre par défaut. ### `SharedTestFn = Arc<dyn Fn(...)>` Nécessaire pour cloner les hooks vers les tâches parallèles. Naming clair. La transformation `before_all.take().map(Arc::from)` est propre. ### `fail_fast_tripped` — AtomicBool partagé Le flag est vérifié avant de créer chaque tâche, pas pendant son exécution. Il y a donc une fenêtre TOCTOU : une tâche peut démarrer juste avant qu'une autre tâche la tripe, et toutes deux s'exécutent. C'est **inévitable** en parallèle sans synchronisation bloquante — comportement documenté acceptable. ### Ordre des résultats dans le rapport Les résultats parallèles sont collectés via `JoinSet::join_next()` en ordre de complétion (pas d'enregistrement). Le `report.tests` sera donc partiellement désordonné (séquentiel en ordre, puis parallèle en ordre d'achèvement). Pourrait surprendre des utilisateurs qui s'attendent à un rapport ordonné. Vaut la peine de le documenter. ### Test de timing fragile ```rust assert!(elapsed < Duration::from_millis(550), "..."); ``` 3 tâches de 200ms en parallèle avec workers=4 → ~200ms attendu. La marge de 550ms est généreuse mais peut échouer sur une machine CI très chargée. Acceptable pour un test d'intégration. ### Tests - `parallel_mode_disabled_runs_everything_sequentially` — vérifie que `.parallel()` sur les tests est ignoré si le mode n'est pas activé. - `parallel_mode_with_failfast_skips_later_parallel_tests` — asserts souples (`skipped >= 1`) pour tenir compte du non-déterminisme. Aucun bloquant. (Le bug timeout de #11 est ici déjà atténué — `TestRunner::new()` utilise `with_defaults()`.)

charles reviewed

2026-04-11 17:55:14 +00:00

charles left a comment

✅ Pas de bloquant — architecture séquentiel-d'abord correcte, isolation des clients propre. À documenter : résultats parallèles en ordre de complétion, pas d'enregistrement.