chore: test improvements - #12
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds VM error prefixes and validation, writes precompiled modules atomically, restricts precompile lookup to registered runners, checks both module variants, and consolidates development optimization profiles. ChangesVM error validation
Precompiled module safety
Development build settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PrecompileBuilder
participant Filesystem
participant load_modules
participant RunnerRegistry
PrecompileBuilder->>Filesystem: Write temporary precompiled artifacts
Filesystem-->>PrecompileBuilder: Rename artifacts into destination
load_modules->>RunnerRegistry: Verify runner ID and hash
RunnerRegistry-->>load_modules: Return authorized runner
load_modules->>Filesystem: Check both module variants
Filesystem-->>load_modules: Return available artifacts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
executor/tests/precompile_lookup.rs (1)
9-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a panic-safe temp directory for test cleanup.
custom_runner_is_not_in_the_registryonly removesrootat line 61, after allassert!calls succeed. If any assertion on lines 48-59 fails, the test panics and the temp directory understd::env::temp_dir()is never removed. Repeated test failures accumulate orphaned directories.Use
tempfile::TempDir, which removes its directory on drop regardless of panics, instead ofunique_dirplus a manualremove_dir_allcall.🤖 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 `@executor/tests/precompile_lookup.rs` around lines 9 - 62, Update registry_with_one_builtin and custom_runner_is_not_in_the_registry to use tempfile::TempDir for the temporary root, relying on its Drop cleanup instead of unique_dir and the manual remove_dir_all call. Preserve returning and using the TempDir path needed by runners::cache::Reader while ensuring cleanup occurs when assertions panic.
🤖 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 `@executor/crates/sdk-rs/src/abi/consts.rs`:
- Around line 377-392: The dynamic constructors for __VmError::WasmTrap and
__VmError::Host must align with VmError::is_valid_ by rejecting empty input or
updating validation to accept empty suffixes; preserve consistency between
val_str and validation, and add tests covering empty input for both
constructors.
---
Nitpick comments:
In `@executor/tests/precompile_lookup.rs`:
- Around line 9-62: Update registry_with_one_builtin and
custom_runner_is_not_in_the_registry to use tempfile::TempDir for the temporary
root, relying on its Drop cleanup instead of unique_dir and the manual
remove_dir_all call. Preserve returning and using the TempDir path needed by
runners::cache::Reader while ensuring cleanup occurs when assertions panic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 20b8aeeb-5b2a-4469-9c27-3f63764efcb0
📒 Files selected for processing (5)
executor/Cargo.tomlexecutor/crates/sdk-rs/src/abi/consts.rsexecutor/src/exe/precompile.rsexecutor/src/rt/supervisor/actions.rsexecutor/tests/precompile_lookup.rs
| if let Some(rest) = s.strip_prefix("exit_code ") { | ||
| if rest.parse::<i32>().is_ok_and(|v| v.to_string() == rest) { | ||
| return true; | ||
| } | ||
| } | ||
| if let Some(rest) = s.strip_prefix("wasm_trap ") { | ||
| if !rest.is_empty() { | ||
| return true; | ||
| } | ||
| } | ||
| if let Some(rest) = s.strip_prefix("host ") { | ||
| if !rest.is_empty() { | ||
| return true; | ||
| } | ||
| } | ||
| false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align dynamic constructors with VmError::is_valid_.
__VmError::WasmTrap::val_str("") and __VmError::Host::val_str("") create wasm_trap and host , but this validator rejects both values because the suffix is empty. Reject empty input in the constructors, or accept empty suffixes here. Add tests for both dynamic constructors.
🤖 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 `@executor/crates/sdk-rs/src/abi/consts.rs` around lines 377 - 392, The dynamic
constructors for __VmError::WasmTrap and __VmError::Host must align with
VmError::is_valid_ by rejecting empty input or updating validation to accept
empty suffixes; preserve consistency between val_str and validation, and add
tests covering empty input for both constructors.
* chore(executor): harden the precompile cache and speed up dev builds 🔒️⚡ * chore(sdk-rs): reject an empty description in vm_error constructors ✅
9064b28 to
1c8126d
Compare
1 participant
Auto-opened executor mirror of genlayerlabs/genvm-manager#17.
Carries the executor-side work for that manager PR. Auto-closed as merged when the manager PR lands (its
pr/v0.2/chore/test-improvementsbranch is moved ontov0.2-dev).Summary by CodeRabbit
Bug Fixes
Tests