feat(iii-worker): add cross-platform integration test suite and CI#1449
Merged
andersonleal merged 2 commits intomainfrom Apr 10, 2026
Merged
feat(iii-worker): add cross-platform integration test suite and CI#1449andersonleal merged 2 commits intomainfrom
andersonleal merged 2 commits intomainfrom
Conversation
Add comprehensive integration test suite for iii-worker covering all worker runtime features (local, binary, OCI, config, lifecycle) with cross-platform parity on Linux and macOS. Test infrastructure: - Shared tests/common/ module with fixtures, CWD isolation, path helpers - Feature gates (integration-vm, integration-oci) for heavy tests - Refactored existing tests to use shared helpers Integration test coverage: - Local worker lifecycle (add/start/stop, path detection, workspace mgmt) - Binary worker (download, arch detection, permissions, checksum) - OCI worker (layer extraction, safety limits, multi-arch selection) - Worker manager (RuntimeAdapter dispatch, state serialization) - Project auto-detection (language, package manager, scripts) - Firmware resolution chain (local, embedded, download, error) - VM boot args and configuration (ungated + feature-gated) CI configuration: - worker-test-matrix: cargo-nextest on ubuntu-latest + macos-latest (every PR) - worker-test-vm-macos: VM feature-gated tests on macos-latest (every PR) - worker-test-oci-macos: OCI feature-gated tests on macos-latest (every PR) - worker-test-vm-linux: VM tests on self-hosted KVM runner (manual) - worker-test-oci-linux: OCI tests on self-hosted runner (manual) - JUnit XML reporting via cargo-nextest CI profile
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a nextest CI profile and GitHub Actions jobs, introduces two test-only Cargo features, exposes several internal helper functions as public, and adds many new focused integration tests and shared test utilities while removing one legacy monolithic test file. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
crates/iii-worker/tests/common/isolation.rs (1)
13-34: Consider panic safety for CWD restoration.If the closure panics,
set_current_dir(original)won't execute, leaving the process in the temp directory. For test utilities this is usually acceptable since a panic means test failure, but you could use a drop guard for robustness:♻️ Optional: Use a drop guard for panic safety
+struct CwdGuard { + original: std::path::PathBuf, +} + +impl Drop for CwdGuard { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.original); + } +} + pub async fn in_temp_dir_async<F, Fut>(f: F) where F: FnOnce() -> Fut, Fut: std::future::Future<Output = ()>, { let _guard = CWD_LOCK.lock().unwrap(); let dir = tempfile::tempdir().unwrap(); let original = std::env::current_dir().unwrap(); + let _cwd_guard = CwdGuard { original: original.clone() }; std::env::set_current_dir(dir.path()).unwrap(); f().await; - std::env::set_current_dir(original).unwrap(); + // CwdGuard restores on drop (including panic unwind) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/common/isolation.rs` around lines 13 - 34, The current in_temp_dir and in_temp_dir_async change the cwd but restore it only after the closure returns, so on panic the cwd may not be restored; create a small RAII guard type (e.g., RestoreCwd) that stores the original PathBuf and implements Drop to call std::env::set_current_dir(original) (handle/propagate errors as appropriate), then in both in_temp_dir and in_temp_dir_async set the cwd to the tempdir and instantiate the guard (let _restore = RestoreCwd(original)) before invoking f so the Drop impl always runs even if f panics; keep the guard alive across await in in_temp_dir_async by not moving it out of scope until after f().await.crates/iii-worker/tests/worker_manager_integration.rs (1)
90-97: This smoke test is tautological.
assert!(available || !available)can never fail, so it doesn't protect against regressions. If the goal is just “does not panic”, calling the function is enough; otherwise assert a real host-independent invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/worker_manager_integration.rs` around lines 90 - 97, The test currently uses a tautological assertion assert!(available || !available) which can never fail; remove that assertion and make the test only call iii_worker::cli::worker_manager::libkrun::libkrun_available() (e.g., let _ = libkrun_available();) to serve as a true “does not panic” smoke test, or replace it with a meaningful, host-independent invariant if one exists; also update the test name/comment to reflect that it only checks for no panic if you choose the removal approach.crates/iii-worker/tests/vm_args_integration.rs (1)
622-633: This assertion never exercises behavior.
result.is_none() || result.is_some()is always true. If this is only a visibility smoke test, calling the function is enough; otherwise keep thepath.exists()assertion for theSomebranch and drop the tautology.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_args_integration.rs` around lines 622 - 633, The assertion result.is_none() || result.is_some() is a tautology; replace it with a meaningful check: either make this a pure visibility smoke test by removing the assert and simply calling resolve_krunfw_file_path() (e.g., let _ = resolve_krunfw_file_path();) or, if you want to validate the Some branch, match on resolve_krunfw_file_path() and assert that the returned path exists (e.g., match result { Some(p) => assert!(p.exists(), "krunfw path must exist"), None => () }); use the function name resolve_krunfw_file_path to locate the test and apply one of these fixes.crates/iii-worker/tests/vm_integration.rs (1)
200-239: This test can drift from the implementation it claims to cover.
env_merger_image_env_overridden_by_spec_envreimplements the merge locally instead of exercisingLibkrunAdapter::start()’s merge path (or a shared helper). If the production merge semantics change, this test can stay green while the real behavior regresses. Extract the merge into a reusable function and assert against that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_integration.rs` around lines 200 - 239, The test env_merger_image_env_overridden_by_spec_env reimplements the env merge instead of calling the production merge used by LibkrunAdapter::start(), which can drift; extract the merge logic into a single reusable function (e.g., fn merge_envs(image: &HashMap<String,String>, spec: &HashMap<String,String>) -> HashMap<String,String>) and call that from LibkrunAdapter::start() (replace the in-place loop there with merge_envs) and from this test so the test asserts the real merge behavior; update the test to build image_env/spec_env and call merge_envs(...) and assert on the returned map rather than duplicating the merge loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/iii-worker/tests/config_clear_integration.rs`:
- Around line 15-43: The tests call
iii_worker::cli::managed::handle_managed_clear while only sandboxing CWD via
in_temp_dir_async, but handle_managed_clear uses dirs::home_dir() so the tests
can mutate the real ~/.iii; update the test helper (in_temp_dir_async) or each
test to also set the process HOME to the temp dir (e.g.,
std::env::set_var("HOME", temp_dir.path())) before invoking handle_managed_clear
and restore afterwards, then assert the expected files under that sandboxed home
(~/.iii/workers and ~/.iii/images) rather than the real home; modify the three
tests (handle_managed_clear_single_no_artifacts,
handle_managed_clear_invalid_name, handle_managed_clear_all_no_artifacts) to use
the updated helper or to set HOME inline so all operations are confined to the
temp directory.
In `@crates/iii-worker/tests/firmware_integration.rs`:
- Around line 47-61: The test sets HOME and removes III_LIBKRUNFW_PATH but only
restores them when original_home is Some, which leaks env changes when they were
originally unset; update the cleanup to handle the None branch by calling
std::env::remove_var("HOME") and std::env::remove_var("III_LIBKRUNFW_PATH") when
original values are None (or replace the pattern with a small RAII EnvGuard that
restores or removes the vars in Drop), targeting the restore blocks around the
call to resolve_libkrunfw_dir() (and replicate the fix in the other similar
blocks at lines ~78-90, ~141-158, ~259-276, ~297-314).
In `@crates/iii-worker/tests/oci_worker_integration.rs`:
- Around line 202-215: The test expected_oci_arch_returns_known_value rejects
valid outputs on other architectures; update the test (function
expected_oci_arch_returns_known_value) to call expected_oci_arch() and then: for
known targets (use cfg!(target_arch = "aarch64") and cfg!(target_arch =
"x86_64")) assert exact expected values ("arm64" and "amd64" respectively), and
for all other targets assert only that the returned string is non-empty (e.g.,
not ""), so the test accepts std::env::consts::ARCH fallbacks while still
validating known mappings.
- Around line 389-396: Replace the placeholder type-check test with a real
no-network smoke test that calls pull_and_extract_rootfs so its early validation
runs: invoke iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs with
a deliberately invalid or malformed image reference (e.g., an empty string or
clearly invalid ref) and assert it returns the expected validation error
(instead of attempting network access); this ensures pull_and_extract_rootfs's
input validation path is exercised without needing a registry.
In `@crates/iii-worker/tests/vm_args_integration.rs`:
- Around line 428-430: The test uses nix::libc::kill(pid, 0) which can falsely
succeed for unreaped zombies; instead keep the Child handle (remove the
std::mem::forget(child) call), do not drop it, and after calling adapter.stop()
call child.try_wait() and assert it returns Ok(Some(status)) to ensure the
process was actually reaped; update the assertions around the kill(pid, 0)
checks to use child.try_wait() and any related variables (child, adapter.stop,
pid) accordingly.
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 406-413: Remove the backend-specific VM error text checks in the
is_known_error block inside the vm_integration test (the code referencing
stderr_str and is_known_error) and replace that assertion with a simple check
that stderr is non-empty (e.g., assert!(!stderr_str.trim().is_empty(), "stderr
should contain VM error output. Actual stderr: {}", stderr_str)); this avoids
pinning to libkrun/host/runtime wording while still ensuring the VM produced
error output (the --rootfs propagation check using
stderr.contains(&temp_dir_path) can remain as-is).
In `@crates/iii-worker/tests/worker_manager_integration.rs`:
- Around line 80-88: The test relies on create_adapter("unknown") silently
returning a LibkrunAdapter, which hides configuration errors; fix by making
create_adapter validate the runtime string instead of always constructing
LibkrunAdapter: change create_adapter to return a Result<Arc<dyn
RuntimeAdapter>, Error> (or Option) and return Err for unsupported/misspelled
runtimes, update callers/tests to handle the Result and replace this test to
expect an error (or, if you must preserve current behavior temporarily, rename
the test to indicate it's a regression/compatibility test and add #[ignore]
until the API is tightened); key symbols to modify are create_adapter and the
place that instantiates LibkrunAdapter.
---
Nitpick comments:
In `@crates/iii-worker/tests/common/isolation.rs`:
- Around line 13-34: The current in_temp_dir and in_temp_dir_async change the
cwd but restore it only after the closure returns, so on panic the cwd may not
be restored; create a small RAII guard type (e.g., RestoreCwd) that stores the
original PathBuf and implements Drop to call std::env::set_current_dir(original)
(handle/propagate errors as appropriate), then in both in_temp_dir and
in_temp_dir_async set the cwd to the tempdir and instantiate the guard (let
_restore = RestoreCwd(original)) before invoking f so the Drop impl always runs
even if f panics; keep the guard alive across await in in_temp_dir_async by not
moving it out of scope until after f().await.
In `@crates/iii-worker/tests/vm_args_integration.rs`:
- Around line 622-633: The assertion result.is_none() || result.is_some() is a
tautology; replace it with a meaningful check: either make this a pure
visibility smoke test by removing the assert and simply calling
resolve_krunfw_file_path() (e.g., let _ = resolve_krunfw_file_path();) or, if
you want to validate the Some branch, match on resolve_krunfw_file_path() and
assert that the returned path exists (e.g., match result { Some(p) =>
assert!(p.exists(), "krunfw path must exist"), None => () }); use the function
name resolve_krunfw_file_path to locate the test and apply one of these fixes.
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 200-239: The test env_merger_image_env_overridden_by_spec_env
reimplements the env merge instead of calling the production merge used by
LibkrunAdapter::start(), which can drift; extract the merge logic into a single
reusable function (e.g., fn merge_envs(image: &HashMap<String,String>, spec:
&HashMap<String,String>) -> HashMap<String,String>) and call that from
LibkrunAdapter::start() (replace the in-place loop there with merge_envs) and
from this test so the test asserts the real merge behavior; update the test to
build image_env/spec_env and call merge_envs(...) and assert on the returned map
rather than duplicating the merge loop.
In `@crates/iii-worker/tests/worker_manager_integration.rs`:
- Around line 90-97: The test currently uses a tautological assertion
assert!(available || !available) which can never fail; remove that assertion and
make the test only call
iii_worker::cli::worker_manager::libkrun::libkrun_available() (e.g., let _ =
libkrun_available();) to serve as a true “does not panic” smoke test, or replace
it with a meaningful, host-independent invariant if one exists; also update the
test name/comment to reflect that it only checks for no panic if you choose the
removal approach.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dac5e70f-8183-4b57-be49-03cd7c422f18
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.config/nextest.toml.github/workflows/ci.ymlcrates/iii-worker/Cargo.tomlcrates/iii-worker/src/cli/binary_download.rscrates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/vm_boot.rscrates/iii-worker/src/cli/worker_manager/libkrun.rscrates/iii-worker/tests/binary_worker_integration.rscrates/iii-worker/tests/common/assertions.rscrates/iii-worker/tests/common/fixtures.rscrates/iii-worker/tests/common/isolation.rscrates/iii-worker/tests/common/mod.rscrates/iii-worker/tests/common_fixtures_integration.rscrates/iii-worker/tests/config_clear_integration.rscrates/iii-worker/tests/config_crossplatform_integration.rscrates/iii-worker/tests/config_crud_integration.rscrates/iii-worker/tests/config_file_integration.rscrates/iii-worker/tests/config_force_integration.rscrates/iii-worker/tests/config_managed_integration.rscrates/iii-worker/tests/config_path_type_integration.rscrates/iii-worker/tests/firmware_integration.rscrates/iii-worker/tests/local_worker_integration.rscrates/iii-worker/tests/oci_gate_smoke.rscrates/iii-worker/tests/oci_worker_integration.rscrates/iii-worker/tests/project_detection_integration.rscrates/iii-worker/tests/vm_args_integration.rscrates/iii-worker/tests/vm_gate_smoke.rscrates/iii-worker/tests/vm_integration.rscrates/iii-worker/tests/worker_integration.rscrates/iii-worker/tests/worker_manager_integration.rs
💤 Files with no reviewable changes (1)
- crates/iii-worker/tests/config_file_integration.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
crates/iii-worker/tests/oci_worker_integration.rs (2)
390-397:⚠️ Potential issue | 🟠 MajorReplace placeholder gated test with a real no-network smoke test.
Line 391-397 currently only type-checks an import, so the
integration-ociCI path can pass without executing target logic.Suggested fix
#[tokio::test] async fn pull_and_extract_rootfs_placeholder() { - // This test requires network access and a running OCI registry. - // It is intentionally feature-gated behind integration-oci. - // Full implementation depends on registry availability. - // For now, verify the import compiles under the feature gate. use iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs; - let _ = pull_and_extract_rootfs; // type-check only + + let dir = tempfile::tempdir().unwrap(); + let err = pull_and_extract_rootfs("not a valid image reference", dir.path()) + .await + .expect_err("invalid reference should fail fast before network calls"); + assert!(err.to_string().to_lowercase().contains("invalid")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/oci_worker_integration.rs` around lines 390 - 397, The test pull_and_extract_rootfs_placeholder currently only type-checks the import; replace it with a real no-network smoke test that exercises pull_and_extract_rootfs: create a temporary directory (e.g., via tempfile::tempdir), write a minimal OCI image layout or a small tarball representing a rootfs into that temp location, call iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs pointing at the local artifact (or a file:// URL) and assert the Result is Ok and the expected files/directories were extracted; keep the test gated behind integration-oci and use tokio::test async runtime, referencing the existing pull_and_extract_rootfs function and the test name pull_and_extract_rootfs_placeholder when replacing the stub.
202-215:⚠️ Potential issue | 🟡 MinorFix non-portable architecture assertion.
Line 205 currently rejects valid fallback architectures from
expected_oci_arch()on non-x86_64/aarch64targets.Suggested fix
#[test] fn expected_oci_arch_returns_known_value() { let arch = expected_oci_arch(); - assert!( - arch == "arm64" || arch == "amd64", - "expected arm64 or amd64, got: {}", - arch - ); - - if cfg!(target_arch = "aarch64") { + if cfg!(target_arch = "aarch64") { assert_eq!(arch, "arm64"); - } - if cfg!(target_arch = "x86_64") { + } else if cfg!(target_arch = "x86_64") { assert_eq!(arch, "amd64"); + } else { + assert!(!arch.is_empty(), "expected non-empty arch fallback, got empty"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/oci_worker_integration.rs` around lines 202 - 215, The test expected_oci_arch_returns_known_value is non-portable because it unconditionally asserts the returned arch is "arm64" or "amd64"; update the test to only enforce those specific expectations for the corresponding compile-time targets and relax the check for others: keep the cfg!(target_arch = "aarch64") { assert_eq!(arch, "arm64"); } and cfg!(target_arch = "x86_64") { assert_eq!(arch, "amd64"); } branches, but remove or replace the initial assert!(arch == "arm64" || arch == "amd64", ...) with a portable check (e.g., assert!(!arch.is_empty()) or validate a small allowed fallback set) so expected_oci_arch() can return valid platform-specific fallbacks on other architectures; locate this change in the test function expected_oci_arch_returns_known_value and adjust the assertions accordingly.crates/iii-worker/tests/worker_manager_integration.rs (1)
80-88:⚠️ Potential issue | 🟠 MajorThis test currently locks in invalid-runtime fallback as acceptable behavior.
Line 84 asserts that
"unknown"still yields a usable adapter, which bakes in a silent misconfiguration path instead of surfacing an unsupported runtime error. Please treat this as a temporary compatibility test at most, not correctness behavior.Suggested adjustment to avoid codifying fallback as correctness
-/// create_adapter with an unknown runtime string still returns a valid Arc. -/// Current implementation always returns LibkrunAdapter regardless of input. +/// Compatibility regression test: unknown runtime currently falls back to LibkrunAdapter. +/// TODO: replace with an error-path test once create_adapter validates runtime values. #[tokio::test] -async fn create_adapter_with_unknown_runtime() { +async fn create_adapter_unknown_runtime_current_behavior() { let adapter: Arc<dyn RuntimeAdapter> = create_adapter("unknown"); // Must not panic -- the adapter is valid even for unknown runtimes. let result = adapter.status("nonexistent").await; let _ = result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/worker_manager_integration.rs` around lines 80 - 88, The test create_adapter_with_unknown_runtime currently treats create_adapter("unknown") returning a usable Arc as acceptable; instead mark this as a temporary compatibility test so we don't codify fallback-as-correctness: rename the test to compatibility_create_adapter_with_unknown_runtime (or add #[ignore]) and add a comment that it's a temporary compatibility check, or better, change the assertion to validate behavior only when the API supports it (e.g., if create_adapter is changed to return Result then assert Err for unknown runtimes). Ensure you modify the test function name and annotate with #[ignore] or update expectations around create_adapter to avoid baking the fallback behavior as correctness.crates/iii-worker/tests/vm_integration.rs (1)
397-404:⚠️ Potential issue | 🟡 MinorAvoid pinning this test to backend-specific VM error text.
The rootfs-path assertion already proves
--rootfswas propagated. Matching specific phrases like"No init binary available"or"PassthroughFs failed"is likely to be flaky across libkrun/host/runtime combinations.Suggested simplification
- // Verify the error message is from the expected validation path - let is_known_error = stderr_str.contains("No init binary available") - || stderr_str.contains("VM execution failed") - || stderr_str.contains("PassthroughFs failed"); - assert!( - is_known_error, - "stderr should contain a known VM boot error message.\nActual stderr: {}", - stderr_str - ); + assert!( + !stderr_str.trim().is_empty(), + "stderr should contain VM error output.\nActual stderr: {}", + stderr_str + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_integration.rs` around lines 397 - 404, The test currently pins to backend-specific VM error phrases by building is_known_error from stderr_str.contains("No init binary available") || ...; remove those specific checks and instead assert a generic expectation (e.g., that stderr_str is non-empty or contains the word "error") so the test is not flaky across runtimes: replace the is_known_error logic with a simple check on stderr_str (for example assert!(!stderr_str.is_empty(), "stderr should contain a VM boot error message. Actual stderr: {}", stderr_str) or assert!(stderr_str.to_lowercase().contains("error"), ...)), and drop the string literals "No init binary available", "VM execution failed", and "PassthroughFs failed" references. Ensure you update the assertion that currently references is_known_error and keep the variable name stderr_str to locate the change.crates/iii-worker/tests/firmware_integration.rs (1)
49-63:⚠️ Potential issue | 🟠 MajorRestore-or-remove every env var in cleanup.
These blocks still leave process-global state behind:
HOMEis only restored when it was originally set, and the first two tests never restore the removedIII_LIBKRUNFW_PATH/III_INIT_PATHat all. That can leak temp paths into later tests after the tempdir is dropped. Please switch each cleanup to a full restore-or-remove path, or wrap the pattern in a small RAII env guard.Also applies to: 83-95, 152-169, 279-296, 317-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/firmware_integration.rs` around lines 49 - 63, The cleanup currently restores HOME only when it existed and never restores/removes III_LIBKRUNFW_PATH / III_INIT_PATH, leaking temp paths; change each test cleanup (the unsafe blocks around std::env::set_var/remove_var in firmware_integration.rs that surround calls like resolve_libkrunfw_dir) to perform full restore-or-remove semantics: capture original Option<String> for each env var (HOME, III_LIBKRUNFW_PATH, III_INIT_PATH), and in the cleanup either set_var(original) if Some or remove_var if None; alternatively implement and use a small RAII helper (e.g., EnvGuard) that records originals and on drop restores or removes each variable to guarantee no process-global state is left behind across tests.
🧹 Nitpick comments (3)
crates/iii-worker/tests/project_detection_integration.rs (1)
28-160: Optional: extract small file-fixture helpers to reduce repetition.There’s repeated
tempdir + write marker files + loadsetup across many tests. A tiny helper (e.g.,mk_project(&[("package.json","{}"), ...])) would keep tests shorter and easier to extend.Also applies to: 250-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/project_detection_integration.rs` around lines 28 - 160, Tests repeat tempdir creation and file writes (e.g., in auto_detect_typescript_npm_from_package_json, auto_detect_typescript_bun_from_bun_lock, auto_detect_rust_from_cargo_toml, package_manager_detection_bun_vs_npm_default); extract a small helper like mk_project(files: &[(&str,&str)]) -> tempfile::TempDir (or PathBuf) that creates a tempdir, writes each filename/content pair, and returns the dir so tests call mk_project(&[("package.json","{}")]) etc. Update each test to use mk_project and remove the duplicated tempfile::tempdir()/std::fs::write code; keep assertions and calls to auto_detect_project unchanged.crates/iii-worker/tests/binary_worker_integration.rs (1)
353-356: Prefer separator-agnostic path assertions.Using
"workers/image-resize"hardcodes/. PreferPathcomposition for portability and clearer intent.Suggested assertion
+ let expected_suffix = std::path::Path::new("workers").join("image-resize"); assert!( - result.ends_with("workers/image-resize"), - "path should end with 'workers/image-resize', got: {}", + result.ends_with(&expected_suffix), + "path should end with '{}', got: {}", + expected_suffix.display(), result.display() );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/binary_worker_integration.rs` around lines 353 - 356, The assertion hardcodes a forward slash; replace the string literal with a separator-agnostic Path composed via Path::new("workers").join("image-resize") and assert that result.ends_with(&expected_path) (use a Path or PathBuf named e.g. expected_path) so the test works on Windows and Unix; update the assertion message to display result.display() as before if desired.crates/iii-worker/tests/firmware_integration.rs (1)
113-115: Avoidto_str().unwrap()for the override path.This makes the test unnecessarily brittle on Unix, where a valid temp path may be non-UTF-8.
std::env::set_varacceptsOsStr, so passfile_path.as_os_str()directly instead.Suggested change
unsafe { - std::env::set_var("III_LIBKRUNFW_PATH", file_path.to_str().unwrap()); + std::env::set_var("III_LIBKRUNFW_PATH", file_path.as_os_str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/firmware_integration.rs` around lines 113 - 115, The test currently calls std::env::set_var("III_LIBKRUNFW_PATH", file_path.to_str().unwrap()) which can panic or fail for non-UTF-8 temp paths; change the override to pass an OsStr directly (use file_path.as_os_str()) instead of to_str().unwrap(), and keep the surrounding unsafe block and original_env handling (the variables to update are file_path and the set_var call in the test where original_env is captured).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/iii-worker/tests/binary_worker_integration.rs`:
- Around line 298-311: The HOME env cleanup in the test around
binary_workers_dir is unsafe and leaves HOME mutated if it was originally unset
or if the test panics; replace the manual unsafe set/restore with an RAII guard:
call std::env::var_os("HOME") to capture the original Option<OsString>, set HOME
to tmp.path() with std::env::set_var, and create a small struct (e.g.,
HomeEnvGuard) that stores the original Option and implements Drop to restore by
calling std::env::set_var when Some or std::env::remove_var when None; use this
guard in the test so restoration runs even on panic, and apply the same change
to the other test block that mirrors this logic (the block around lines
338-351).
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 191-230: The test reimplements the env-merge logic locally instead
of exercising the real code path in LibkrunAdapter::start(), so it won't catch
regressions in precedence or normalization; update the test to either (A) invoke
the production merge path by running the same code used by
LibkrunAdapter::start() (call into the adapter start path or helper it uses) and
assert results, or (B) refactor the merge implementation into a single shared
function (e.g., merge_envs or the existing helper used by start()) and call that
function from both LibkrunAdapter::start() and this test, then assert that PATH
is overridden, LANG preserved and III_URL added and the length is 3.
---
Duplicate comments:
In `@crates/iii-worker/tests/firmware_integration.rs`:
- Around line 49-63: The cleanup currently restores HOME only when it existed
and never restores/removes III_LIBKRUNFW_PATH / III_INIT_PATH, leaking temp
paths; change each test cleanup (the unsafe blocks around
std::env::set_var/remove_var in firmware_integration.rs that surround calls like
resolve_libkrunfw_dir) to perform full restore-or-remove semantics: capture
original Option<String> for each env var (HOME, III_LIBKRUNFW_PATH,
III_INIT_PATH), and in the cleanup either set_var(original) if Some or
remove_var if None; alternatively implement and use a small RAII helper (e.g.,
EnvGuard) that records originals and on drop restores or removes each variable
to guarantee no process-global state is left behind across tests.
In `@crates/iii-worker/tests/oci_worker_integration.rs`:
- Around line 390-397: The test pull_and_extract_rootfs_placeholder currently
only type-checks the import; replace it with a real no-network smoke test that
exercises pull_and_extract_rootfs: create a temporary directory (e.g., via
tempfile::tempdir), write a minimal OCI image layout or a small tarball
representing a rootfs into that temp location, call
iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs pointing at the
local artifact (or a file:// URL) and assert the Result is Ok and the expected
files/directories were extracted; keep the test gated behind integration-oci and
use tokio::test async runtime, referencing the existing pull_and_extract_rootfs
function and the test name pull_and_extract_rootfs_placeholder when replacing
the stub.
- Around line 202-215: The test expected_oci_arch_returns_known_value is
non-portable because it unconditionally asserts the returned arch is "arm64" or
"amd64"; update the test to only enforce those specific expectations for the
corresponding compile-time targets and relax the check for others: keep the
cfg!(target_arch = "aarch64") { assert_eq!(arch, "arm64"); } and
cfg!(target_arch = "x86_64") { assert_eq!(arch, "amd64"); } branches, but remove
or replace the initial assert!(arch == "arm64" || arch == "amd64", ...) with a
portable check (e.g., assert!(!arch.is_empty()) or validate a small allowed
fallback set) so expected_oci_arch() can return valid platform-specific
fallbacks on other architectures; locate this change in the test function
expected_oci_arch_returns_known_value and adjust the assertions accordingly.
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 397-404: The test currently pins to backend-specific VM error
phrases by building is_known_error from stderr_str.contains("No init binary
available") || ...; remove those specific checks and instead assert a generic
expectation (e.g., that stderr_str is non-empty or contains the word "error") so
the test is not flaky across runtimes: replace the is_known_error logic with a
simple check on stderr_str (for example assert!(!stderr_str.is_empty(), "stderr
should contain a VM boot error message. Actual stderr: {}", stderr_str) or
assert!(stderr_str.to_lowercase().contains("error"), ...)), and drop the string
literals "No init binary available", "VM execution failed", and "PassthroughFs
failed" references. Ensure you update the assertion that currently references
is_known_error and keep the variable name stderr_str to locate the change.
In `@crates/iii-worker/tests/worker_manager_integration.rs`:
- Around line 80-88: The test create_adapter_with_unknown_runtime currently
treats create_adapter("unknown") returning a usable Arc as acceptable; instead
mark this as a temporary compatibility test so we don't codify
fallback-as-correctness: rename the test to
compatibility_create_adapter_with_unknown_runtime (or add #[ignore]) and add a
comment that it's a temporary compatibility check, or better, change the
assertion to validate behavior only when the API supports it (e.g., if
create_adapter is changed to return Result then assert Err for unknown
runtimes). Ensure you modify the test function name and annotate with #[ignore]
or update expectations around create_adapter to avoid baking the fallback
behavior as correctness.
---
Nitpick comments:
In `@crates/iii-worker/tests/binary_worker_integration.rs`:
- Around line 353-356: The assertion hardcodes a forward slash; replace the
string literal with a separator-agnostic Path composed via
Path::new("workers").join("image-resize") and assert that
result.ends_with(&expected_path) (use a Path or PathBuf named e.g.
expected_path) so the test works on Windows and Unix; update the assertion
message to display result.display() as before if desired.
In `@crates/iii-worker/tests/firmware_integration.rs`:
- Around line 113-115: The test currently calls
std::env::set_var("III_LIBKRUNFW_PATH", file_path.to_str().unwrap()) which can
panic or fail for non-UTF-8 temp paths; change the override to pass an OsStr
directly (use file_path.as_os_str()) instead of to_str().unwrap(), and keep the
surrounding unsafe block and original_env handling (the variables to update are
file_path and the set_var call in the test where original_env is captured).
In `@crates/iii-worker/tests/project_detection_integration.rs`:
- Around line 28-160: Tests repeat tempdir creation and file writes (e.g., in
auto_detect_typescript_npm_from_package_json,
auto_detect_typescript_bun_from_bun_lock, auto_detect_rust_from_cargo_toml,
package_manager_detection_bun_vs_npm_default); extract a small helper like
mk_project(files: &[(&str,&str)]) -> tempfile::TempDir (or PathBuf) that creates
a tempdir, writes each filename/content pair, and returns the dir so tests call
mk_project(&[("package.json","{}")]) etc. Update each test to use mk_project and
remove the duplicated tempfile::tempdir()/std::fs::write code; keep assertions
and calls to auto_detect_project unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a27200e1-566b-4f1d-b151-ee22de6c7e8b
📒 Files selected for processing (10)
crates/iii-worker/src/cli/binary_download.rscrates/iii-worker/src/cli/vm_boot.rscrates/iii-worker/tests/binary_worker_integration.rscrates/iii-worker/tests/firmware_integration.rscrates/iii-worker/tests/local_worker_integration.rscrates/iii-worker/tests/oci_worker_integration.rscrates/iii-worker/tests/project_detection_integration.rscrates/iii-worker/tests/vm_args_integration.rscrates/iii-worker/tests/vm_integration.rscrates/iii-worker/tests/worker_manager_integration.rs
✅ Files skipped from review due to trivial changes (2)
- crates/iii-worker/src/cli/binary_download.rs
- crates/iii-worker/tests/vm_args_integration.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/iii-worker/src/cli/vm_boot.rs
- crates/iii-worker/tests/local_worker_integration.rs
andersonleal
force-pushed
the
feat/ci-cross-platform-worker-tests
branch
from
3b6830d to
b91496c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/iii-worker/tests/binary_worker_integration.rs (1)
298-311:⚠️ Potential issue | 🟠 MajorMake
HOMErestoration panic-safe and handle the originally-unset case.Both tests leave process-global state mutated if
HOMEwas unset before the test, and any panic afterset_var()skips restoration entirely. Wrap the override in a small RAII guard that capturesstd::env::var_os("HOME")and restores withset_var/remove_varinDrop.Proposed fix
+use std::ffi::OsString; use std::sync::Mutex; /// Serializes tests that mutate environment variables (HOME). /// Each integration test file runs in its own process, so only intra-file locking is needed. static ENV_LOCK: Mutex<()> = Mutex::new(()); + +struct HomeEnvGuard(Option<OsString>); + +impl Drop for HomeEnvGuard { + fn drop(&mut self) { + // SAFETY: test-only env mutation, serialized via ENV_LOCK + unsafe { + match &self.0 { + Some(home) => std::env::set_var("HOME", home), + None => std::env::remove_var("HOME"), + } + } + } +} @@ fn binary_workers_dir_uses_home() { let _guard = ENV_LOCK.lock().unwrap(); let tmp = tempfile::tempdir().unwrap(); - let original_home = std::env::var("HOME").ok(); + let _home_guard = HomeEnvGuard(std::env::var_os("HOME")); // SAFETY: test-only, serialized via ENV_LOCK unsafe { std::env::set_var("HOME", tmp.path()); } let result = binary_workers_dir(); - - // Restore HOME immediately - unsafe { - if let Some(ref home) = original_home { - std::env::set_var("HOME", home); - } - } @@ fn binary_worker_path_appends_name() { let _guard = ENV_LOCK.lock().unwrap(); let tmp = tempfile::tempdir().unwrap(); - let original_home = std::env::var("HOME").ok(); + let _home_guard = HomeEnvGuard(std::env::var_os("HOME")); // SAFETY: test-only, serialized via ENV_LOCK unsafe { std::env::set_var("HOME", tmp.path()); } let result = binary_worker_path("image-resize"); - - // Restore HOME immediately - unsafe { - if let Some(ref home) = original_home { - std::env::set_var("HOME", home); - } - }Also applies to: 338-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/binary_worker_integration.rs` around lines 298 - 311, The test mutates process-global HOME unsafely and may not restore it on panic; replace the manual original_home/std::env::set_var blocks in binary_worker_integration.rs (around the binary_workers_dir() calls) with a small RAII guard: capture the original value via std::env::var_os("HOME") before overriding, set HOME to the temp path, and implement Drop for the guard to restore using std::env::set_var if Some(value) or std::env::remove_var if None; remove the unsafe manual restore blocks and use the guard to ensure panic-safe restoration.crates/iii-worker/tests/vm_integration.rs (2)
397-404:⚠️ Potential issue | 🟡 MinorDrop the backend-specific VM error text match.
Once you’ve asserted that stderr contains the passed
rootfspath, matching"No init binary available","VM execution failed", or"PassthroughFs failed"just makes this flaky across runtime/backend variants. A non-empty stderr check is enough here.Suggested simplification
- let is_known_error = stderr_str.contains("No init binary available") - || stderr_str.contains("VM execution failed") - || stderr_str.contains("PassthroughFs failed"); - assert!( - is_known_error, - "stderr should contain a known VM boot error message.\nActual stderr: {}", - stderr_str - ); + assert!( + !stderr_str.trim().is_empty(), + "stderr should contain VM error output.\nActual stderr: {}", + stderr_str + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_integration.rs` around lines 397 - 404, Remove the backend-specific string checks (the is_known_error boolean that matches "No init binary available", "VM execution failed", or "PassthroughFs failed") and replace that assertion with a non-empty stderr check: after you already assert stderr contains the passed rootfs path, assert that stderr_str is not empty (e.g., assert!(!stderr_str.is_empty(), "...") or equivalent) so the test no longer depends on backend-specific error text; update/remove references to is_known_error and the corresponding assert to use stderr_str directly.
191-230:⚠️ Potential issue | 🟠 MajorExercise the real env-merge path instead of reimplementing it here.
This test duplicates the merge locally, so it can still pass if
LibkrunAdapter::start()changes precedence or adds normalization. Please assert through the same helper/start path used in production, or extract the merge into a shared function and test that directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_integration.rs` around lines 191 - 230, The test currently reimplements the env-merge logic locally; instead call the real merge implementation used by LibkrunAdapter::start() (the env merging logic around lines 422-429) or factor that merge into a single helper (e.g., extract to a public function like merge_envs(image_env, spec_env) or make the existing helper callable) and have the test exercise that helper/LibkrunAdapter::start()-driven path; update the test to build the same image_env/spec_env inputs and assert results via the shared merge function or by invoking LibkrunAdapter::start() so changes to merge precedence/normalization are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 247-275: The job worker-test-vm-linux is currently configured to
run automatically on ubuntu-latest; change it to run only on self-hosted/manual
triggers by replacing runs-on: ubuntu-latest with runs-on: [self-hosted, linux,
heavy] (or your org's self-hosted labels) and add a job-level guard like if:
github.event_name == 'workflow_dispatch' so the job only runs when manually
dispatched; update the same pattern for the other heavy-test lanes referenced
(the job at lines ~302-330) to use the same self-hosted labels and manual-only
guard.
- Around line 203-355: The PR modifies CI workflow jobs (e.g.,
worker-test-matrix, worker-test-vm-linux, worker-test-vm-macos,
worker-test-oci-linux, worker-test-oci-macos) and therefore requires explicit
repository CI/CD workflow approval before merging; please obtain the required
approval per repo policy (request review/approval from the designated approver
or admin, add the approval comment on this PR, and wait for the confirmation)
and ensure the approval is recorded in the PR before merging.
---
Duplicate comments:
In `@crates/iii-worker/tests/binary_worker_integration.rs`:
- Around line 298-311: The test mutates process-global HOME unsafely and may not
restore it on panic; replace the manual original_home/std::env::set_var blocks
in binary_worker_integration.rs (around the binary_workers_dir() calls) with a
small RAII guard: capture the original value via std::env::var_os("HOME") before
overriding, set HOME to the temp path, and implement Drop for the guard to
restore using std::env::set_var if Some(value) or std::env::remove_var if None;
remove the unsafe manual restore blocks and use the guard to ensure panic-safe
restoration.
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 397-404: Remove the backend-specific string checks (the
is_known_error boolean that matches "No init binary available", "VM execution
failed", or "PassthroughFs failed") and replace that assertion with a non-empty
stderr check: after you already assert stderr contains the passed rootfs path,
assert that stderr_str is not empty (e.g., assert!(!stderr_str.is_empty(),
"...") or equivalent) so the test no longer depends on backend-specific error
text; update/remove references to is_known_error and the corresponding assert to
use stderr_str directly.
- Around line 191-230: The test currently reimplements the env-merge logic
locally; instead call the real merge implementation used by
LibkrunAdapter::start() (the env merging logic around lines 422-429) or factor
that merge into a single helper (e.g., extract to a public function like
merge_envs(image_env, spec_env) or make the existing helper callable) and have
the test exercise that helper/LibkrunAdapter::start()-driven path; update the
test to build the same image_env/spec_env inputs and assert results via the
shared merge function or by invoking LibkrunAdapter::start() so changes to merge
precedence/normalization are covered.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e63bfb18-7d83-4dee-b8c4-8d4891bb249d
📒 Files selected for processing (11)
.github/workflows/ci.ymlcrates/iii-worker/src/cli/binary_download.rscrates/iii-worker/src/cli/vm_boot.rscrates/iii-worker/tests/binary_worker_integration.rscrates/iii-worker/tests/firmware_integration.rscrates/iii-worker/tests/local_worker_integration.rscrates/iii-worker/tests/oci_worker_integration.rscrates/iii-worker/tests/project_detection_integration.rscrates/iii-worker/tests/vm_args_integration.rscrates/iii-worker/tests/vm_integration.rscrates/iii-worker/tests/worker_manager_integration.rs
✅ Files skipped from review due to trivial changes (3)
- crates/iii-worker/tests/oci_worker_integration.rs
- crates/iii-worker/tests/vm_args_integration.rs
- crates/iii-worker/tests/firmware_integration.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/iii-worker/src/cli/binary_download.rs
- crates/iii-worker/src/cli/vm_boot.rs
- crates/iii-worker/tests/worker_manager_integration.rs
- crates/iii-worker/tests/local_worker_integration.rs
andersonleal
force-pushed
the
feat/ci-cross-platform-worker-tests
branch
from
b91496c to
ec2a208
Compare
andersonleal
force-pushed
the
feat/ci-cross-platform-worker-tests
branch
from
ec2a208 to
57a4d33
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/iii-worker/src/cli/vm_boot.rs (1)
111-118:⚠️ Potential issue | 🟡 MinorQuote empty strings explicitly.
shell_quote("")currently returns"", which means an empty arg is lost whenbuild_worker_cmd()is later interpreted by a shell. If empty args are possible here, this should return''instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/vm_boot.rs` around lines 111 - 118, shell_quote currently returns an empty string for "" which causes empty arguments to be lost when the result is interpreted by a shell; update shell_quote (the function named shell_quote in vm_boot.rs used by build_worker_cmd) to explicitly detect an empty input and return "''" and otherwise preserve the existing alphanumeric/unreserved check and the fallback that wraps and escapes single quotes (s.replace('\'', "'\\''")), so empty args are represented as an explicit pair of single quotes.crates/iii-worker/src/cli/binary_download.rs (1)
62-67:⚠️ Potential issue | 🟠 MajorWindows archive selection is inconsistent with the extractor path.
archive_extension()now advertises.zipfor Windows, butdownload_and_install_binary()still unconditionally feeds the payload intoextract_binary_from_targz(). On Windows this will fail on every download. Please either add a zip extraction branch or reject Windows targets until that path exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/binary_download.rs` around lines 62 - 67, archive_extension() returns "zip" for Windows but download_and_install_binary() still always calls extract_binary_from_targz(), causing Windows downloads to fail; update download_and_install_binary() to branch on the archive extension (use archive_extension(target) or target.contains("windows")) and call a new extract_binary_from_zip() for zip archives (implement zip extraction logic) or alternatively return an Err/reject Windows targets explicitly if zip extraction is not implemented; ensure you reference and update the calls around download_and_install_binary, archive_extension, and replace the unconditional extract_binary_from_targz() invocation with a conditional that handles both "tar.gz" and "zip".
♻️ Duplicate comments (7)
crates/iii-worker/tests/vm_integration.rs (1)
191-230:⚠️ Potential issue | 🟠 MajorThis test still reimplements the merge instead of exercising the real path.
Because it merges two local
HashMaps, it won't catch regressions ifLibkrunAdapter::start()changes precedence or adds normalization. Please drive this through the production helper/start path, or extract the merge into a shared helper and test that directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_integration.rs` around lines 191 - 230, The test currently reimplements the env-merge logic instead of exercising the real implementation; update the test to either call the production merge path used by LibkrunAdapter::start() (so the test constructs an OCI image env and a ContainerSpec and invokes the same helper/start flow used in start()) or refactor the merge logic into a shared function (e.g., extract merge_image_and_spec_env into a module-level helper) and have the test call that function directly; ensure you reference and exercise LibkrunAdapter::start()’s merging behavior (lines ~422-429) or the new shared helper so changes to precedence/normalization are caught..github/workflows/ci.yml (2)
203-356:⚠️ Potential issue | 🟡 MinorPlease get the required approval for this workflow change before merging.
This PR modifies
.github/workflows/ci.yml, so it still needs the explicit CI/CD workflow approval required by repo policy.As per coding guidelines "Request approval before making changes to CI/CD workflows (
.github/)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 203 - 356, This change touches the CI workflow file (.github/workflows/ci.yml) — specifically the jobs like worker-test-matrix, worker-test-vm-linux, worker-test-vm-macos, worker-test-oci-linux and worker-test-oci-macos — so obtain the required CI/CD workflow approval before merging: open the PR for the designated approvers or follow the repo's workflow-approval process, add a comment tagging the approver team/person, and wait for explicit approval in the PR before merging.
303-331:⚠️ Potential issue | 🟠 MajorThe Linux OCI lane still doesn't match the PR's heavy-test contract.
worker-test-oci-linuxis still automatic onubuntu-latest, but the PR objective says the Linux heavy lanes should be manual and self-hosted. This will run in hosted CI instead of the intended environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 303 - 331, The CI job worker-test-oci-linux is configured to run automatically on ubuntu-latest but must be a manual, self-hosted heavy-test lane; modify the job definition (worker-test-oci-linux) to require a manual trigger and run on self-hosted runners by replacing runs-on: ubuntu-latest with an appropriate self-hosted label array (e.g., runs-on: [self-hosted, linux, heavy-test]) and add a guard so it only executes on manual dispatch (e.g., if: ${{ github.event_name == 'workflow_dispatch' }} or equivalent at the job level); keep other steps the same.crates/iii-worker/tests/vm_args_integration.rs (1)
404-466:⚠️ Potential issue | 🟠 MajorUse
Child::try_wait()instead ofkill(pid, 0)in these process-lifecycle assertions.Both tests can pass falsely for unreaped zombies:
kill(pid, 0)may still report success after termination, andstd::mem::forget(child)throws away the handle you need to verify reaping deterministically. Keep theChildand asserttry_wait()returnsSome(_)afterstop().#!/bin/bash set -euo pipefail sh -c 'trap "" TERM; sleep 60' & pid=$! kill -KILL "$pid" || true sleep 0.2 echo "kill -0 before wait:" if kill -0 "$pid" 2>/dev/null; then echo "visible" else echo "gone" fi wait "$pid" 2>/dev/null || true echo "kill -0 after wait:" if kill -0 "$pid" 2>/dev/null; then echo "visible" else echo "gone" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/vm_args_integration.rs` around lines 404 - 466, The tests stop_terminates_sleeping_process and stop_escalates_to_sigkill_when_sigterm_ignored wrongly drop the Child and use kill(pid, 0) which can pass for unreaped zombies; instead keep the Child returned by Command::spawn (remove std::mem::forget(child)), call adapter.stop(...), then use child.try_wait() to assert it returns Some(status) (for the SIGKILL case you can await a short sleep like 200-300ms before try_wait to allow reaping); update both tests to assert reaping via Child::try_wait() rather than using nix::libc::kill.crates/iii-worker/tests/binary_worker_integration.rs (1)
294-351:⚠️ Potential issue | 🟠 MajorMake the
HOMEoverride restoration panic-safe and handle the unset case.These tests still only restore
HOMEwhen it originally existed, and the cleanup is skipped entirely on panic. Please switch this to an RAII guard usingvar_os()plusDrop, and callremove_var("HOME")when the original value was unset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/binary_worker_integration.rs` around lines 294 - 351, The tests binary_workers_dir_uses_home and binary_worker_path_appends_name currently set HOME unsafely and only restore it if originally present and on non-panic paths; replace that pattern with an RAII restore guard: capture the original value with std::env::var_os("HOME"), set the new HOME using std::env::set_var(tmp.path()), and create a small drop-restorer (e.g., a local struct RestoreHome { original: Option<OsString> }) whose Drop implementation calls std::env::set_var when original.is_some() or std::env::remove_var("HOME") when original.is_none(); use this guard instead of the unsafe manual restore and keep ENV_LOCK use as-is to maintain serialization.crates/iii-worker/tests/oci_worker_integration.rs (2)
202-216:⚠️ Potential issue | 🟡 MinorThis assertion still rejects valid fallback architectures.
expected_oci_arch()only guaranteesarm64/amd64on the known target mappings; other hosts can legitimately fall back tostd::env::consts::ARCH. This test should assert exact values foraarch64/x86_64and only require non-empty output otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/oci_worker_integration.rs` around lines 202 - 216, The test expected_oci_arch_returns_known_value is too strict for hosts that fall back to std::env::consts::ARCH; update it so that when cfg!(target_arch = "aarch64") you assert expected_oci_arch() == "arm64", when cfg!(target_arch = "x86_64") assert it == "amd64", and for all other architectures assert that expected_oci_arch() is non-empty (or at least not the empty string) instead of forcing it to be one of "arm64" or "amd64"; locate and modify the test function expected_oci_arch_returns_known_value and use the expected_oci_arch() call to implement these conditional assertions.
390-397:⚠️ Potential issue | 🟠 MajorThe gated OCI test is still just a placeholder.
This lane can pass without ever invoking
pull_and_extract_rootfs(), so it doesn't actually cover the code path the new CI job is supposed to validate. A no-network smoke test using an intentionally invalid image reference would exercise the early validation path without depending on registry access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/oci_worker_integration.rs` around lines 390 - 397, The test pull_and_extract_rootfs_placeholder currently only type-checks and doesn't exercise the function; replace the placeholder with a no-network smoke test that actually calls iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs using an intentionally invalid/unreachable image reference (e.g., malformed name or non-existent registry) and assert the expected early validation error (or Err return) rather than requiring network; ensure the test name pull_and_extract_rootfs_placeholder (or rename to reflect behavior) invokes pull_and_extract_rootfs and checks for the proper error variant or message so the CI job validates the early failure path without network access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 236-241: The workflow's upload-artifact step
(actions/upload-artifact@v4, artifact name junit-worker-test-${{ matrix.os }})
is pointing to target/nextest/ci/junit.xml while the nextest CI profile in
.config/nextest.toml writes path = "junit.xml"; fix by either updating the
workflow's with.path to the actual report location produced by the CI profile
(e.g. path: junit.xml) or change the nextest CI profile to output into
target/nextest/ci/junit.xml so both locations match.
---
Outside diff comments:
In `@crates/iii-worker/src/cli/binary_download.rs`:
- Around line 62-67: archive_extension() returns "zip" for Windows but
download_and_install_binary() still always calls extract_binary_from_targz(),
causing Windows downloads to fail; update download_and_install_binary() to
branch on the archive extension (use archive_extension(target) or
target.contains("windows")) and call a new extract_binary_from_zip() for zip
archives (implement zip extraction logic) or alternatively return an Err/reject
Windows targets explicitly if zip extraction is not implemented; ensure you
reference and update the calls around download_and_install_binary,
archive_extension, and replace the unconditional extract_binary_from_targz()
invocation with a conditional that handles both "tar.gz" and "zip".
In `@crates/iii-worker/src/cli/vm_boot.rs`:
- Around line 111-118: shell_quote currently returns an empty string for ""
which causes empty arguments to be lost when the result is interpreted by a
shell; update shell_quote (the function named shell_quote in vm_boot.rs used by
build_worker_cmd) to explicitly detect an empty input and return "''" and
otherwise preserve the existing alphanumeric/unreserved check and the fallback
that wraps and escapes single quotes (s.replace('\'', "'\\''")), so empty args
are represented as an explicit pair of single quotes.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 203-356: This change touches the CI workflow file
(.github/workflows/ci.yml) — specifically the jobs like worker-test-matrix,
worker-test-vm-linux, worker-test-vm-macos, worker-test-oci-linux and
worker-test-oci-macos — so obtain the required CI/CD workflow approval before
merging: open the PR for the designated approvers or follow the repo's
workflow-approval process, add a comment tagging the approver team/person, and
wait for explicit approval in the PR before merging.
- Around line 303-331: The CI job worker-test-oci-linux is configured to run
automatically on ubuntu-latest but must be a manual, self-hosted heavy-test
lane; modify the job definition (worker-test-oci-linux) to require a manual
trigger and run on self-hosted runners by replacing runs-on: ubuntu-latest with
an appropriate self-hosted label array (e.g., runs-on: [self-hosted, linux,
heavy-test]) and add a guard so it only executes on manual dispatch (e.g., if:
${{ github.event_name == 'workflow_dispatch' }} or equivalent at the job level);
keep other steps the same.
In `@crates/iii-worker/tests/binary_worker_integration.rs`:
- Around line 294-351: The tests binary_workers_dir_uses_home and
binary_worker_path_appends_name currently set HOME unsafely and only restore it
if originally present and on non-panic paths; replace that pattern with an RAII
restore guard: capture the original value with std::env::var_os("HOME"), set the
new HOME using std::env::set_var(tmp.path()), and create a small drop-restorer
(e.g., a local struct RestoreHome { original: Option<OsString> }) whose Drop
implementation calls std::env::set_var when original.is_some() or
std::env::remove_var("HOME") when original.is_none(); use this guard instead of
the unsafe manual restore and keep ENV_LOCK use as-is to maintain serialization.
In `@crates/iii-worker/tests/oci_worker_integration.rs`:
- Around line 202-216: The test expected_oci_arch_returns_known_value is too
strict for hosts that fall back to std::env::consts::ARCH; update it so that
when cfg!(target_arch = "aarch64") you assert expected_oci_arch() == "arm64",
when cfg!(target_arch = "x86_64") assert it == "amd64", and for all other
architectures assert that expected_oci_arch() is non-empty (or at least not the
empty string) instead of forcing it to be one of "arm64" or "amd64"; locate and
modify the test function expected_oci_arch_returns_known_value and use the
expected_oci_arch() call to implement these conditional assertions.
- Around line 390-397: The test pull_and_extract_rootfs_placeholder currently
only type-checks and doesn't exercise the function; replace the placeholder with
a no-network smoke test that actually calls
iii_worker::cli::worker_manager::oci::pull_and_extract_rootfs using an
intentionally invalid/unreachable image reference (e.g., malformed name or
non-existent registry) and assert the expected early validation error (or Err
return) rather than requiring network; ensure the test name
pull_and_extract_rootfs_placeholder (or rename to reflect behavior) invokes
pull_and_extract_rootfs and checks for the proper error variant or message so
the CI job validates the early failure path without network access.
In `@crates/iii-worker/tests/vm_args_integration.rs`:
- Around line 404-466: The tests stop_terminates_sleeping_process and
stop_escalates_to_sigkill_when_sigterm_ignored wrongly drop the Child and use
kill(pid, 0) which can pass for unreaped zombies; instead keep the Child
returned by Command::spawn (remove std::mem::forget(child)), call
adapter.stop(...), then use child.try_wait() to assert it returns Some(status)
(for the SIGKILL case you can await a short sleep like 200-300ms before try_wait
to allow reaping); update both tests to assert reaping via Child::try_wait()
rather than using nix::libc::kill.
In `@crates/iii-worker/tests/vm_integration.rs`:
- Around line 191-230: The test currently reimplements the env-merge logic
instead of exercising the real implementation; update the test to either call
the production merge path used by LibkrunAdapter::start() (so the test
constructs an OCI image env and a ContainerSpec and invokes the same
helper/start flow used in start()) or refactor the merge logic into a shared
function (e.g., extract merge_image_and_spec_env into a module-level helper) and
have the test call that function directly; ensure you reference and exercise
LibkrunAdapter::start()’s merging behavior (lines ~422-429) or the new shared
helper so changes to precedence/normalization are caught.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9c328f7-f8ea-429f-8852-3216e55da1bf
📒 Files selected for processing (11)
.github/workflows/ci.ymlcrates/iii-worker/src/cli/binary_download.rscrates/iii-worker/src/cli/vm_boot.rscrates/iii-worker/tests/binary_worker_integration.rscrates/iii-worker/tests/firmware_integration.rscrates/iii-worker/tests/local_worker_integration.rscrates/iii-worker/tests/oci_worker_integration.rscrates/iii-worker/tests/project_detection_integration.rscrates/iii-worker/tests/vm_args_integration.rscrates/iii-worker/tests/vm_integration.rscrates/iii-worker/tests/worker_manager_integration.rs
✅ Files skipped from review due to trivial changes (2)
- crates/iii-worker/tests/project_detection_integration.rs
- crates/iii-worker/tests/local_worker_integration.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/iii-worker/tests/worker_manager_integration.rs
- crates/iii-worker/tests/firmware_integration.rs
sergiofilhowz
approved these changes
Apr 10, 2026
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
2 participants
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
tests/common/module with fixtures, CWD isolation, and path assertion helpersintegration-vm,integration-oci) for heavy tests that require special infrastructureconfig_file_integration.rssplit into 5 focused files,worker_integration.rsmigrated to common helpers)worker-test-matrix(ubuntu + macos, every PR), feature-gated VM/OCI jobs (macOS every PR, Linux self-hosted manual)Test plan
cargo test -p iii-worker— 419 tests pass locally (12s)cargo test -p iii-worker --features integration-vm --no-run— compilescargo test -p iii-worker --features integration-oci --no-run— compilesworker-test-matrixruns on both ubuntu-latest and macos-latestworker-test-vm-macosandworker-test-oci-macosrun on macos-latestSummary by CodeRabbit
New Features
Features
Tests