Repository navigation
ignore - #3408
Open
seanmcguire12 wants to merge 4 commits into
Open
ignore#3408seanmcguire12 wants to merge 4 commits into
seanmcguire12 wants to merge 4 commits into
Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 15 files
Confidence score: 3/5
- The Windows command fixtures in
cli-functions-contract.test.ts,cli-templates.test.ts, andnode-executable.tswon't launch becausespawnSynccan't execute.cmdshims without a shell. Add cmd-aware launching or an executable shim. - In
skills-install.test.ts, killingcmd.exeleaves the Node child holding stdout and stderr, sospawnPassthroughcan exceed the new 10-second limit. Ensure the child is terminated too.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/tests/skills-install.test.ts">
<violation number="1" location="packages/cli/tests/skills-install.test.ts:206">
P2: On Windows, killing `cmd.exe` leaves this Node child alive with inherited stdout/stderr handles, so `spawnPassthrough` does not close until the 15-second fallback fires. The new `<10s` assertion therefore fails in the Windows CI job; close the stub’s output streams before starting the fallback timer.</violation>
</file>
<file name="packages/cli/tests/cli-templates.test.ts">
<violation number="1" location="packages/cli/tests/cli-templates.test.ts:273">
P2: These clone tests now run on Windows, but the scaffolder cannot launch the `npx.cmd`/`uvx.cmd` fixtures with `spawnSync` and no shell, so the tests fail before reaching their assertions. Add Windows `.cmd` handling to the scaffolder before enabling these cases.</violation>
</file>
<file name="packages/cli/tests/helpers/node-executable.ts">
<violation number="1" location="packages/cli/tests/helpers/node-executable.ts:18">
P2: The Windows `.cmd` shim cannot run in the functions tests that call `spawnSync("npm"/"pnpm", ...)` without a shell. Make those invocations cmd-aware or provide a launcher that direct spawning can execute, so these Windows tests reach the fake package manager.</violation>
</file>
<file name="packages/cli/tests/cli-functions-contract.test.ts">
<violation number="1" location="packages/cli/tests/cli-functions-contract.test.ts:122">
P1: This test now runs on Windows, but the CLI invokes `npm` with `spawnSync` and no shell while the fixture provides only `npm.cmd` there. Node cannot directly launch `.cmd` files, so the fake never runs and this test fails; make the CLI launch Windows package-manager shims safely before enabling it.</violation>
</file>
Architecture diagram
sequenceDiagram
participant GH as GitHub Actions
participant WF as Workflow (ci/baseline)
participant NPM as pnpm/turbo
participant CLI as Browse CLI
participant DAEMON as Browse Daemon
participant CHROME as Headless Chrome
participant FS as Filesystem
participant VITEST as Vitest Runner
Note over GH,WF: CI dispatch (Linux + NEW Windows baseline)
GH->>WF: Trigger workflow (PR paths / dispatch)
WF->>WF: Checkout & setup-node-pnpm
WF->>NPM: turbo build --filter=browse
NPM-->>WF: Build artifacts
alt Linux smoke job (ci.yml)
WF->>NPM: pnpm --filter browse test:smoke
NPM->>VITEST: Run vitest.smoke.config.ts
VITEST->>CLI: Spawn CLI process (runCli)
else Windows baseline (browse-windows-baseline.yml)
WF->>NPM: pnpm --filter browse test:cli
NPM->>VITEST: Run CLI contract tests
VITEST->>CLI: Spawn CLI with Node-based stubs
end
rect rgb(240, 248, 255)
Note over CLI,CHROME: Smoke Test Flow (NEW)
VITEST->>FS: Create temp dir + fixture server
VITEST->>CLI: browse open <url> --local --headless
CLI->>DAEMON: Start daemon (managed-local mode)
DAEMON->>CHROME: Launch headless Chrome
CHROME-->>DAEMON: Page loaded (title/URL)
DAEMON-->>CLI: OpenResult JSON
CLI-->>VITEST: stdout JSON
VITEST->>CLI: browse snapshot
CLI->>DAEMON: Get accessibility tree
DAEMON-->>CLI: Snapshot refs
CLI-->>VITEST: Tree JSON
VITEST->>CLI: browse fill #name "Browse user"
CLI->>DAEMON: Fill input
DAEMON->>CHROME: Set value
DAEMON-->>CLI: filled: true
VITEST->>CLI: browse click @ref (snapshot ref)
CLI->>DAEMON: Click element
DAEMON->>CHROME: Dispatch click
CHROME-->>DAEMON: DOM updated
DAEMON-->>CLI: clicked: true
VITEST->>CLI: browse get text #result
CLI->>DAEMON: Query DOM
DAEMON-->>CLI: "Hello, Browse user"
VITEST->>CLI: browse stop
CLI->>DAEMON: Stop daemon + browser
alt Restart same session
VITEST->>CLI: browse open (same session id)
CLI->>DAEMON: New daemon PIDs
DAEMON->>CHROME: Relaunch
VITEST->>CLI: browse get value #name -> empty
end
end
rect rgb(255, 248, 240)
Note over VITEST,FS: Test Fixture Portability (NEW writeNodeExecutable)
VITEST->>FS: Write .cjs + launcher (.cmd on win32 / shell on POSIX)
VITEST->>CLI: Spawn CLI with PATH=stubDir
CLI->>FS: External command (npx/npm/pnpm stub)
FS-->>CLI: Command output/exit code
alt Windows npm pack test
VITEST->>NPM: cmd.exe /c npm pack --dry-run
NPM-->>VITEST: Pack manifest JSON
end
end
WF->>WF: Summarize outcomes + upload artifacts
WF-->>GH: Job summary (new step output)
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| }); | ||
|
|
||
| itPosix("generates package-lock.json without registry resolved URLs", async () => { | ||
| it("generates package-lock.json without registry resolved URLs", async () => { |
Contributor
There was a problem hiding this comment.
P1: This test now runs on Windows, but the CLI invokes npm with spawnSync and no shell while the fixture provides only npm.cmd there. Node cannot directly launch .cmd files, so the fake never runs and this test fails; make the CLI launch Windows package-manager shims safely before enabling it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/cli/tests/cli-functions-contract.test.ts, line 122:
<comment>This test now runs on Windows, but the CLI invokes `npm` with `spawnSync` and no shell while the fixture provides only `npm.cmd` there. Node cannot directly launch `.cmd` files, so the fake never runs and this test fails; make the CLI launch Windows package-manager shims safely before enabling it.</comment>
<file context>
@@ -117,12 +119,16 @@ describe("functions API contracts", () => {
});
- itPosix("generates package-lock.json without registry resolved URLs", async () => {
+ it("generates package-lock.json without registry resolved URLs", async () => {
const cwd = await createFunctionFixture("functions-publish-lockgen-");
const argsLog = join(cwd, "npm-args.log");
</file context>
| // Exceed the install deadline, but bound the fixture's lifetime even if a | ||
| // Windows shell exits before its Node child. The test must return before | ||
| // this fallback fires, so an orphaned child cannot make the timeout test pass. | ||
| await writeNodeExecutable(stubDir, "npx", "setTimeout(() => {}, 15_000);"); |
Contributor
There was a problem hiding this comment.
P2: On Windows, killing cmd.exe leaves this Node child alive with inherited stdout/stderr handles, so spawnPassthrough does not close until the 15-second fallback fires. The new <10s assertion therefore fails in the Windows CI job; close the stub’s output streams before starting the fallback timer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/cli/tests/skills-install.test.ts, line 206:
<comment>On Windows, killing `cmd.exe` leaves this Node child alive with inherited stdout/stderr handles, so `spawnPassthrough` does not close until the 15-second fallback fires. The new `<10s` assertion therefore fails in the Windows CI job; close the stub’s output streams before starting the fallback timer.</comment>
<file context>
@@ -187,19 +189,21 @@ async function createTempDir(prefix: string): Promise<string> {
+ // Exceed the install deadline, but bound the fixture's lifetime even if a
+ // Windows shell exits before its Node child. The test must return before
+ // this fallback fires, so an orphaned child cannot make the timeout test pass.
+ await writeNodeExecutable(stubDir, "npx", "setTimeout(() => {}, 15_000);");
}
</file context>
Suggested change
| await writeNodeExecutable(stubDir, "npx", "setTimeout(() => {}, 15_000);"); | |
| await writeNodeExecutable(stubDir, "npx", "process.stdout.destroy(); process.stderr.destroy(); setTimeout(() => {}, 15_000);"); |
|
|
||
| itPosix("clones TypeScript templates with create-browser-app via npx", async () => { | ||
| const stubDir = await createTempDir("browse-templates-ts-stub-"); | ||
| it("clones TypeScript templates with create-browser-app via npx", async () => { |
Contributor
There was a problem hiding this comment.
P2: These clone tests now run on Windows, but the scaffolder cannot launch the npx.cmd/uvx.cmd fixtures with spawnSync and no shell, so the tests fail before reaching their assertions. Add Windows .cmd handling to the scaffolder before enabling these cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/cli/tests/cli-templates.test.ts, line 273:
<comment>These clone tests now run on Windows, but the scaffolder cannot launch the `npx.cmd`/`uvx.cmd` fixtures with `spawnSync` and no shell, so the tests fail before reaching their assertions. Add Windows `.cmd` handling to the scaffolder before enabling these cases.</comment>
<file context>
@@ -270,21 +270,22 @@ describe("templates commands", () => {
- itPosix("clones TypeScript templates with create-browser-app via npx", async () => {
- const stubDir = await createTempDir("browse-templates-ts-stub-");
+ it("clones TypeScript templates with create-browser-app via npx", async () => {
+ const stubDir = await createTempDir("browse templates ts stub-");
const logPath = join(stubDir, "commands.log");
</file context>
| await writeFile(script, source + "\n"); | ||
| if (process.platform === "win32") { | ||
| await writeFile( | ||
| join(directory, name + ".cmd"), |
Contributor
There was a problem hiding this comment.
P2: The Windows .cmd shim cannot run in the functions tests that call spawnSync("npm"/"pnpm", ...) without a shell. Make those invocations cmd-aware or provide a launcher that direct spawning can execute, so these Windows tests reach the fake package manager.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/cli/tests/helpers/node-executable.ts, line 18:
<comment>The Windows `.cmd` shim cannot run in the functions tests that call `spawnSync("npm"/"pnpm", ...)` without a shell. Make those invocations cmd-aware or provide a launcher that direct spawning can execute, so these Windows tests reach the fake package manager.</comment>
<file context>
@@ -0,0 +1,39 @@
+ await writeFile(script, source + "\n");
+ if (process.platform === "win32") {
+ await writeFile(
+ join(directory, name + ".cmd"),
+ [
+ "@echo off",
</file context>
This branch has not been deployed
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.
Summary by cubic
Makes the Browse CLI test suite cross-platform and adds a real-Chrome smoke test.
writeNodeExecutable, removingitPosixguards that previously skipped tests on Windows.test:smoke, a vitest suite that runs Browse CLI operations against a real headless Chrome instance across separate processes.browse-smokeLinux job toci.yml, gated on changes to Browse-related files and paths.npm packthroughcmd.exeon Windows and excludes browser tests from the regular vitest config.Written for commit cb8d351. Summary will update on new commits.