test(e2e): add E2E smoke test harness and initial smoke tests#187
test(e2e): add E2E smoke test harness and initial smoke tests#187AprilNEA wants to merge 2 commits into
Conversation
Introduce the `arcbox-e2e-smoke` crate under `tests/e2e-smoke/` with a reusable `TestEnvironment` that manages the full daemon + helper lifecycle. The harness kills stale processes, cleans sockets, verifies port 5553 is free, starts the helper (sudo) and signed daemon, then waits for the "ArcBox daemon started" marker before yielding control to tests. Teardown uses SIGTERM with SIGKILL fallback and cleans up leftover containers.
Three ignored tests that exercise the full ArcBox stack via the Docker CLI: - container_run_and_exit: `docker run --rm alpine echo hello-arcbox` - docker_ps: verify `docker ps` succeeds - docker_info: verify `docker info` returns a non-empty server version Run with: cargo test -p arcbox-e2e-smoke --test smoke -- --ignored --test-threads=1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348c5725b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use tokio::sync::OnceCell; | ||
|
|
||
| /// Shared test environment, initialized once across all tests. | ||
| static ENV: OnceCell<TestEnvironment> = OnceCell::const_new(); |
There was a problem hiding this comment.
Ensure test environment cleanup actually runs
The shared environment is stored in a static OnceCell<TestEnvironment> and only exposed as &'static TestEnvironment, so TestEnvironment::drop is never invoked on normal test process shutdown. That means the teardown logic (killing daemon/helper, removing containers/sockets) is skipped after successful runs, leaving background ArcBox processes alive and contaminating later test executions.
Useful? React with 👍 / 👎.
| ) | ||
| .into()); | ||
| } | ||
| let helper_process = start_helper(&helper_bin)?; |
There was a problem hiding this comment.
Avoid leaking helper process on startup errors
The helper is spawned before later fallible daemon setup steps, but those steps can return early (missing daemon binary, log file creation failure, daemon spawn failure) before TestEnvironment is fully constructed. In those error paths the Child handle is dropped without termination, leaving a privileged arcbox-helper process running and requiring manual cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new arcbox-e2e-smoke crate under tests/ that provides a shared TestEnvironment harness to boot the full ArcBox stack (helper + daemon) and run a small set of ignored E2E smoke tests against the Docker API.
Changes:
- Introduces an E2E smoke-test harness (
TestEnvironment) intended to manage helper/daemon lifecycle and cleanup. - Adds three initial ignored tokio-based smoke tests (
container_run_and_exit,docker_ps,docker_info) for macOS. - Registers the new test crate in the workspace.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/e2e-smoke/src/harness.rs |
Implements the shared environment lifecycle (process management, readiness wait, teardown, docker helper). |
tests/e2e-smoke/src/lib.rs |
Exposes the harness as the crate’s public API. |
tests/e2e-smoke/tests/smoke.rs |
Adds initial ignored smoke tests using the harness (macOS-only). |
tests/e2e-smoke/Cargo.toml |
Adds the new test crate manifest and dependencies. |
Cargo.toml |
Adds tests/e2e-smoke to workspace members. |
Cargo.lock |
Adds lockfile entry for the new crate. |
| use tokio::sync::OnceCell; | ||
|
|
||
| /// Shared test environment, initialized once across all tests. | ||
| static ENV: OnceCell<TestEnvironment> = OnceCell::const_new(); | ||
|
|
||
| /// Full ArcBox environment for E2E tests. | ||
| /// | ||
| /// Owns the daemon and helper child processes and tears them down on drop. | ||
| pub struct TestEnvironment { | ||
| daemon_process: Option<Child>, | ||
| helper_process: Option<Child>, | ||
| docker_host: String, |
There was a problem hiding this comment.
static ENV: OnceCell<TestEnvironment> will require TestEnvironment: Sync for a static, but std::process::Child is not Sync, so this is likely to fail to compile. Consider storing the environment behind a Mutex/RwLock (e.g., OnceCell<Mutex<TestEnvironment>>) or storing only PIDs/handles that are Sync and doing process management via those.
| let is_ready = l.contains("ArcBox daemon started"); | ||
| let _ = tx.send(l); | ||
| if is_ready { | ||
| break; | ||
| } |
There was a problem hiding this comment.
wait_for_daemon_ready stops reading the daemon's stdout once the ready marker is seen (break), leaving stdout undrained for the rest of the test run. If the daemon writes more than a pipe buffer afterwards, it can block and stall the tests. Keep draining stdout in a background thread/task for the daemon’s lifetime (or redirect stdout to a file instead of piping it).
| let is_ready = l.contains("ArcBox daemon started"); | |
| let _ = tx.send(l); | |
| if is_ready { | |
| break; | |
| } | |
| // Keep draining stdout for the daemon lifetime even | |
| // after the ready marker is observed. Otherwise the | |
| // daemon can block on a full stdout pipe later in the | |
| // test run. If the receiver has already gone away, | |
| // ignore the send failure and continue draining/logging. | |
| let _ = tx.send(l); |
| let log_file_stderr = std::fs::File::create(&log_file)?; | ||
|
|
||
| let daemon_process = Command::new(&daemon_bin) | ||
| .env("ARCBOX_HELPER_SOCKET", "/tmp/arcbox-helper.sock") | ||
| .stdout(Stdio::piped()) | ||
| .stderr(log_file_stderr) | ||
| .spawn() | ||
| .map_err(|e| format!("failed to start arcbox-daemon: {e}"))?; |
There was a problem hiding this comment.
The daemon stderr is written via a File::create handle (no O_APPEND), while the stdout thread appends to the same log file with OpenOptions::append(true). Concurrent writes using different offsets can overwrite/interleave unpredictably. Use separate stdout/stderr log files, or open a single shared file handle with append semantics for both streams.
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) |
There was a problem hiding this comment.
start_helper pipes stdout/stderr but the harness never reads them. If arcbox-helper logs to stderr (it does by default), the pipe can fill and block the helper, causing flakiness/hangs. Redirect helper output to a log file or Stdio::inherit()/Stdio::null() instead of piping without draining.
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()) | |
| // Do not pipe helper output unless we actively drain it. | |
| // `arcbox-helper` logs to stderr by default, and an undrained pipe can | |
| // fill up and block the helper, causing flaky or hanging E2E tests. | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) |
| // Helper runs as root. | ||
| let _ = Command::new("sudo") | ||
| .args(["pkill", "-9", "-f", "arcbox-helper"]) | ||
| .output(); |
There was a problem hiding this comment.
All sudo invocations (sudo pkill ..., sudo -E <helper>) can block waiting for a password prompt, which is particularly painful in cargo test. Prefer sudo -n and surface a clear error if passwordless sudo is not available, or perform an explicit preflight check (sudo -n true).
| // Helper runs as root. | |
| let _ = Command::new("sudo") | |
| .args(["pkill", "-9", "-f", "arcbox-helper"]) | |
| .output(); | |
| // Helper runs as root. Use non-interactive sudo so tests fail fast instead | |
| // of blocking on a password prompt when passwordless sudo is unavailable. | |
| match Command::new("sudo") | |
| .args(["-n", "pkill", "-9", "-f", "arcbox-helper"]) | |
| .output() | |
| { | |
| Ok(output) if !output.status.success() => { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| eprintln!( | |
| "[e2e] warning: failed to kill stale arcbox-helper with `sudo -n`; \ | |
| passwordless sudo may be required for cleanup: {}", | |
| stderr.trim() | |
| ); | |
| } | |
| Err(err) => { | |
| eprintln!( | |
| "[e2e] warning: failed to execute `sudo -n pkill` for arcbox-helper cleanup: {err}" | |
| ); | |
| } | |
| Ok(_) => {} | |
| } |
| let home = dirs::home_dir().ok_or("cannot determine home directory")?; | ||
| let arcbox_run = home.join(".arcbox/run"); | ||
| let docker_socket = arcbox_run.join("docker.sock"); | ||
| let docker_host = format!("unix://{}", docker_socket.display()); |
There was a problem hiding this comment.
The harness hardcodes ~/.arcbox/run/docker.sock for DOCKER_HOST. ArcBox already has HostLayout as the source of truth for run/socket paths (and supports non-default data dirs); using it here would prevent the harness from pointing at the wrong socket if layout resolution changes or is overridden.
| let home = dirs::home_dir().ok_or("cannot determine home directory")?; | |
| let arcbox_run = home.join(".arcbox/run"); | |
| let docker_socket = arcbox_run.join("docker.sock"); | |
| let docker_host = format!("unix://{}", docker_socket.display()); | |
| let host_layout = arcbox_core::HostLayout::resolve()?; | |
| let arcbox_run = host_layout.run_dir().to_path_buf(); | |
| let docker_host = format!("unix://{}", host_layout.docker_socket().display()); |
| path = "tests/smoke.rs" | ||
|
|
||
| [dependencies] | ||
| dirs = "6" |
There was a problem hiding this comment.
This crate depends on dirs = "6" directly even though dirs is already defined in [workspace.dependencies]. Using dirs = { workspace = true } here keeps the version centralized and consistent with the rest of the workspace.
| dirs = "6" | |
| dirs = { workspace = true } |
Summary
arcbox-e2e-smokecrate with aTestEnvironmentharness that manages the full ArcBox stack lifecycle (helper + daemon) for E2E testscontainer_run_and_exit,docker_ps,docker_infoLinear: ABX-304, ABX-305
TestEnvironmentlifecyclearcbox-daemon/arcbox-helper/ desktop daemon~/.arcbox/run/*.sock,/tmp/arcbox-helper.sock)sudo -Ewith/tmp/arcbox-helper.sockUses
tokio::sync::OnceCellto share one daemon across all tests.New files
tests/e2e-smoke/Cargo.tomltests/e2e-smoke/src/harness.rsTestEnvironmentstruct + lifecycletests/e2e-smoke/src/lib.rstests/e2e-smoke/tests/smoke.rsHow to run
Test plan
container_run_and_exitpasses with running ArcBox environment#[ignore])