fix(test): fix all unit tests for macOS Apple Silicon#1039
Conversation
This commit fixes all unit tests to pass on macOS with M-series chips. Key changes: - Enhanced DockerTestRunner with container IP retrieval for macOS bridge mode - Replaced socks5 image with v2fly for cross-platform compatibility - Fixed cross-container communication using host.docker.internal - Fixed WireGuard UDP checksum verification on macOS virtualization - Added missing test initialize() calls - Fixed test config paths and added proper feature gates
There was a problem hiding this comment.
Pull request overview
Fixes macOS Apple Silicon test failures by adjusting Docker-based test networking assumptions, updating test fixtures, and adding platform-specific workarounds.
Changes:
- Updated Docker test runner to improve startup cleanup and expose container/gateway IP helpers.
- Adjusted multiple tests/configs to work with Docker Desktop networking (ports, addresses, feature gates, initialization).
- Added workaround to recompute IPv4 UDP checksums for WireGuard packets in environments where checksums get corrupted.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| clash-lib/tests/smoke_tests.rs | Gates Shadowsocks smoke test behind shadowsocks feature. |
| clash-lib/tests/data/config/client/rules.yaml | Updates DNS cert/key paths to point at repo test assets. |
| clash-lib/tests/api_tests.rs | Gates API tests behind shadowsocks feature. |
| clash-lib/src/proxy/wg/mod.rs | Refactors test imports and adds initialize() call. |
| clash-lib/src/proxy/wg/device.rs | Recalculates IPv4 UDP checksum on RX before handing packet to smoltcp. |
| clash-lib/src/proxy/vless/mod.rs | Publishes VLESS container port to avoid binding/connectivity issues. |
| clash-lib/src/proxy/utils/test_utils/docker_utils/docker_runner.rs | Adds container inspect caching, cleanup-on-start-failure, IP helpers, and improved multi-container cleanup. |
| clash-lib/src/proxy/utils/test_utils/docker_utils/consts.rs | Switches SOCKS5 test image to v2fly/v2fly-core. |
| clash-lib/src/proxy/trojan/mod.rs | Refactors test imports and adds initialize() call. |
| clash-lib/src/proxy/socks/outbound/mod.rs | Reworks SOCKS5 tests to use v2fly config files and dynamic server address. |
| clash-lib/src/proxy/shadowsocks/outbound/mod.rs | Publishes SS ports, uses host.docker.internal for cross-container communication, adds initialize(). |
| clash-lib/src/proxy/group/smart/mod.rs | Adds initialize() and resolves test port conflict. |
| clash-lib/src/proxy/group/relay/mod.rs | Refactors test imports and adds initialize() calls. |
| clash-lib/src/app/remote_content_manager/providers/proxy_provider/proxy_set_provider.rs | Simplifies provider test fixture from SS to SOCKS5. |
| clash-bin/tests/data/config/socks5-noauth.json | Adds v2fly config for unauthenticated SOCKS5 inbound. |
| clash-bin/tests/data/config/socks5-auth.json | Adds v2fly config for authenticated SOCKS5 inbound. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace nested map().flatten() chains with and_then() for cleaner code - Remove .unwrap() calls in gateway_ip to eliminate panic risks - Both methods now safely return None if any intermediate value is missing
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Thanks for the work and I think this would help us having more coverage on macOS for PRs! |
- Add support for remote Docker daemon connections through DOCKER_HOST
environment variable with support for http/https/tcp, unix, and npipe protocols
|
I still feel there's a bit of an issue here. Let me finish testing both the main Windows and Linux versions first before we proceed. |
Enable verbose output for Git commands in CI. Signed-off-by: iHsin <sun@ihsin.dev>
Signed-off-by: iHsin <sun@ihsin.dev>
Signed-off-by: iHsin <sun@ihsin.dev>
Use dynamic port allocation instead of hardcoded ports (53553-53557) to avoid conflicts with running services.
- Remove unnecessary clone in hysteria2 fragment creation - Refactor nested if-let to let-chain pattern in wireguard device
|
This is basically all modified. During testing, two main issues were fixed. There were quite a few changes, so it might need a thorough review. Other targets have not been tested yet. For testing, I added a few things to docker-test, such as |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I don't see any major issues with the changes above. The main audit points might be: These two points may require manual review:
|
|
Impressive work—this PR tackles a huge cross-platform testing surface and still keeps everything cohesive. The Docker test refactors (including DOCKER_HOST support and macOS networking fixes) feel especially thorough and “perfectionist” in the best way, and it’s great to see CI and unit tests coming out reliably green across targets. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let futs = vec![proxy_task, target_local_server_handler]; | ||
|
|
||
| select_all(futs).await.0? | ||
| let res = select_all(futs).await.0?; | ||
| tx.send(()).ok(); // signal the target local server to shutdown | ||
| res |
There was a problem hiding this comment.
select_all(futs).await returns as soon as either the proxy task or the local server task finishes. If the proxy finishes first, the local server task is only signaled via tx.send(()) but not awaited, so this function can return while the listener is still bound (race), potentially causing port-in-use flakes in subsequent tests. Consider always awaiting the server task after sending the shutdown signal (or use tokio::join! / tokio::select! that waits for proxy completion then gracefully shuts down and awaits the server).
| Err(_) => { | ||
| tracing::error!( | ||
| "connect_stream timeout (5s) for destination(tcp): {}", | ||
| destination | ||
| ); |
There was a problem hiding this comment.
The timeout duration passed to tokio::time::timeout is 3s, but the error log says connect_stream timeout (5s). This makes debugging confusing; please update the message to match the actual timeout value.
| let res = select_all(futs).await.0?; | ||
| tx.send(()).ok(); |
There was a problem hiding this comment.
Same as the TCP helper: select_all(futs).await returns when the first task completes, then the function returns without awaiting target_local_server_handler after signaling shutdown. This can leave the UDP socket bound briefly and cause flakiness in later tests; consider awaiting the server task before returning.
| let res = select_all(futs).await.0?; | |
| tx.send(()).ok(); | |
| let (res, _idx, remaining) = select_all(futs).await; | |
| tx.send(()).ok(); | |
| for handle in remaining { | |
| let _ = handle.await; | |
| } |
| info!("Error accepting connection(tcp): {}", e); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| _ = &mut rx => { | ||
| info!("target_local_server_handler(tcp) received shutdown signal, exiting..."); |
There was a problem hiding this comment.
In this UDP code path the log messages still say tcp (e.g. Error accepting connection(tcp) / target_local_server_handler(tcp)), which is misleading when diagnosing UDP failures. Please rename these to udp to match the actual transport.
| info!("Error accepting connection(tcp): {}", e); | |
| continue; | |
| } | |
| } | |
| } | |
| _ = &mut rx => { | |
| info!("target_local_server_handler(tcp) received shutdown signal, exiting..."); | |
| info!("Error accepting connection(udp): {}", e); | |
| continue; | |
| } | |
| } | |
| } | |
| _ = &mut rx => { | |
| info!("target_local_server_handler(udp) received shutdown signal, exiting..."); |
| #[cfg(feature = "shadowsocks")] | ||
| #[tokio::test(flavor = "current_thread")] | ||
| #[serial_test::serial] |
There was a problem hiding this comment.
Adding #[cfg(feature = "shadowsocks")] to the test function means this integration test crate will still compile when the feature is off, but the top-level imports (start_clash, wait_port_ready, PathBuf, etc.) will become unused and can fail CI if warnings are denied. Consider gating the entire file with #![cfg(feature = "shadowsocks")] or applying the same cfg to the relevant use items.
| #[cfg(feature = "shadowsocks")] | ||
| #[tokio::test(flavor = "current_thread")] | ||
| #[serial_test::serial] | ||
| async fn test_get_set_allow_lan() { |
There was a problem hiding this comment.
Same issue as smoke_tests.rs: with the test functions #[cfg(feature = "shadowsocks")], the crate-level imports at the top of this file can become unused when the feature is off. If your CI treats warnings as errors, this will break builds. Consider gating the entire file or the imports with the same cfg(feature = "shadowsocks").
| // Bind to port 0 to get OS-assigned available ports | ||
| let udp_sock = UdpSocket::bind("127.0.0.1:0").await?; | ||
| let udp_addr = udp_sock.local_addr()?; | ||
| drop(udp_sock); | ||
|
|
||
| let tcp_sock = TcpListener::bind("127.0.0.1:0").await?; | ||
| let tcp_addr = tcp_sock.local_addr()?; | ||
| drop(tcp_sock); | ||
|
|
||
| let dot_sock = TcpListener::bind("127.0.0.1:0").await?; | ||
| let dot_addr = dot_sock.local_addr()?; | ||
| drop(dot_sock); | ||
|
|
||
| let doh_sock = TcpListener::bind("127.0.0.1:0").await?; | ||
| let doh_addr = doh_sock.local_addr()?; | ||
| drop(doh_sock); | ||
|
|
||
| let doh3_sock = UdpSocket::bind("127.0.0.1:0").await?; | ||
| let doh3_addr = doh3_sock.local_addr()?; | ||
| drop(doh3_sock); |
There was a problem hiding this comment.
The test reserves ephemeral ports by binding to 127.0.0.1:0, reading local_addr(), and then immediately dropping the sockets before starting the real listeners. This introduces a race where another process/test can grab the port in between, leading to flaky failures. Prefer letting get_dns_listener bind to port 0 itself (and then reading back the chosen ports), or keep the sockets/listeners open and pass them into the server if the API allows.
🤔 This is a ...
🔗 Related issue link
💡 Background and solution
While implementing vless + reality, during unit testing, I discovered a bunch of tests that were causing my tests to fail. Although these weren't issues with the functionality I was implementing, my perfectionism drove me to fix these erroneous tests.
📝 Changelog
☑️ Self-Check before Merge
Summary
This PR fixes all unit tests to pass on macOS with Apple Silicon (M-series chips).
Previously, running
CLASH_DOCKER_TEST=1 cross test --workspace --exclude clash-ffi -F "plus"would fail on macOS ARM64 due to several Docker networking differences and platform-specific issues.What's Changed
Docker Test Infrastructure
The biggest difference between Linux and macOS Docker is the network mode. Linux can use
--network=hostwhich makes everything simple - containers share the host network. But on macOS (Docker Desktop), host mode doesn'twork, so containers run in bridge mode with their own IPs.
Changes:
container_ip()andgateway_ip()methods toDockerTestRunnerSOCKS5 Tests
The old socks5 image (
ghcr.io/wzshiming/socks5/socks5) had issues on ARM64. Switched tov2fly/v2fly-corewhich we already use for other tests. Added JSON config files for auth/noauth modes instead of relying on CLIargs.
Cross-Container Communication (ShadowTLS & OBFS)
These tests spin up multiple containers - one for the actual proxy server, another for the obfuscation layer. On Linux with host mode, they can talk via
127.0.0.1. On macOS, containers can't reach each other that way.Fixed by using
host.docker.internalwhich Docker Desktop provides for exactly this situation - it routes back to the host, which then forwards to the other container's mapped port.WireGuard UDP Checksum
This one was fun to debug. WireGuard tests were failing with checksum errors on macOS. Turns out some virtualization environments (including Docker Desktop's VM) can mess with UDP checksums due to checksum offloading or
NAT.
Since WireGuard already guarantees integrity via AEAD encryption, we can safely recalculate the UDP checksum for incoming IPv4 packets before handing them to smoltcp. It's a bit redundant but harmless, and it fixes the
issue.
Misc Fixes
initialize()calls to several tests (for proper logging setup)#[cfg(feature = "shadowsocks")]gates to tests that need itrules.yamlproxy_set_providertest to use socks5 instead of ss (simpler, no encryption config needed)Testing
All tests pass on macOS M4: