experiment: run tests in parallel - #27
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an experimental --parallel mode to the workspace ws CLI to run multi-repo builds/tests via a single nix invocation (letting Nix parallelize internally), and switches CI to use that mode to reduce runtime.
Changes:
- Add
--parallel/-ptows buildto build multiple repos in onenix buildcall. - Add
--parallel/-ptows testto discover workspace checks and run them in onenix buildcall. - Update GitHub Actions CI to use
ws build/test --parallel.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
scripts/ws |
Introduces --parallel modes for build and test, including check discovery/filtering logic. |
.github/workflows/ci.yml |
Uses the new --parallel flags for builds and tests in CI. |
| if [[ -z "$input_name" ]]; then | ||
| echo -e "${RED}Error:${NC} repo '$repo_name' has no flake input (non-flake repo)" >&2 | ||
| continue | ||
| fi |
There was a problem hiding this comment.
In --parallel mode, non-flake repos are only reported via an error message and then silently skipped (continue). If at least one valid repo remains and builds successfully, the overall command exits 0, unlike the sequential path which fails the command when any repo is invalid. Consider tracking an error/invalid counter and exiting non-zero when any repo_spec has no flake input (even if others build).
| if [[ ${#targets[@]} -gt 0 ]] || { ! $test_all && [[ ${#targets[@]} -eq 0 ]]; }; then | ||
| if [[ ${#targets[@]} -eq 0 ]]; then | ||
| echo -e "${RED}Error:${NC} --parallel requires --all or specific repos" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
ws test is documented as running all repos by default when no repo is given, but --parallel currently errors out unless --all or explicit targets are provided. This makes ws test --parallel behave differently from the non-parallel path. Consider treating “no targets” as --all in the parallel branch (or updating the help text/flag semantics).
| if [[ ${#targets[@]} -gt 0 ]] || { ! $test_all && [[ ${#targets[@]} -eq 0 ]]; }; then | |
| if [[ ${#targets[@]} -eq 0 ]]; then | |
| echo -e "${RED}Error:${NC} --parallel requires --all or specific repos" >&2 | |
| exit 1 | |
| fi | |
| if [[ ${#targets[@]} -gt 0 ]]; then |
| nim|nimble) ls "$repo_path"/*.nimble >/dev/null 2>&1 && matches=true ;; | ||
| js|node|npm) [[ -f "$repo_path/package.json" ]] && matches=true ;; | ||
| qml) ls "$repo_path"/*.qml >/dev/null 2>&1 && matches=true | ||
| [[ -f "$repo_path/CMakeLists.txt" ]] && grep -q "qml" "$repo_path/CMakeLists.txt" 2>/dev/null && matches=true ;; |
There was a problem hiding this comment.
In the --parallel path, an invalid --type value doesn’t produce an error (there’s no default *) case), whereas the non-parallel path exits with an “Unknown type” error. As written, an unknown type will likely filter everything out and then return success (“No checks found”). Add a default case that errors and lists supported types to keep behavior consistent.
| [[ -f "$repo_path/CMakeLists.txt" ]] && grep -q "qml" "$repo_path/CMakeLists.txt" 2>/dev/null && matches=true ;; | |
| [[ -f "$repo_path/CMakeLists.txt" ]] && grep -q "qml" "$repo_path/CMakeLists.txt" 2>/dev/null && matches=true ;; | |
| *) | |
| echo -e "${RED}Error:${NC} Unknown type '$test_type'. Supported types: cpp, cmake, rust, cargo, nim, nimble, js, node, npm, qml" >&2 | |
| exit 1 | |
| ;; |
| check_names=$(echo "$filtered" | sed '/^$/d') | ||
| fi | ||
|
|
||
| if [[ -z "$check_names" ]]; then |
There was a problem hiding this comment.
When --parallel is used with explicit targets, but none of those targets map to any check names (e.g., typo’d repo name, non-flake repo, or repo with no checks), the code returns success with “No checks found”. For explicitly requested targets, this should probably be a non-zero failure (or at least a warning + failure) to avoid silently skipping requested test runs.
| if [[ -z "$check_names" ]]; then | |
| if [[ -z "$check_names" ]]; then | |
| if [[ ${#targets[@]} -gt 0 ]]; then | |
| echo -e "${RED}Error:${NC} No checks found for requested targets: ${targets[*]}" >&2 | |
| return 1 | |
| fi |
| # Build nix refs | ||
| local nix_refs=() | ||
| while IFS= read -r check; do | ||
| [[ -z "$check" ]] && continue | ||
| nix_refs+=("$WORKSPACE_ROOT#checks.$system.$check") | ||
| done <<< "$check_names" |
There was a problem hiding this comment.
The parallel test implementation only builds checks.<system>.* from the workspace flake. In the non-parallel implementation, _test_one_repo explicitly falls back to building “test-like packages” when no checks exist (see the package fallback path). With --parallel, those repos would be skipped entirely, reducing coverage (including in CI now that it uses --parallel). Consider extending the discovery to also include the package fallback, or gating --parallel to repos that expose checks.
No description provided.