ci: enable docker integration tests on macOS in coverage workflow#1351
Open
ci: enable docker integration tests on macOS in coverage workflow#1351
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGitHub Actions coverage workflow updated to run on macOS-13 and provision Docker on macOS by installing and starting Colima; Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Runner as "macOS-13 runner"
participant Colima as "Colima"
participant Docker as "Docker Engine"
participant Tests as "Test/Coverage job"
GH->>Runner: start coverage job (matrix includes macOS-13)
Runner->>Runner: check OS == macOS
alt macOS
Runner->>Colima: install colima & docker
Runner->>Colima: colima start (cpu=2, memory=4GB)
Colima->>Docker: provision Docker Engine
end
Runner->>Tests: set CLASH_DOCKER_TEST="true" and run tests
Tests->>Docker: execute containerized tests (if used)
Tests->>GH: report coverage results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ibigbug
force-pushed
the
coverage/docker-all-platforms
branch
from
f6479ac to
ee7e3d2
Compare
![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: 2
🤖 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/coverage.yml:
- Around line 55-59: Add the missing CI-mode environment variable to the
coverage test step by adding CLASH_RS_CI: "true" to the env block where
CROSS_CONTAINER_OPTS, CLASH_DOCKER_TEST, TS_AUTH_KEY, and RUST_LOG are defined
so tests run with CI-mode paths enabled; update the env mapping in the same
workflow step to include CLASH_RS_CI with the string value "true".
- Around line 45-47: The "Set up Docker" step uses
crazy-max/ghaction-setup-docker@v3 which fails on Apple Silicon; update the
workflow matrix entries that use "macos-latest" to "macos-15-intel" so the macOS
runner is Intel-based and Docker is supported (keep the existing step name "Set
up Docker" and the conditional if: runner.os != 'Linux' 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d7ccfd2-82c3-45d2-ae63-7e05c6516a3d
📒 Files selected for processing (1)
.github/workflows/coverage.yml
Comment on lines
55
to
59
| env: | ||
| CROSS_CONTAINER_OPTS: "--network host" | ||
| CLASH_DOCKER_TEST: ${{ startsWith(matrix.os, 'ubuntu') && 'true' || 'false' }} | ||
| CLASH_DOCKER_TEST: "true" | ||
| TS_AUTH_KEY: ${{ secrets.TS_AUTH_KEY }} | ||
| RUST_LOG: ts_control=debug,ts_runtime=debug,tailscale=debug |
Contributor
There was a problem hiding this comment.
Missing CI-mode flag in coverage test environment.
Please set CLASH_RS_CI: "true" in this step’s env so CI-mode test paths are consistently enabled.
Suggested fix
env:
CROSS_CONTAINER_OPTS: "--network host"
CLASH_DOCKER_TEST: "true"
+ CLASH_RS_CI: "true"
TS_AUTH_KEY: ${{ secrets.TS_AUTH_KEY }}
RUST_LOG: ts_control=debug,ts_runtime=debug,tailscale=debugBased on learnings: Use CLASH_RS_CI=true environment variable when running tests in CI mode.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| env: | |
| CROSS_CONTAINER_OPTS: "--network host" | |
| CLASH_DOCKER_TEST: ${{ startsWith(matrix.os, 'ubuntu') && 'true' || 'false' }} | |
| CLASH_DOCKER_TEST: "true" | |
| TS_AUTH_KEY: ${{ secrets.TS_AUTH_KEY }} | |
| RUST_LOG: ts_control=debug,ts_runtime=debug,tailscale=debug | |
| env: | |
| CROSS_CONTAINER_OPTS: "--network host" | |
| CLASH_DOCKER_TEST: "true" | |
| CLASH_RS_CI: "true" | |
| TS_AUTH_KEY: ${{ secrets.TS_AUTH_KEY }} | |
| RUST_LOG: ts_control=debug,ts_runtime=debug,tailscale=debug |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coverage.yml around lines 55 - 59, Add the missing CI-mode
environment variable to the coverage test step by adding CLASH_RS_CI: "true" to
the env block where CROSS_CONTAINER_OPTS, CLASH_DOCKER_TEST, TS_AUTH_KEY, and
RUST_LOG are defined so tests run with CI-mode paths enabled; update the env
mapping in the same workflow step to include CLASH_RS_CI with the string value
"true".
12a611f to
f42eeab
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ibigbug
force-pushed
the
coverage/docker-all-platforms
branch
3 times, most recently
from
6b2b4ed to
13e28f5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/coverage.yml (2)
57-61:⚠️ Potential issue | 🟡 MinorMissing CI-mode flag in the coverage test env.
Please add
CLASH_RS_CI: "true"in this env block so CI-mode test paths are consistently enabled.Suggested fix
env: CROSS_CONTAINER_OPTS: "--network host" CLASH_DOCKER_TEST: "true" + CLASH_RS_CI: "true" TS_AUTH_KEY: ${{ secrets.TS_AUTH_KEY }} RUST_LOG: ts_control=debug,ts_runtime=debug,tailscale=debugBased on learnings: Use CLASH_RS_CI=true environment variable when running tests in CI mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml around lines 57 - 61, Add the CI-mode env var CLASH_RS_CI: "true" to the coverage job's environment block so CI-specific test paths run; update the same env mapping that currently contains CROSS_CONTAINER_OPTS, CLASH_DOCKER_TEST, TS_AUTH_KEY and RUST_LOG to include CLASH_RS_CI: "true" alongside those variables.
20-20:⚠️ Potential issue | 🔴 CriticalUnsupported runner label will break the macOS matrix leg.
Line 20 uses
macos-13, which is not a supported hosted runner label in current GitHub Actions labels (per your actionlint output), so this job entry can fail to schedule.Suggested fix
- os: ["macos-13", "windows-latest", "ubuntu-latest"] + os: ["macos-15-intel", "windows-latest", "ubuntu-latest"]What are the currently supported GitHub-hosted macOS runner labels in GitHub Actions, and is `macos-13` still valid?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml at line 20, The macOS runner label "macos-13" in the matrix under the "os" entry is unsupported and will fail to schedule; update that value to a currently supported runner label (e.g., "macos-latest" or a supported specific label like "macos-12"/"macos-14" per current GitHub-hosted runners) by replacing "macos-13" in the os: ["macos-13", "windows-latest", "ubuntu-latest"] list so the matrix uses a valid macOS runner.
🤖 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/coverage.yml:
- Around line 45-50: The Windows runner is enabling CLASH_DOCKER_TEST but there
is no Docker/Linux container setup for Windows; update the workflow so
Docker-based tests are skipped on Windows by gating the CLASH_DOCKER_TEST env or
the job step with an OS check (use the workflow symbol CLASH_DOCKER_TEST and the
job/step that runs the Docker tests), e.g., add a condition like runner.os !=
'Windows' to the env assignment or to the test step that runs the Docker
containers, ensuring only macOS/Linux runners run the Docker tests
(alternatively implement a full WSL2/Docker-in-WSL setup if you choose option
2).
---
Duplicate comments:
In @.github/workflows/coverage.yml:
- Around line 57-61: Add the CI-mode env var CLASH_RS_CI: "true" to the coverage
job's environment block so CI-specific test paths run; update the same env
mapping that currently contains CROSS_CONTAINER_OPTS, CLASH_DOCKER_TEST,
TS_AUTH_KEY and RUST_LOG to include CLASH_RS_CI: "true" alongside those
variables.
- Line 20: The macOS runner label "macos-13" in the matrix under the "os" entry
is unsupported and will fail to schedule; update that value to a currently
supported runner label (e.g., "macos-latest" or a supported specific label like
"macos-12"/"macos-14" per current GitHub-hosted runners) by replacing "macos-13"
in the os: ["macos-13", "windows-latest", "ubuntu-latest"] list so the matrix
uses a valid macOS runner.
🪄 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: 655b2260-ad07-44d9-99ba-87e8bc9f22ea
📒 Files selected for processing (1)
.github/workflows/coverage.yml
ibigbug
force-pushed
the
coverage/docker-all-platforms
branch
2 times, most recently
from
479ba33 to
9668a1d
Compare
Add crazy-max/ghaction-setup-docker@v3 step for macOS and Windows to provide Docker with Linux container support. Change CLASH_DOCKER_TEST from ubuntu-only to all platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ibigbug
force-pushed
the
coverage/docker-all-platforms
branch
from
9668a1d to
20925c4
Compare
- Add host_port field to DockerTestRunner (threaded from builder) - Add host_port() accessor method - container_ip() returns None when CLASH_DOCKER_USE_HOST_IP is set, so callers fall back to 127.0.0.1 + published host port - Fix WireGuard test to use runner.host_port() when env var is set (WG uses different host/container ports unlike all other tests) - Set CLASH_DOCKER_USE_HOST_IP=true in CI for macOS colima setup On macOS with OrbStack (local dev), container IPs are routable via the 'orb' tunnel interface, so leaving CLASH_DOCKER_USE_HOST_IP unset preserves the existing behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ibigbug
changed the title
Contributor
📊 Proxy Throughput ResultsShadowsocks
Trojan
VMess
VLESS
SOCKS5
AnyTLS
Hysteria2
TUIC
ShadowQUIC
SSH
Netem Tests (50 ms delay, 1% packet loss)Shadowsocks
Trojan
Hysteria2
TUIC
ShadowQUIC
Ran 34 variant(s) in parallel; each direction transfers the full payload. 🖥️ Test Environment
📎 View full workflow run and download artifacts Full test logDownload the |
When CLASH_DOCKER_USE_HOST_IP is set (macOS CI with colima): 1. docker_runner: trigger API-based file copy instead of bind mounts. macOS temp paths (/var/folders/…) don't exist inside the colima VM, so bind mounts fail with ENOENT. The existing remote-Docker tar-upload path already handles this correctly; extend the condition to cover CLASH_DOCKER_USE_HOST_IP in addition to HTTP/TCP DOCKER_HOST URLs. 2. run_test_suites_and_cleanup: skip PingPongUdp and DnsUdp suites. colima uses QEMU SLIRP networking; SLIRP does not relay arbitrary UDP from containers back to macOS host processes on random ephemeral ports, so UDP reverse-path connectivity is unavailable. TCP suites still run and provide meaningful proxy-correctness coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SLIRP's virtual 192.168.5.2 (what host.docker.internal resolves to inside containers) does not proxy arbitrary TCP ports back to macOS localhost. Connections from container to the test binary's ephemeral echo-server port receive TCP RST (ECONNREFUSED). This affects all ping-pong-style tests that require the proxy container to initiate a connection back to the host. Only LatencyTcp (outbound: binary → container → internet) is reliable with colima SLIRP networking. Skip PingPongTcp, PingPongUdp, and DnsUdp when CLASH_DOCKER_USE_HOST_IP is set; colima CI will run LatencyTcp only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… set Add DockerTestRunner::server_port(container_port) helper that returns host_port when container_ip() is None (macOS colima mode), or the given container_port when running with direct container IP access. All docker integration tests were using the hardcoded container port (e.g. 10002, 2222, 8443) even when falling back to 127.0.0.1 as the server address. Lima only port-forwards the dynamically allocated host_port to the container's port — it does NOT expose the container port directly on macOS localhost. This caused all 18 non-WireGuard tests to time out. Tests fixed: socks5, trojan (ws+grpc), vmess (ws+grpc+h2), hysteria2, anytls, ssh, vless. tuic and shadowquic already had this pattern correct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous approach used colima with QEMU SLIRP networking and Lima's SSH-based port forwarding. This only works for TCP — UDP ports (QUIC, WireGuard) published by Docker containers are never forwarded to the macOS host, causing tuic, shadowquic, hysteria2, and wg tests to always time out. Fix by using socket_vmnet + colima --network-address, which gives the colima VM a real IP address reachable from macOS for both TCP and UDP. The VM IP is exported as CLASH_DOCKER_HOST_IP and used by the test harness instead of 127.0.0.1. Changes: - coverage.yml: install socket_vmnet, start colima with --network-address, export CLASH_DOCKER_HOST_IP=<vm-ip> instead of CLASH_DOCKER_USE_HOST_IP - docker_runner.rs: add colima_host_ip() helper; container_ip() returns Some(vm_ip) when CLASH_DOCKER_HOST_IP is set so callers connect to the VM's real IP; server_port() returns host_port in that case (Docker binds host_port on the VM IP, not the container-internal port) - mod.rs: update use_host_ip check to CLASH_DOCKER_HOST_IP - wg/mod.rs: replace manual env-var check with runner.server_port(10002) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ort tests When CLASH_DOCKER_HOST_IP cannot be extracted (socket_vmnet fails to assign a routable IP), fall back to CLASH_DOCKER_USE_HOST_IP=true which uses 127.0.0.1 via Lima's TCP SSH port-forwarding. Changes: - coverage.yml: add robust IP extraction with debugging + fallback to CLASH_DOCKER_USE_HOST_IP=true when socket_vmnet IP unavailable - docker_runner.rs: restore CLASH_DOCKER_USE_HOST_IP -> 127.0.0.1 support as fallback when CLASH_DOCKER_HOST_IP is not set - mod.rs: add skip_if_tcp_only_host_mode() helper; check both env vars for PingPong/DnsUdp suite skip logic - hysteria2, shadowquic, tuic, wg tests: skip early when TCP-only host mode is active (Lima cannot forward UDP for QUIC/WireGuard transport) - Cross.toml: add CLASH_DOCKER_HOST_IP and CLASH_DOCKER_USE_HOST_IP to env passthrough so cross containers see the values Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace 'sudo brew services start socket_vmnet' (launchd service) with 'limactl sudoers' which lets Lima spawn socket_vmnet directly with elevated privileges — the documented CI approach and more reliable. Use set -euo pipefail + fail loudly (Python raises RuntimeError) if the colima address is empty, so setup failures surface immediately rather than silently falling back and breaking all tests. Remove all test skip logic — tests must always run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Lima enforces that the socket_vmnet binary is owned by root (uid 0) as a security check. brew installs it owned by the runner user (uid 501). Fix by chowning to root:wheel immediately after brew install. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… check Lima 2.x validates that the socket_vmnet binary AND every ancestor directory up to / is owned by root (uid 0). Previously we only chowned the binary itself; Lima then failed on the parent bin/ directory. Switch from single-file chown to: sudo chown -R root:wheel "$(brew --cellar socket_vmnet)" This covers socket_vmnet, socket_vmnet/1.x.x, socket_vmnet/1.x.x/bin, and the binary itself in one pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ership check Lima 2.x walks every ancestor directory of the socket_vmnet binary up to / and requires each to be owned by root. On GitHub's Intel macOS runners, /usr/local/Cellar (and subdirs) are owned by the runner user (uid 501), so no amount of chowning inside Cellar is enough. Instead, copy the binary to /usr/local/bin (which is root-owned, with all ancestors root-owned) and write a minimal ~/.lima/_config/networks.yaml to redirect Lima's socketVMNet path there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub macOS Intel runners give the runner user (uid 501) ownership of everything under /usr/local, including /usr/local/bin. Lima's security check requires every ancestor dir of the binary to be root-owned. The Lima docs explicitly recommend /opt/socket_vmnet as the install path — /opt is root-owned on macOS Intel since Homebrew doesn't touch it. sudo mkdir -p /opt/socket_vmnet/bin sudo cp <brew binary> /opt/socket_vmnet/bin/socket_vmnet Point networks.yaml at /opt/socket_vmnet/bin/socket_vmnet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The socketVMNet ownership check now passes (/opt/socket_vmnet works).
New failure: varRun resolved to '.' because Lima doesn't merge user
networks.yaml with system defaults — unspecified fields go to Go zero
values (''), which resolve to the current directory.
Include all three paths fields explicitly:
socketVMNet: /opt/socket_vmnet/bin/socket_vmnet
varRun: /private/var/run/lima
sudoers: /private/etc/sudoers.d/lima
Also install the sudoers file to /private/etc/sudoers.d/lima to match.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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
Enable Docker integration tests (
#[cfg(docker_test)]) on macOS in the coverage workflow, in addition to the existing Ubuntu support. Windows is excluded because GitHub-hosted Windows runners don't support Linux containers.Changes
Workflow (
.github/workflows/coverage.yml)macos-latest(ARM, no nested virt) tomacos-15-intel(Intel, supports colima)/var/run/docker.sockCLASH_DOCKER_USE_HOST_IP=truefor macOS — tells the test harness to use127.0.0.1+ published host ports instead of unreachable container-internal IPsCLASH_DOCKER_TEST: truefor Linux and macOS;falsefor WindowsTest harness (
docker_runner.rs)host_port: u16field toDockerTestRunner(threaded from builder's_server_port)host_port()accessorcontainer_ip()returnsNonewhenCLASH_DOCKER_USE_HOST_IPis set, so callers fall back to"127.0.0.1"+ the published host port (forwarded by colima automatically)WireGuard test (
wg/mod.rs)runner.host_port()instead of hardcoded10002whenCLASH_DOCKER_USE_HOST_IPis set, since WireGuard is the only test where host port ≠ container portWhy this works per environment
container_ip()172.17.x.x(routable)172.17.x.x(routable via orb tunnel)None(env var set)127.0.0.1+ host port (colima-forwarded)