mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
86a2668fb0
Test env leakage, both directions. **Out** — six `argent-tools-client` suites pointed `HOME`/`USERPROFILE` at a temp dir, deleted the dir, and never restored, so a `--no-isolate` run handed every later file a home that no longer exists. Lifted the redirect into a shared `test/helpers/home-redirect.ts`; the three suites that already hand-rolled the save/restore moved onto it too. **In** — `packages/telemetry` had no `setupFiles` at all, so its unit tests read the developer's shell. Added `test/setup/clear-telemetry-env.ts` (ARGENT_ prefix sweep + `DO_NOT_TRACK` + the cloud-agent detector's markers), wired via `setupFiles`, following the `@argent/tool-server` precedent — and dropped the per-file guards it replaces. **Pinned, not just fixed** — a `test/setup/assert-env-restored.ts` in each of the three packages fails any suite that leaves a watched variable changed, so dropping a restore can no longer pass. Each watches `HOME` and `USERPROFILE`, plus `PATH` (`argent-tools-client`, `tool-server`) or `CI` and the five `OTEL_EXPORTER_OTLP_*` (`telemetry`) — the ones the sweep deliberately leaves ambient and individual tests then set by hand. Each of the three `clear-*-env` setup files has a pin test that re-imports it against planted sentinels; `packages/tool-server`'s existing copy gains the mixed-case one it was missing. `packages/tool-server` is in the diff because it has the same two gaps: it originated the prefix sweep (both pre-existing copies were case-sensitive, so they are fixed together) and it has the largest redirect surface of the three — 12 files hand-rolling the redirect plus 17 going through `scopeTempHome` — with no guard. It is already green under one; deleting `scopeTempHome`'s restore turns 17 files red. Two of its suites restored `HOME` by assigning back a saved value that can be `undefined`, writing the string `"undefined"`; the guard caught both on a host with no ambient `HOME`, and they are fixed here. `windows-e2e.yml` runs seven `tool-server` files through the config this branch edits, but neither that config nor `test/setup/**` was in its `paths:` filter — so the new setup file would have reached windows-latest unverified. Added both. No docs change: test infrastructure only, no user-facing behaviour. Fixes #1028 Fixes #839 Same-class problems found while reviewing this and filed rather than folded in: #1068 (`argent-cli` fails under `DO_NOT_TRACK=1`), #1069 (the `ios-device-runner` suites pin only `HOME`, so the redirect is inert on Windows — its second half, the restore that wrote the string `"undefined"`, is fixed here because the new guard caught it), #1070 (`useTempHome` shares one saved-`HOME` slot, so a nested `scopeHome()` restores a deleted dir), #1071 (`tool-server`'s sweep is prefix-only, so an exported `ADB_SERVER_SOCKET` reddens its suite), #1072 (`external-devices.test.ts` reads the developer's real `~/.argent/providers`) and #1073 (`flow-script-environment.test.ts` fails with no ambient `HOME`). <details> <summary>#1028 — probe results, six files</summary> `test/zz-home-probe.test.ts` from the issue body, run per suite. Vitest's sequencer orders by cached duration and falls back to file size, so the cache is cleared before each run to keep the tiny probe last: ``` rm -rf node_modules/.vite/vitest ../../node_modules/.vite/vitest npx vitest run test/<suite>.test.ts test/zz-home-probe.test.ts --no-isolate --no-file-parallelism ``` | suite | before | after | | --- | --- | --- | | `launcher-state` | 1 failed \| 23 passed — `argent-state-test-… exists=false` | 24 passed | | `launcher-spawn` | 1 failed \| 8 passed — `argent-spawn-test-… exists=false` | 9 passed | | `launcher-version-gate` | 1 failed \| 7 passed — `argent-version-gate-test-… exists=false` | 8 passed | | `launcher-duplicate-spawn` | 1 failed \| 10 passed — `argent-dup-test-… exists=false` | 11 passed | | `launcher-exit` | 1 failed \| 1 passed — `argent-exit-test-… exists=false` | 2 passed | | `link-config` | 1 failed \| 43 passed — `argent-link-config-test-… exists=false` | 44 passed | | `launcher-sweep` (control, fixed in #713) | passed | passed | | `launcher-ps-guard` (control, fixed in #713) | passed | passed | The probe itself is a diagnostic and is not part of the diff — `test/setup/assert-env-restored.ts` is the durable guard, and it needs no `--no-isolate` to fire. </details> <details> <summary>#839 — the stated repro is stale; here is what is actually left</summary> The issue was filed 2026-08-18. #714 landed 2026-08-19 and hand-enumerated the variables inside `index.test.ts` (a `CONSENT_ENV_KEYS` `beforeEach`) and `base-props.test.ts` (eight `delete`s). On `aa90873b5` all three named repros are green: ``` $ DO_NOT_TRACK=1 npx vitest run --root packages/telemetry → 319 passed $ ARGENT_TELEMETRY=0 npx vitest run --root packages/telemetry → 319 passed $ CURSOR_AGENT_WORKER_ID=x npx vitest run --root packages/telemetry → 319 passed ``` What remains is the structural gap the issue names: the protection is per-file and per-variable, nothing pins it, and `vitest.config.ts` still has no `setupFiles`. This PR moves the property to the setup file and deletes the two hand-rolled guards, so the setup file is what holds it — see the mutation section, where removing the `setupFiles` line reproduces the issue's 17 and 1 exactly. Post-change sweep, one hostile value per run, 329 tests each: `DO_NOT_TRACK=1`, `ARGENT_TELEMETRY=0`, `ARGENT_TELEMETRY_DEBUG=1`, `CLAUDE_CODE_ENVIRONMENT_KIND=byoc`, `CLAUDE_CODE_ENTRYPOINT=remote`, `CLAUDE_CODE_REMOTE_SESSION_ID=x`, `CURSOR_AGENT_WORKER_ID=x`, `CURSOR_WORKER_POOL_NAME=x`, `GITHUB_ACTIONS=true`, `GITHUB_ACTOR=copilot`, `GITHUB_WORKFLOW_REF=…copilot-swe-agent.yml@refs/heads/main`, `REPLIT_AGENT=1`, `ARGENT_HOST`/`ARGENT_PORT`/`ARGENT_AUTH_TOKEN`, `CI=1`, `CI=false`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_HEADERS`, the real CI combination, and all eleven at once — 329 passed every time. `GITHUB_ACTIONS` is a ci-info vendor input as well as a copilot-agent marker, so clearing it does move `isCi()` — but only its GitHub Actions branch, and an Actions run also sets `CI`, which `isCi()` answers ahead of any vendor check. `GITHUB_ACTOR` and `GITHUB_WORKFLOW_REF` appear in neither `ci-info/vendors.json` nor `ci-detect.ts`'s generic list, so they cost nothing. The rest of the ci-info surface and `OTEL_*` are deliberately left alone; the setup file says so. </details> <details> <summary>Mutation testing — every guard, applied by hand, verified to fail, reverted</summary> | mutation | result | | --- | --- | | drop `setupFiles` from `packages/telemetry/vitest.config.ts` | `DO_NOT_TRACK=1` → 19 failed (**17 in `index.test.ts`**, matching the issue, + the two registration pins) | | drop `CURSOR_WORKER_POOL_NAME` from `CLEARED_ENV_VARS` | drift test red | | move an `env.` read into `src/detect/vendor.ts` | drift test red, naming `detect/vendor.ts` | | rewrite a read as `env["X"]`, `const { X } = env`, `const { X }: NodeJS.ProcessEnv = env` or `const { X = "d" } = env` | drift test red | | add `cloud-agent-detect.ts` to `UNSWEPT_SRC_FILES` | drift test red | | revert `toUpperCase()` in any of the three sweeps | that package's mixed-case case red | | drop `restoreHome()` from one suite / from all nine | 1 file / 9 files red, each naming its deleted dir | | drop `scopeTempHome()`'s restore in `tool-server` | 17 files red | | drop the hand-rolled `CI` restore in `base-props.test.ts` | red, naming that file (green before; under `--no-isolate`, 7 unrelated failures in `otel-endpoint.test.ts`) | | narrow either guard's filter to variables that were already set | `assert-env-restored.test.ts` red | | replace the leaked half of the guard's message with a constant | `assert-env-restored.test.ts` red | | rename one function's `env` parameter in `cloud-agent-detect.ts` | drift test red — the read is found under the new binding | | narrow any guard's filter to a subset of `SCOPED_ENV_VARS` | `assert-env-restored.test.ts` red — the pin plants every entry | | drop the hand-rolled OTLP restore in `otel-endpoint-live.test.ts` | red, naming all four leaked variables | | move the guard's `AMBIENT` snapshot inside `assertEnvRestored` | `assert-env-restored.test.ts` red in all three packages | | delete the guard's `afterAll(...)` registration | `assert-env-restored.test.ts` red in all three packages | | drop `restoreHome()` from `artifacts.test.ts`'s two `afterEach` | red — the file-level guard catches a per-test omission | | drop ps-guard's `PATH` restore | red, naming the file | | empty `leakedEnvVars`, or drop a name from `SCOPED_ENV_VARS` | `assert-env-restored.test.ts` red | | replace the helper's delete branch with a plain assignment | `home-redirect.test.ts` red | | drop `USERPROFILE` from the helper | `home-redirect.test.ts` red | </details> <details> <summary>Local CI parity</summary> ``` npx vitest run --root packages/argent-tools-client 213 passed | 1 skipped npx vitest run --root packages/argent-tools-client --no-isolate 213 passed | 1 skipped npx vitest run --root packages/telemetry 329 passed npx vitest run --root packages/telemetry --no-isolate 329 passed npx vitest run --root packages/tool-server 6166 passed | 1 skipped npx tsc --noEmit -p packages/{argent-tools-client,telemetry,tool-server}/tsconfig.test.json clean npm run format / npm run lint / npm run knip clean ``` </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved test isolation by consistently restoring temporary environment settings after test runs. - Added safeguards that detect leaked environment settings and flag incomplete cleanup. - Added coverage to ensure telemetry-related environment variables are cleared before tests and do not affect results. - Added cross-platform checks for environment variable handling, including mixed-case names. - Expanded validation for home-directory redirection and restoration behavior. - Added checks confirming environment cleanup runs in the correct order across test suites. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
194 lines
9.1 KiB
YAML
194 lines
9.1 KiB
YAML
name: Windows E2E
|
|
|
|
# Proves Argent runs on Windows. iOS Simulator is macOS-only (Apple), so
|
|
# "Argent on Windows" means the Android + Chromium host control plane.
|
|
#
|
|
# This job, windows-chromium, has the tool-server drive a real headless Chrome
|
|
# over CDP (discover → screenshot → describe → tap → observe the DOM change).
|
|
# Chromium control is pure host-side TypeScript, so a hosted Windows runner (no
|
|
# virtualization) verifies it fully. It also runs the cross-platform resolver
|
|
# unit tests natively on Windows and confirms adb.exe resolution.
|
|
#
|
|
# The simulator-server.exe itself (the binary the Android path spawns) lives in
|
|
# the private radon repo, so it can't be built here without a cross-repo token.
|
|
# Its Windows build + smoke-run is verified in radon CI instead — see the
|
|
# build_argent_windows job in radon's simulator-server-ci workflow (companion
|
|
# PR). WHPX is available on windows-latest (Server 2025), so an Android emulator
|
|
# does boot on a hosted runner — that is where the boot-device stdio fix was
|
|
# verified. The full Android emulator E2E still lives in radon CI alongside that
|
|
# binary rather than here, because this repo can't build simulator-server.exe
|
|
# without a cross-repo token; that path is therefore out of scope for this job.
|
|
|
|
on:
|
|
workflow_dispatch:
|
|
pull_request:
|
|
branches:
|
|
- main
|
|
paths:
|
|
- "packages/tool-server/src/utils/command-on-path.ts"
|
|
- "packages/tool-server/src/utils/android-binary.ts"
|
|
- "packages/tool-server/src/utils/adb.ts"
|
|
- "packages/tool-server/src/utils/check-deps.ts"
|
|
- "packages/tool-server/src/utils/vega-cli.ts"
|
|
- "packages/tool-server/src/utils/chromium-discovery.ts"
|
|
- "packages/tool-server/src/tools/simulator/stop-metro.ts"
|
|
- "packages/tool-server/src/tools/devices/boot-device.ts"
|
|
- "packages/tool-server/src/utils/device-shutdown.ts"
|
|
# The unit tests this job runs — editing one must re-run it here, and so
|
|
# must editing the config or a setup file every one of them loads.
|
|
- "packages/tool-server/vitest.config.ts"
|
|
- "packages/tool-server/test/setup/**"
|
|
- "packages/tool-server/test/command-on-path.test.ts"
|
|
- "packages/tool-server/test/android-binary-windows.test.ts"
|
|
- "packages/tool-server/test/adb-resolve-avd-path.test.ts"
|
|
- "packages/tool-server/test/check-deps.test.ts"
|
|
- "packages/tool-server/test/http-dep-gate.test.ts"
|
|
- "packages/tool-server/test/stop-metro-netstat.test.ts"
|
|
- "packages/tool-server/test/device-shutdown.test.ts"
|
|
- "packages/native-devtools-ios/src/index.ts"
|
|
- "packages/native-devtools-ios/test/index.test.ts"
|
|
- "packages/argent/scripts/bundle-tools.cjs"
|
|
- "packages/argent/scripts/argent-simulator-server.cjs"
|
|
- "scripts/dev.cjs"
|
|
- "scripts/download-simulator-server.sh"
|
|
- "scripts/ci/windows-chromium-e2e.mjs"
|
|
- "scripts/ci/e2e-chromium-page.html"
|
|
- ".github/workflows/windows-e2e.yml"
|
|
|
|
jobs:
|
|
windows-chromium:
|
|
name: Chromium control plane + resolver unit tests (Windows)
|
|
runs-on: windows-latest
|
|
timeout-minutes: 20
|
|
|
|
steps:
|
|
- name: Checkout
|
|
uses: actions/checkout@v4
|
|
|
|
- name: Set up Node.js
|
|
uses: actions/setup-node@v4
|
|
with:
|
|
node-version: "20"
|
|
|
|
- name: Install dependencies
|
|
run: npm ci
|
|
|
|
- name: Fix rollup native bindings (win32)
|
|
# vitest 4 transforms via rollup, whose platform binary is an optional
|
|
# dep npm ci sometimes skips; install the win32 one explicitly.
|
|
run: npm install @rollup/rollup-win32-x64-msvc --no-save
|
|
|
|
- name: Build workspace
|
|
run: npm run build
|
|
|
|
- name: Resolver unit tests on Windows (tool-server)
|
|
# The win32 branches added for Windows support, exercised on a real
|
|
# Windows host (real `where.exe`, real `.exe` access semantics) rather
|
|
# than a mocked platform. The broader suite has POSIX-only fixtures, so
|
|
# this is the curated cross-platform subset.
|
|
working-directory: packages/tool-server
|
|
run: >
|
|
npx vitest run
|
|
test/command-on-path.test.ts
|
|
test/android-binary-windows.test.ts
|
|
test/adb-resolve-avd-path.test.ts
|
|
test/check-deps.test.ts
|
|
test/http-dep-gate.test.ts
|
|
test/stop-metro-netstat.test.ts
|
|
test/device-shutdown.test.ts
|
|
|
|
- name: Resolver unit tests on Windows (native-devtools-ios)
|
|
working-directory: packages/native-devtools-ios
|
|
run: npx vitest run
|
|
|
|
- name: adb.exe resolves on Windows
|
|
# windows-latest ships the Android SDK with ANDROID_HOME/ANDROID_SDK_ROOT
|
|
# set but does NOT put platform-tools on PATH — exactly the gap the
|
|
# Windows resolver closes (`.exe` suffix + SDK-root fallback). Resolve
|
|
# adb the way the tool-server does, then run the *resolved* path (a bare
|
|
# `adb` wouldn't be found, which is the whole point).
|
|
shell: pwsh
|
|
run: |
|
|
$adb = node -e "require('./packages/tool-server/dist/utils/android-binary.js').resolveAndroidBinary('adb').then(p => process.stdout.write(p || ''))"
|
|
Write-Host "resolved adb -> $adb"
|
|
if ([string]::IsNullOrWhiteSpace($adb) -or -not $adb.ToLower().EndsWith('adb.exe')) {
|
|
Write-Error "expected resolveAndroidBinary('adb') to return an adb.exe path"
|
|
exit 1
|
|
}
|
|
if (-not (Test-Path $adb)) { Write-Error "resolved adb path does not exist: $adb"; exit 1 }
|
|
& $adb version
|
|
|
|
- name: Locate Chrome
|
|
id: chrome
|
|
run: |
|
|
$candidates = @(
|
|
"$env:ProgramFiles\Google\Chrome\Application\chrome.exe",
|
|
"${env:ProgramFiles(x86)}\Google\Chrome\Application\chrome.exe",
|
|
"$env:LOCALAPPDATA\Google\Chrome\Application\chrome.exe"
|
|
)
|
|
$chrome = $candidates | Where-Object { Test-Path $_ } | Select-Object -First 1
|
|
if (-not $chrome) { Write-Error "Google Chrome not found on this runner"; exit 1 }
|
|
Write-Host "Chrome: $chrome"
|
|
"path=$chrome" >> $env:GITHUB_OUTPUT
|
|
|
|
# Launch Chrome + the tool-server AND run the E2E in one step. On Windows
|
|
# runners, `Start-Process` background processes don't reliably survive into
|
|
# a later step (unlike `nohup` on Linux), so both must stay in scope for
|
|
# the duration of the E2E — hence a single step rather than three.
|
|
- name: Launch Chrome + tool-server and run Chromium E2E
|
|
env:
|
|
ARGENT_E2E_URL: http://127.0.0.1:3033
|
|
run: |
|
|
$page = "file:///" + ((Resolve-Path scripts/ci/e2e-chromium-page.html).Path -replace '\\','/')
|
|
$udd = Join-Path $env:RUNNER_TEMP "chrome-e2e-profile"
|
|
$chromeLog = Join-Path $env:RUNNER_TEMP "chrome.log"
|
|
Start-Process -FilePath "${{ steps.chrome.outputs.path }}" -ArgumentList @(
|
|
"--headless=new","--remote-debugging-port=9222","--user-data-dir=$udd",
|
|
"--no-first-run","--no-default-browser-check","--disable-gpu",$page
|
|
) -RedirectStandardOutput $chromeLog -RedirectStandardError "$chromeLog.err" | Out-Null
|
|
$cdpUp = $false
|
|
for ($i = 0; $i -lt 30; $i++) {
|
|
try {
|
|
$r = Invoke-WebRequest -UseBasicParsing "http://127.0.0.1:9222/json/version" -TimeoutSec 2
|
|
if ($r.StatusCode -eq 200) { Write-Host "CDP up at t+${i}s"; $cdpUp = $true; break }
|
|
} catch {}
|
|
Start-Sleep -Seconds 1
|
|
}
|
|
if (-not $cdpUp) { Get-Content "$chromeLog.err" -ErrorAction SilentlyContinue; Write-Error "Chrome CDP never came up on :9222"; exit 1 }
|
|
|
|
$env:ARGENT_PORT = "3033"
|
|
New-Item -ItemType Directory -Force -Path "$env:USERPROFILE\.argent" | Out-Null
|
|
$tsLog = Join-Path $env:RUNNER_TEMP "tool-server.log"
|
|
Start-Process -FilePath "node" -ArgumentList @("packages/tool-server/dist/index.js","start") `
|
|
-RedirectStandardOutput $tsLog -RedirectStandardError "$tsLog.err" | Out-Null
|
|
$tsUp = $false
|
|
for ($i = 0; $i -lt 40; $i++) {
|
|
try {
|
|
$r = Invoke-WebRequest -UseBasicParsing "http://127.0.0.1:3033/tools" -TimeoutSec 2
|
|
if ($r.StatusCode -eq 200) { Write-Host "tool-server up at t+${i}s"; $tsUp = $true; break }
|
|
} catch {}
|
|
Start-Sleep -Seconds 1
|
|
}
|
|
if (-not $tsUp) {
|
|
Write-Host "--- tool-server.log ---"; Get-Content $tsLog -ErrorAction SilentlyContinue
|
|
Write-Host "--- tool-server.err ---"; Get-Content "$tsLog.err" -ErrorAction SilentlyContinue
|
|
Write-Error "tool-server never came up on :3033"; exit 1
|
|
}
|
|
|
|
$env:ARGENT_E2E_OUT = Join-Path $env:RUNNER_TEMP "e2e"
|
|
node scripts/ci/windows-chromium-e2e.mjs
|
|
|
|
- name: Upload artifacts
|
|
if: always()
|
|
uses: actions/upload-artifact@v4
|
|
with:
|
|
name: windows-chromium-e2e
|
|
path: |
|
|
${{ runner.temp }}/e2e/*.png
|
|
${{ runner.temp }}/tool-server.log
|
|
${{ runner.temp }}/tool-server.log.err
|
|
${{ runner.temp }}/chrome.log
|
|
${{ runner.temp }}/chrome.log.err
|
|
if-no-files-found: warn
|
|
retention-days: 7
|