chore: Expand and optimize comprehensive test coverage - #1053
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds deterministic Rust crate-test batching and dedicated CI matrices, portable validation, Q2_0 and OpenMP support, control-plane and model-resolution changes, benchmark and sidecar tests, typed process termination, and shared Playwright fixtures and assertions. ChangesCI test batching
Portable validation
Quantization and native linking
Runtime behavior and test updates
UI E2E fixtures and assertions
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
255cd4e to
164eb52
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr_builds.yml:
- Around line 427-432: Add a least-privilege permissions block to the
rust_crate_tests job, granting only contents: read, matching the existing ci.yml
rust_crate_tests configuration. Keep the job’s current conditions, matrix
settings, runner, and timeout unchanged.
- Line 444: Update the checkout step in the rust_crate_tests job to set
persist-credentials: false, matching the equivalent checkout configuration in
ci.yml. Keep the existing actions/checkout@v5 action unchanged.
In `@crates/model-hf/src/store/local.rs`:
- Around line 1240-1248: Split the oversized Rust files by extracting separable
responsibilities into named modules under 1,000 lines: in
crates/model-hf/src/store/local.rs, move mmproj resolution logic and its tests
around find_mmproj_path into a semantic module; in
crates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rs, move
listener-lifecycle tests into a semantic mesh test module; and in
crates/mesh-llm-system/src/benchmark.rs, move benchmark subprocess test support
into a named module. Update module declarations and visibility as needed while
preserving behavior and keeping each extracted module’s tests with its
implementation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 324ec3f1-febb-4357-90a4-3d92e0c12592
📒 Files selected for processing (32)
.github/AGENTS.md.github/actions/compute-changes/action.yml.github/workflows/ci.yml.github/workflows/pr_builds.ymlJustfileci/ci.mdcrates/llama-quant-ffi/src/lib.rscrates/mesh-client/src/client/control_plane.rscrates/mesh-client/tests/control_plane_client.rscrates/mesh-llm-host-runtime/src/exact_test_wrappers.rscrates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rscrates/mesh-llm-host-runtime/src/network/nostr/model_packs.rscrates/mesh-llm-host-runtime/src/network/openai/response/probe.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-ui/e2e/a11y/home-a11y.spec.tscrates/mesh-llm-ui/e2e/configuration/schema-controls.spec.tscrates/mesh-llm-ui/e2e/fixtures/base.tscrates/mesh-llm-ui/e2e/perf/meshviz-200.spec.tscrates/mesh-llm-ui/e2e/plugins/web-ui-exemplar.live.spec.tscrates/mesh-llm-ui/e2e/smoke/chat-mobile.spec.tscrates/mesh-llm-ui/e2e/smoke/home.spec.tscrates/mesh-llm-ui/e2e/smoke/live-parity.spec.tscrates/mesh-llm-ui/e2e/smoke/topnav-responsive.spec.tscrates/mesh-llm-ui/index.htmlcrates/mesh-llm-ui/playwright.config.tscrates/model-hf/src/store/local.rscrates/skippy-quantize/src/types.rsscripts/affected-crates.shscripts/plan-test-batches.shscripts/test-portable.shtools/xtask/src/repo_consistency.rstools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (1)
- crates/mesh-llm-host-runtime/src/exact_test_wrappers.rs
164eb52 to
9ad8c02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/mesh-llm-host-runtime/src/mesh/tests/control_listener.rs (1)
141-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
tempfile::TempDirfor this test directoryThis cleanup only runs at the end of the test, so any
?or panic leaves the directory behind.tempfileis already available in this crate, and switching toTempDirwould make the cleanup automatic and let you drop the random-suffix bookkeeping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/mesh/tests/control_listener.rs` around lines 141 - 180, Update the stalled-handshake test setup to use tempfile::TempDir instead of a manually generated path and random suffix. Pass TempDir’s path to start_owner_control_test_server, remove the explicit remove_dir_all cleanup, and retain the existing test behavior while relying on TempDir’s automatic cleanup on errors or panics.crates/model-hf/src/store/local/mmproj.rs (1)
231-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify these tests on
tempfile::tempdir()for consistent cleanup.
mmproj_path_falls_back_to_single_sibling_sidecarandmmproj_path_ignores_ambiguous_sibling_sidecarsalready usetempfile::tempdir(), but these four still build timestamped dirs understd::env::temp_dir()with manualcreate_dir_all/remove_dir_all, which leaks artifacts on panic. Switching them totempdir()aligns with the PR's temp-path handling and guarantees cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/model-hf/src/store/local/mmproj.rs` around lines 231 - 333, Update the four tests shown—mmproj_path_prefers_bf16_generic_precision_variants, resolve_mmproj_path_prefers_explicit_override, mmproj_path_prefers_quant_matched_named_candidate, and mmproj_path_prefers_quant_matched_generic_sibling—to create their temporary directories with tempfile::tempdir(). Use the returned directory path for test files, remove manual create_dir_all and remove_dir_all calls, and retain the existing assertions and test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/llama-quant-ffi/build.rs`:
- Around line 346-354: Update the OpenMP library-name handling before the loop
to combine entries from both OpenMP_C_LIB_NAMES and OpenMP_CXX_LIB_NAMES instead
of using or_else. Preserve the existing empty-name and pthread filtering, then
pass every merged library to link_linux_lib_from_cache.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/mesh/tests/control_listener.rs`:
- Around line 141-180: Update the stalled-handshake test setup to use
tempfile::TempDir instead of a manually generated path and random suffix. Pass
TempDir’s path to start_owner_control_test_server, remove the explicit
remove_dir_all cleanup, and retain the existing test behavior while relying on
TempDir’s automatic cleanup on errors or panics.
In `@crates/model-hf/src/store/local/mmproj.rs`:
- Around line 231-333: Update the four tests
shown—mmproj_path_prefers_bf16_generic_precision_variants,
resolve_mmproj_path_prefers_explicit_override,
mmproj_path_prefers_quant_matched_named_candidate, and
mmproj_path_prefers_quant_matched_generic_sibling—to create their temporary
directories with tempfile::tempdir(). Use the returned directory path for test
files, remove manual create_dir_all and remove_dir_all calls, and retain the
existing assertions and test behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3479ce-8ffb-4d2d-b18b-da31741921c7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/AGENTS.md.github/actions/compute-changes/action.yml.github/workflows/ci.yml.github/workflows/pr_builds.ymlJustfileci/ci.mdcrates/llama-quant-ffi/build.rscrates/llama-quant-ffi/src/lib.rscrates/mesh-client/src/client/control_plane.rscrates/mesh-client/tests/control_plane_client.rscrates/mesh-llm-host-runtime/src/exact_test_wrappers.rscrates/mesh-llm-host-runtime/src/mesh/tests/control_listener.rscrates/mesh-llm-host-runtime/src/mesh/tests/mod.rscrates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rscrates/mesh-llm-host-runtime/src/network/nostr/model_packs.rscrates/mesh-llm-host-runtime/src/network/openai/response/probe.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-system/src/benchmark/tests.rscrates/mesh-llm-ui/e2e/a11y/home-a11y.spec.tscrates/mesh-llm-ui/e2e/configuration/schema-controls.spec.tscrates/mesh-llm-ui/e2e/fixtures/base.tscrates/mesh-llm-ui/e2e/perf/meshviz-200.spec.tscrates/mesh-llm-ui/e2e/plugins/web-ui-exemplar.live.spec.tscrates/mesh-llm-ui/e2e/smoke/chat-mobile.spec.tscrates/mesh-llm-ui/e2e/smoke/home.spec.tscrates/mesh-llm-ui/e2e/smoke/live-parity.spec.tscrates/mesh-llm-ui/e2e/smoke/topnav-responsive.spec.tscrates/mesh-llm-ui/index.htmlcrates/mesh-llm-ui/playwright.config.tscrates/model-hf/src/store/local.rscrates/model-hf/src/store/local/mmproj.rscrates/skippy-bench/Cargo.tomlcrates/skippy-bench/src/evals.rscrates/skippy-quantize/src/types.rsscripts/affected-crates.shscripts/plan-test-batches.shscripts/test-portable.shtools/xtask/src/repo_consistency.rstools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (2)
- crates/mesh-llm-host-runtime/src/exact_test_wrappers.rs
- crates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rs
🚧 Files skipped from review as they are similar to previous changes (25)
- crates/mesh-llm-ui/e2e/smoke/home.spec.ts
- crates/mesh-llm-ui/index.html
- .github/AGENTS.md
- crates/mesh-llm-ui/e2e/fixtures/base.ts
- crates/mesh-llm-host-runtime/src/network/openai/response/probe.rs
- scripts/affected-crates.sh
- crates/mesh-llm-ui/e2e/a11y/home-a11y.spec.ts
- crates/mesh-client/tests/control_plane_client.rs
- scripts/plan-test-batches.sh
- .github/workflows/ci.yml
- crates/mesh-llm-ui/e2e/configuration/schema-controls.spec.ts
- crates/mesh-llm-ui/e2e/smoke/topnav-responsive.spec.ts
- Justfile
- tools/xtask/src/repo_consistency.rs
- scripts/test-portable.sh
- crates/mesh-llm-ui/e2e/smoke/chat-mobile.spec.ts
- crates/mesh-client/src/client/control_plane.rs
- .github/workflows/pr_builds.yml
- ci/ci.md
- .github/actions/compute-changes/action.yml
- crates/mesh-llm-ui/e2e/perf/meshviz-200.spec.ts
- crates/mesh-llm-ui/e2e/smoke/live-parity.spec.ts
- crates/mesh-llm-ui/playwright.config.ts
- tools/xtask/src/workflow_checks.rs
- crates/mesh-llm-host-runtime/src/network/nostr/model_packs.rs
9ad8c02 to
c31ae01
Compare
c31ae01 to
1a1403c
Compare
1a1403c to
b9abdf1
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/mesh-llm-system/src/benchmark/tests.rs (1)
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnv override isn't panic-safe.
f()here runs assertions that panic on failure (e.g.,expect_errin the serial callers). On panic,remove_varnever executes andBENCHMARK_CHILD_ENVleaks into subsequent tests. A drop guard keeps cleanup robust even when a test fails.♻️ Optional: restore env via a drop guard
fn with_benchmark_child_override<T>(path: &Path, f: impl FnOnce() -> T) -> T { - // TODO: Audit that the environment access only happens in single-threaded code. - unsafe { std::env::set_var(BENCHMARK_CHILD_ENV, path) }; - let result = f(); - // TODO: Audit that the environment access only happens in single-threaded code. - unsafe { std::env::remove_var(BENCHMARK_CHILD_ENV) }; - result + struct Restore; + impl Drop for Restore { + fn drop(&mut self) { + // SAFETY: callers are #[serial]; single-threaded env access. + unsafe { std::env::remove_var(BENCHMARK_CHILD_ENV) }; + } + } + // SAFETY: callers are #[serial]; single-threaded env access. + unsafe { std::env::set_var(BENCHMARK_CHILD_ENV, path) }; + let _restore = Restore; + f() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-system/src/benchmark/tests.rs` around lines 52 - 59, Make with_benchmark_child_override panic-safe by introducing a drop guard that removes BENCHMARK_CHILD_ENV when unwound, while preserving the existing setup before invoking f(). Ensure cleanup occurs on both normal return and panic, and retain the current unsafe environment-access handling.crates/model-hf/src/store/local/mmproj.rs (1)
231-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tempfile::tempdir()for the remaining temp-dir tests too.These four tests build the directory via
std::env::temp_dir()+create_dir_alland only clean up on the success path — theremove_dir_allnever runs if an assertion panics, leaking the directory. The sibling tests in this same file already usetempfile::tempdir(), which cleans up onDropand avoids path collisions. Aligning them is a small, consistent hygiene win.♻️ Example for one test
- let temp = std::env::temp_dir().join(format!( - "mesh-llm-mmproj-precision-test-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - std::fs::create_dir_all(&temp).unwrap(); - let model = temp.join("Qwen3.5-0.8B-Q4_K_M.gguf"); + let temp = tempfile::tempdir().unwrap(); + let model = temp.path().join("Qwen3.5-0.8B-Q4_K_M.gguf");Then drop the trailing
let _ = std::fs::remove_dir_all(&temp);.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/model-hf/src/store/local/mmproj.rs` around lines 231 - 333, Update the four tests—mmproj_path_prefers_bf16_generic_precision_variants, resolve_mmproj_path_prefers_explicit_override, mmproj_path_prefers_quant_matched_named_candidate, and mmproj_path_prefers_quant_matched_generic_sibling—to create temporary directories with tempfile::tempdir(). Use the resulting directory path for test files and remove the trailing manual remove_dir_all cleanup calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-system/src/benchmark/tests.rs`:
- Line 322: Update the temporary path construction in the affected tests at the
fixed-name sites, including the fingerprint roundtrip test and the analogous
sites near the other referenced lines, to append a collision-resistant suffix
using std::process::id() and current nanoseconds, matching the existing pattern
in this file. Preserve each test’s filename purpose while ensuring concurrent
test-binary invocations use distinct paths.
In `@crates/mesh-llm-ui/e2e/smoke/topnav-responsive.spec.ts`:
- Around line 56-61: Update the polling predicate around readTopNavMetrics to
include the breakpoint-specific responsive control visibility expected after
setViewportSize, not just headerHeight, controlTopSpread, and
horizontalOverflow. Ensure polling completes only when the layout metrics and
visibility checks used later are simultaneously satisfied, so the subsequent
assertions in the top-nav smoke test are stable.
---
Nitpick comments:
In `@crates/mesh-llm-system/src/benchmark/tests.rs`:
- Around line 52-59: Make with_benchmark_child_override panic-safe by
introducing a drop guard that removes BENCHMARK_CHILD_ENV when unwound, while
preserving the existing setup before invoking f(). Ensure cleanup occurs on both
normal return and panic, and retain the current unsafe environment-access
handling.
In `@crates/model-hf/src/store/local/mmproj.rs`:
- Around line 231-333: Update the four
tests—mmproj_path_prefers_bf16_generic_precision_variants,
resolve_mmproj_path_prefers_explicit_override,
mmproj_path_prefers_quant_matched_named_candidate, and
mmproj_path_prefers_quant_matched_generic_sibling—to create temporary
directories with tempfile::tempdir(). Use the resulting directory path for test
files and remove the trailing manual remove_dir_all cleanup calls.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2b3a284-94b8-4c7d-9104-a8a2493b637d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/AGENTS.md.github/actions/compute-changes/action.yml.github/workflows/ci.yml.github/workflows/pr_builds.ymlJustfileci/ci.mdcrates/llama-quant-ffi/build.rscrates/llama-quant-ffi/src/lib.rscrates/mesh-client/src/client/control_plane.rscrates/mesh-client/tests/control_plane_client.rscrates/mesh-llm-host-runtime/src/exact_test_wrappers.rscrates/mesh-llm-host-runtime/src/mesh/tests/control_listener.rscrates/mesh-llm-host-runtime/src/mesh/tests/mod.rscrates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rscrates/mesh-llm-host-runtime/src/network/nostr/model_packs.rscrates/mesh-llm-host-runtime/src/network/openai/response/probe.rscrates/mesh-llm-system/src/benchmark.rscrates/mesh-llm-system/src/benchmark/tests.rscrates/mesh-llm-ui/e2e/a11y/home-a11y.spec.tscrates/mesh-llm-ui/e2e/configuration/schema-controls.spec.tscrates/mesh-llm-ui/e2e/fixtures/base.tscrates/mesh-llm-ui/e2e/perf/meshviz-200.spec.tscrates/mesh-llm-ui/e2e/plugins/web-ui-exemplar.live.spec.tscrates/mesh-llm-ui/e2e/smoke/chat-mobile.spec.tscrates/mesh-llm-ui/e2e/smoke/home.spec.tscrates/mesh-llm-ui/e2e/smoke/live-parity.spec.tscrates/mesh-llm-ui/e2e/smoke/topnav-responsive.spec.tscrates/mesh-llm-ui/index.htmlcrates/mesh-llm-ui/playwright.config.tscrates/model-hf/src/store/local.rscrates/model-hf/src/store/local/mmproj.rscrates/skippy-bench/Cargo.tomlcrates/skippy-bench/src/evals.rscrates/skippy-quantize/src/types.rsscripts/affected-crates.shscripts/plan-test-batches.shscripts/test-portable.shtools/xtask/src/repo_consistency.rstools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (2)
- crates/mesh-llm-host-runtime/src/exact_test_wrappers.rs
- crates/mesh-llm-host-runtime/src/mesh/tests/owner_control.rs
* origin/main: chore: Expand and optimize comprehensive test coverage (#1053) # Conflicts: # crates/llama-quant-ffi/build.rs
Summary
just test-allderive comprehensive Rust coverage from the workspace instead of a hand-maintained crate subsetWhy
The previous local and CI test paths both relied on curated crate lists. New workspace crates could therefore be added without ever entering either test path. Several tests also paid for real timeouts or external/cache state, and the browser suite was only partially exercising its intended coverage.
Results
just test-allThe overall run is 68.80s longer because it now executes about 70% more Rust tests. Despite that coverage expansion, measured Rust-test throughput improved by about 29%, and the two largest focused sections above were reduced by roughly half.
CI behavior
mainand manual runs: distribute all 56 workspace crates across the same four jobsValidation
just test-all— passedactionlint .github/workflows/*.ymlbash -n scripts/plan-test-batches.sh scripts/affected-crates.shcargo fmt --all --checkcargo check -p xtaskcargo clippy -p xtask --all-targets -- -D warningscargo test -p xtask— 4 passedcargo run -p xtask -- repo-consistency ci-crate-listscargo run -p xtask -- repo-consistency release-targetscargo run -p xtask -- repo-consistency publish-cratesgit diff --checkKotlin and Swift portable checks remain explicitly skipped on this machine because their required Java/XCFramework prerequisites are unavailable.
Summary by CodeRabbit
New Features
Q2_0quantization support across parsing/FFI mappings and mmproj sidecar selection.Bug Fixes
Chores / CI
rust_crate_testsmatrix, with updated coverage validation.