Add macOS connect smoke test#8898
Open
atavism wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a macOS VPN connect/disconnect smoke path: native smoke command handling, app and Xcode wiring, a CI smoke script and workflow inputs, and a Flutter integration test with supporting UI keys and traffic verification. ChangesmacOS Connect/Disconnect Smoke Path
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in macOS connect/disconnect smoke test that exercises the real VPN System Extension flow, including a hidden macOS app command for extension status/activation, Flutter integration-test hooks, and CI wiring to run the suite and upload diagnostics.
Changes:
- Introduces a macOS “smoke command” CLI entry point (status/activate with timeout) and runner that reuses the existing
SystemExtensionManager. - Adds a macOS integration test (
macos_connect_smoke_test.dart) and UI debug keys to reliably detect System Extension readiness/failure states. - Adds an opt-in CI path (workflows + script) to run the macOS smoke suite and upload diagnostics artifacts.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| macos/RunnerTests/RunnerTests.swift | Adds unit tests covering smoke-command parsing and structured output/exit codes. |
| macos/Runner/VPN/SystemExtensionSmokeCommand.swift | Adds CLI parsing, JSON output helpers, and a runner around SystemExtensionManager status. |
| macos/Runner/VPN/SystemExtensionManager.swift | Adds Combine import to support @Published usage consumed by the smoke runner. |
| macos/Runner/AppDelegate.swift | Adds early smoke-command handling at launch (headless accessory mode + exit codes). |
| macos/Runner.xcodeproj/project.pbxproj | Wires the new Swift source file into the macOS target build. |
| lib/features/macos_extension/macos_extension_dialog.dart | Adds stable widget keys for integration tests to observe extension UI/status. |
| integration_test/vpn/vpn_smoke_helpers.dart | Includes macos_extension.* keys in debug-key collection for diagnostics. |
| integration_test/vpn/macos_connect_smoke_test.dart | Adds a macOS VPN connect/disconnect integration smoke test with extension readiness gating. |
| integration_test/vpn/connect_smoke_harness.dart | Adds requireTrafficAfterConnect option to force a post-connect traffic check. |
| .github/workflows/build-macos.yml | Adds inputs and an opt-in step to run the macOS smoke suite + upload diagnostics. |
| .github/workflows/app-smoke-tests.yml | Adds a macOS platform option and opt-in macOS smoke inclusion for “all”. |
| .github/scripts/macos_smoke_suite.sh | Adds the macOS smoke suite runner script (preflight sys-ext activation + Flutter test + diagnostics). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
macos/RunnerTests/RunnerTests.swift (2)
594-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing timeout parsing edge-case tests
The error-case tests cover conflicting flags, non-positive values, and cross-form duplicates. Missing cases that exercise distinct code paths in
parseTimeout:
--timeout-secondsas the last argument (no value) → should fail with "needs a value"- Non-numeric value (e.g.,
--timeout-seconds abc) → should fail with "positive number"- Same-form duplicates:
--timeout-seconds 30 --timeout-seconds 45(space-space) and--timeout-seconds=30 --timeout-seconds=45(equals-equals)🧪 Proposed additional test cases
switch SystemExtensionSmokeCommand.parse( arguments: [ "Lantern", "--smoke-system-extension-status", "--timeout-seconds", "30", "--timeout-seconds=45", ] ) { case .failure(let error): XCTAssertTrue(error.message.contains("can only be set once")) default: XCTFail("Expected duplicate timeout arguments to fail") } + + // Missing value (flag is last argument) + switch SystemExtensionSmokeCommand.parse( + arguments: ["Lantern", "--smoke-system-extension-status", "--timeout-seconds"] + ) { + case .failure(let error): + XCTAssertTrue(error.message.contains("needs a value")) + default: + XCTFail("Expected missing timeout value to fail") + } + + // Non-numeric value + switch SystemExtensionSmokeCommand.parse( + arguments: ["Lantern", "--smoke-system-extension-status", "--timeout-seconds", "abc"] + ) { + case .failure(let error): + XCTAssertTrue(error.message.contains("positive number")) + default: + XCTFail("Expected non-numeric timeout to fail") + } + + // Same-form duplicate (space-separated) + switch SystemExtensionSmokeCommand.parse( + arguments: [ + "Lantern", + "--smoke-system-extension-status", + "--timeout-seconds", + "30", + "--timeout-seconds", + "45", + ] + ) { + case .failure(let error): + XCTAssertTrue(error.message.contains("can only be set once")) + default: + XCTFail("Expected duplicate space-form timeout to fail") + } + + // Same-form duplicate (equals-separated) + switch SystemExtensionSmokeCommand.parse( + arguments: [ + "Lantern", + "--smoke-system-extension-status", + "--timeout-seconds=30", + "--timeout-seconds=45", + ] + ) { + case .failure(let error): + XCTAssertTrue(error.message.contains("can only be set once")) + default: + XCTFail("Expected duplicate equals-form timeout to fail") + } }🤖 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 `@macos/RunnerTests/RunnerTests.swift` around lines 594 - 631, The timeout parsing tests in SystemExtensionSmokeCommand are missing edge cases that exercise distinct parseTimeout paths. Extend testSystemExtensionSmokeCommandRejectsInvalidArguments to cover a trailing --timeout-seconds with no value, a non-numeric timeout value, and same-form duplicate timeout arguments for both space-separated and equals-separated styles. Verify each case matches the expected failure message so parseTimeout’s validation and duplicate detection are fully covered.
633-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing more exit-code mappings and the
timeoutSecondsJSON fieldThe exit-code tests cover
.activated(0),.requiresApproval(20),.requiresReboot(21), and.timedOut(124) for the activate action. Missing:.installed(0),.error(1), and status-action exit codes. Additionally, the JSON test doesn't verify thetimeoutSecondsfield that's included whenstatus == .timedOut.🧪 Proposed additional assertions
XCTAssertEqual( SystemExtensionSmokeResult( action: .activate, status: .timedOut, timeout: 120 ).exitCode, 124 ) + + XCTAssertEqual( + SystemExtensionSmokeResult( + action: .activate, + status: .installed, + timeout: 120 + ).exitCode, + 0 + ) + + XCTAssertEqual( + SystemExtensionSmokeResult( + action: .activate, + status: .error, + timeout: 120 + ).exitCode, + 1 + ) + + // Status action exit codes + XCTAssertEqual( + SystemExtensionSmokeResult( + action: .status, + status: .notInstalled, + timeout: 120 + ).exitCode, + 0 + ) + + XCTAssertEqual( + SystemExtensionSmokeResult( + action: .status, + status: .error, + timeout: 120 + ).exitCode, + 1 + ) } + + func testSystemExtensionSmokeResultIncludesTimeoutSecondsWhenTimedOut() throws { + let result = SystemExtensionSmokeResult( + action: .activate, + status: .timedOut, + timeout: 45 + ) + + let data = try XCTUnwrap(result.jsonLine.data(using: .utf8)) + let payload = try XCTUnwrap( + JSONSerialization.jsonObject(with: data) as? [String: Any] + ) + + XCTAssertEqual(payload["timeoutSeconds"] as? Double, 45) + }🤖 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 `@macos/RunnerTests/RunnerTests.swift` around lines 633 - 666, The current `SystemExtensionSmokeResult` coverage only checks a subset of the `exitCode` mappings for `.activate`; extend the `testSystemExtensionSmokeResultUsesDistinctActivationExitCodes` style coverage to include the missing `.installed` and `.error` cases as well as the status/action-specific exit codes so the mapping is fully exercised. Also update the JSON serialization test for `SystemExtensionSmokeResult` to assert the `timeoutSeconds` field is present and correct when `status` is `.timedOut`, using the existing `SystemExtensionSmokeResult` and related status/action symbols to keep the assertions aligned with the model behavior..github/workflows/app-smoke-tests.yml (1)
114-127: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider explicit secret forwarding instead of
secrets: inherit.
secrets: inheritpasses all repository secrets tobuild-macos.yml. While the called workflow needs several Apple secrets, explicitly forwarding only the required secrets would follow least-privilege principles and prevent accidental exposure of unrelated secrets if the called workflow is modified in the future.🤖 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 @.github/workflows/app-smoke-tests.yml around lines 114 - 127, The macOS job in app-smoke-tests is using broad secret inheritance, which gives the called build-macos workflow access to every repository secret. Update the macos job to forward only the specific secrets required by build-macos.yml instead of using secrets: inherit, keeping the existing with inputs and the workflow call intact while applying least-privilege secret handling.Source: Linters/SAST tools
🤖 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 `@macos/Runner/VPN/SystemExtensionSmokeCommand.swift`:
- Around line 198-201: The smoke command can finish too early because
SystemExtensionSmokeCommand.handle(manager.status) is being called whenever
manager.initialized is already true, even if that status came from a cached
singleton query. Update the command submission flow in the logic around
commandSubmitted and handle(_:), and only complete based on the request-specific
result from checkInstallationStatus() or activateExtension() rather than the
current manager.status. Make sure the active request path waits for the fresh
status/activation callback before calling the completion logic so .status and
.activate cannot resolve from stale state.
---
Nitpick comments:
In @.github/workflows/app-smoke-tests.yml:
- Around line 114-127: The macOS job in app-smoke-tests is using broad secret
inheritance, which gives the called build-macos workflow access to every
repository secret. Update the macos job to forward only the specific secrets
required by build-macos.yml instead of using secrets: inherit, keeping the
existing with inputs and the workflow call intact while applying least-privilege
secret handling.
In `@macos/RunnerTests/RunnerTests.swift`:
- Around line 594-631: The timeout parsing tests in SystemExtensionSmokeCommand
are missing edge cases that exercise distinct parseTimeout paths. Extend
testSystemExtensionSmokeCommandRejectsInvalidArguments to cover a trailing
--timeout-seconds with no value, a non-numeric timeout value, and same-form
duplicate timeout arguments for both space-separated and equals-separated
styles. Verify each case matches the expected failure message so parseTimeout’s
validation and duplicate detection are fully covered.
- Around line 633-666: The current `SystemExtensionSmokeResult` coverage only
checks a subset of the `exitCode` mappings for `.activate`; extend the
`testSystemExtensionSmokeResultUsesDistinctActivationExitCodes` style coverage
to include the missing `.installed` and `.error` cases as well as the
status/action-specific exit codes so the mapping is fully exercised. Also update
the JSON serialization test for `SystemExtensionSmokeResult` to assert the
`timeoutSeconds` field is present and correct when `status` is `.timedOut`,
using the existing `SystemExtensionSmokeResult` and related status/action
symbols to keep the assertions aligned with the model behavior.
🪄 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: 315c1b6b-aee5-4075-b30c-27ba785254d7
📒 Files selected for processing (12)
.github/scripts/macos_smoke_suite.sh.github/workflows/app-smoke-tests.yml.github/workflows/build-macos.ymlintegration_test/vpn/connect_smoke_harness.dartintegration_test/vpn/macos_connect_smoke_test.dartintegration_test/vpn/vpn_smoke_helpers.dartlib/features/macos_extension/macos_extension_dialog.dartmacos/Runner.xcodeproj/project.pbxprojmacos/Runner/AppDelegate.swiftmacos/Runner/VPN/SystemExtensionManager.swiftmacos/Runner/VPN/SystemExtensionSmokeCommand.swiftmacos/RunnerTests/RunnerTests.swift
* split macos smoke onto self-hosted runner * code review updates
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/macos-connect-smoke.yml (1)
43-148: 🩺 Stability & Availability | 🔵 TrivialConsider adding a concurrency group for the self-hosted runner.
Multiple dispatches of this workflow could overlap on the single self-hosted runner labeled
lantern-macos-smoke, leading to resource contention or flaky failures. A concurrency group keyed on the workflow reference would queue or cancel redundant runs.💡 Suggested concurrency block
+jobs: + macos-connect-smoke: + concurrency: + group: macos-connect-smoke-${{ github.ref }} + cancel-in-progress: false permissions: contents: read🤖 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 @.github/workflows/macos-connect-smoke.yml around lines 43 - 148, Add a concurrency control to the macOS smoke workflow so overlapping runs on the single self-hosted runner labeled lantern-macos-smoke do not contend for resources. Update the macos-connect-smoke job in the workflow to include a concurrency group keyed on the workflow reference (or equivalent unique run scope), and choose cancel-in-progress or queueing behavior as appropriate. Keep the change localized to the job definition alongside runs-on and timeout-minutes.
🤖 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 @.github/workflows/macos-connect-smoke.yml:
- Around line 60-61: The Checkout repository step using actions/checkout@v4
should explicitly disable GitHub token persistence. Update the checkout
configuration in the macOS smoke workflow to set persist-credentials to false,
matching the existing prepare job pattern in app-smoke-tests.yml, so the smoke
suite and make targets do not leave credentials in .git/config.
---
Nitpick comments:
In @.github/workflows/macos-connect-smoke.yml:
- Around line 43-148: Add a concurrency control to the macOS smoke workflow so
overlapping runs on the single self-hosted runner labeled lantern-macos-smoke do
not contend for resources. Update the macos-connect-smoke job in the workflow to
include a concurrency group keyed on the workflow reference (or equivalent
unique run scope), and choose cancel-in-progress or queueing behavior as
appropriate. Keep the change localized to the job definition alongside runs-on
and timeout-minutes.
🪄 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: 70022213-bee5-4b2b-a2b3-ae5082f1ec08
📒 Files selected for processing (4)
.github/scripts/macos_smoke_suite.sh.github/workflows/app-smoke-tests.yml.github/workflows/build-macos.yml.github/workflows/macos-connect-smoke.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/scripts/macos_smoke_suite.sh
Comment on lines
+60
to
+61
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on the checkout step.
The actions/checkout@v4 action persists the GitHub token in .git/config by default. Since this workflow runs the smoke suite script and make targets that could inadvertently expose credentials, explicitly disable credential persistence — consistent with the prepare job in app-smoke-tests.yml (line 76).
🔒️ Proposed fix
- name: Checkout repository
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 60-61: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/macos-connect-smoke.yml around lines 60 - 61, The Checkout
repository step using actions/checkout@v4 should explicitly disable GitHub token
persistence. Update the checkout configuration in the macOS smoke workflow to
set persist-credentials to false, matching the existing prepare job pattern in
app-smoke-tests.yml, so the smoke suite and make targets do not leave
credentials in .git/config.
Source: Linters/SAST tools
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
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.
Resolves https://github.com/getlantern/engineering/issues/3670
Adds macOS connect test for the real VPN/System Extension flow: hidden app commands for extension status/activation, Flutter integration-test keys, a macOS connect smoke test, and manual CI path
Summary by CodeRabbit