Skip to content

test(cli): fix local platform assumptions - #1098

Open
G4614 wants to merge 4 commits into
boxlite-ai:mainfrom
G4614:g4614/fix-local-cli-test-assumptions
Open

test(cli): fix local platform assumptions#1098
G4614 wants to merge 4 commits into
boxlite-ai:mainfrom
G4614:g4614/fix-local-cli-test-assumptions

Conversation

@G4614

@G4614 G4614 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Keep CLI integration assertions focused on portable behavior instead of Linux-only permissions or host performance.

Test plan:

  • Run the gvproxy port-conflict test binary on macOS (2 passed, 1 skipped)
  • Run the disk-isolation bystander test against real VMs

Summary by CodeRabbit

  • Tests
    • Improved privileged-port regression test handling across different permission and probing environments.
    • Enhanced disk stress testing to verify concurrent activity during disk exhaustion and confirm expected write failures.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e027572-b3d8-4a72-8c7a-f9228a6574d0

📥 Commits

Reviewing files that changed from the base of the PR and between 7378178 and 794543e.

📒 Files selected for processing (1)
  • src/cli/tests/stress_disk.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli/tests/stress_disk.rs

📝 Walkthrough

Walkthrough

The PR improves CLI test reliability. The privileged-port test handles bind outcomes explicitly. The disk stress test runs the fill asynchronously and verifies concurrent bystander writes and ENOSPC.

Changes

CLI test reliability

Layer / File(s) Summary
Privileged port probe handling
src/cli/tests/gvproxy_port_conflict.rs
The test continues only when port 80 binding fails with permission denied. It skips for successful binds and other probe errors.
Concurrent disk-fill progress assertion
src/cli/tests/stress_disk.rs
The test prepares /work/log, runs the victim fill asynchronously, polls with a deadline, verifies ENOSPC, and requires at least two bystander writes during the fill.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing platform-specific assumptions in CLI tests.
Description check ✅ Passed The description states the purpose and verification steps, but it does not use the template headings or describe risks.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@G4614
G4614 force-pushed the g4614/fix-local-cli-test-assumptions branch from 9eef54b to 3e98847 Compare July 31, 2026 03:38
@G4614
G4614 marked this pull request as ready for review July 31, 2026 03:40
@G4614
G4614 requested a review from a team as a code owner July 31, 2026 03:40
@boxlite-agent

boxlite-agent Bot commented Jul 31, 2026

Copy link
Copy Markdown

📦 BoxLite review — looks good · 794543e

Review evidence

  • git diff --numstat origin/main...HEAD && git diff origin/main...HEAD — 2 test files changed, +105/-33
  • cargo check -p boxlite-cli --tests — no rust toolchain in sandbox; also needs VM-capable host
  • grep exec.rs for -d/detach semantics — confirmed exec -d returns immediately, no lock held

Risk notes

  • stress_disk.rs race window — write-then-still_running check has TOCTOU vs fill.done touch; conservative, only risks undercount not false pass
  • test timing/flakiness — 300s deadline, 1MB dd chunks w/ 0.25s sleep give ample margin for 2 polled writes; not executed here
  • gvproxy_port_conflict.rs — narrows skip to PermissionDenied only + ignores non-linux via cfg_attr; logic correct by inspection
  • coverage — both changed files fully read and reasoned about; no execution possible (no cargo, needs real VM)
src/cli/tests/gvproxy_port_conflict.rs
  gvproxy_privileged_port_fails_fast_with_named_error  +21/-7  skip only on non-EACCES bind error
src/cli/tests/stress_disk.rs
  bystander_writes_keep_progressing_while_peer_fills_its_disk  +84/-26  replaced bg-loop with polled guest fill+writes

reviewed 794543e in a BoxLite microVM · @boxlite-agent review to re-run · powered by BoxLite

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/cli/tests/stress_disk.rs`:
- Around line 670-674: Update the stress test’s fill execution to start the `dd`
process asynchronously, poll the bystander log via `line_count(&bystander)`
while the fill process remains running, and require an observed increase before
awaiting fill completion. Preserve the existing completion handling and progress
assertion, then wait for the fill process to finish after progress is detected.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 59f8463a-28c2-45cd-b8d6-e3ad8f13b412

📥 Commits

Reviewing files that changed from the base of the PR and between 2cfa91f and 3e98847.

📒 Files selected for processing (2)
  • src/cli/tests/gvproxy_port_conflict.rs
  • src/cli/tests/stress_disk.rs

Comment thread src/cli/tests/stress_disk.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/cli/tests/stress_disk.rs`:
- Around line 658-661: Move the fill-test status artifacts currently written to
/tmp— including fill.out and fill.done in the script around the fill loop and
the corresponding paths through the additionally affected cases—onto a guest
tmpfs/ramfs location with a generous independent capacity, or use another
completion channel outside the bounded rootfs. Update all writers, pollers,
cleanup commands, and references consistently so rootfs exhaustion cannot
prevent status reporting.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ad6e353c-7268-43f8-b060-6e9898b423aa

📥 Commits

Reviewing files that changed from the base of the PR and between 9b95c7c and 7378178.

📒 Files selected for processing (1)
  • src/cli/tests/stress_disk.rs

Comment thread src/cli/tests/stress_disk.rs Outdated
Comment on lines +658 to +661
"rm -f /tmp/fill.done /tmp/fill.out; \
i=0; while dd if=/dev/zero of=/fill.$i bs=1M count=1; do \
i=$((i + 1)); sleep 0.25; \
done > /tmp/fill.out 2>&1; touch /tmp/fill.done",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the changed test before inspecting its mount and rootfs setup.
ast-grep outline src/cli/tests/stress_disk.rs --items all
sed -n '608,741p' src/cli/tests/stress_disk.rs

# Locate guest filesystem and mount configuration.
rg -n -i -C 4 'tmpfs|/tmp|rootfs|mount|filesystem|overlay' src

Repository: boxlite-ai/boxlite

Length of output: 50375


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== stress_disk outline =="
ast-grep outline src/cli/tests/stress_disk.rs --items all

echo "== stress_disk relevant sections =="
sed -n '1,120p' src/cli/tests/stress_disk.rs
sed -n '600,745p' src/cli/tests/stress_disk.rs

echo "== non-vendor /tmp-related source references =="
rg -n -i --glob '!src/deps/**' --glob '!src/cli/tests/e2fsprogs/**' -C 3 'tmpfs|/tmp|/tmpfs|mnt|mount|rootfs|filesystem|overlay' src || true

echo "== focused e2fsprogs tmpfs mentions (if any) =="
rg -n -i 'tmpfs|/tmp' src/cli/tests/e2fsprogs src/cli/tests/stress_disk.rs || true

Repository: boxlite-ai/boxlite

Length of output: 50377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== stress_disk.rs outline =="
ast-grep outline src/cli/tests/stress_disk.rs --items all || true

echo "== stress_disk.rs top and target section =="
sed -n '1,100p' src/cli/tests/stress_disk.rs
sed -n '600,745p' src/cli/tests/stress_disk.rs

echo "== target code with /tmp and ENOSPC refs only =="
rg -n -i --glob '!src/deps/**' -C 4 'fill\.done|fill\.out|/tmp|ENOSPC|fill\.\$i|while dd|touch|assert' src/cli/tests/stress_disk.rs src || true

Repository: boxlite-ai/boxlite

Length of output: 50377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== exact stress_disk.rs files =="
fd -a 'stress_disk\.rs$' . || true

echo "== git status/stat for stress_disk.rs =="
git status --short
git diff --stat HEAD src/cli/tests/stress_disk.rs || true

echo "== stress_disk.rs target excerpt, line exact =="
awk '1' src/cli/tests/stress_disk.rs | sed -n '600,745p'

echo "== /tmp mentions in src/cli/tests/stress_disk.rs excluding non-matches =="
rg -n -C 4 'fill\.done|fill\.out|/tmp|ENOSPC|while dd|touch|assert' src/cli/tests/stress_disk.rs || true

echo "== candidate mount setup in src/cli/tests for recent code =="
rg -n -C 3 --glob 'src/cli/tests/**' 'mount|--tmpfs|tmpfs|/tmp|filesystem|overlay' src/cli/tests src/boxlite/src/cli src/boxlite/src/runtime src/boxlite/src/fs src/boxlite/src/litebox src/boxlite/src/vmm src/boxlite/src/disk 2>/dev/null | head -n 400 || true

Repository: boxlite-ai/boxlite

Length of output: 50374


Keep fill status reporting outside the bounded rootfs.

/tmp is checked in the tests as a separate tmpfs, but this script uses it for the completion marker and dd output as well. If /tmp has any allocation limit, dd can exhaust space before writing /tmp/fill.out or creating /tmp/fill.done, making the poll loop time out even though rootfs ENOSPC was hit. Use a guest tmpfs/ramfs path with a generous cap or another non-consumable completion channel.

Also applies to: 676-727

🤖 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 `@src/cli/tests/stress_disk.rs` around lines 658 - 661, Move the fill-test
status artifacts currently written to /tmp— including fill.out and fill.done in
the script around the fill loop and the corresponding paths through the
additionally affected cases—onto a guest tmpfs/ramfs location with a generous
independent capacity, or use another completion channel outside the bounded
rootfs. Update all writers, pollers, cleanup commands, and references
consistently so rootfs exhaustion cannot prevent status reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant