Repository navigation
Conversation
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
fd543db to
a6de02a
Compare
ba3a8b0 to
3cbc0d2
Compare
a6de02a to
912433f
Compare
3cbc0d2 to
09c1e3e
Compare
09c1e3e to
d118ae1
Compare
912433f to
bb3b051
Compare
d118ae1 to
94dbe58
Compare
bb3b051 to
0f3e071
Compare
johncwaters
left a comment
johncwaters
left a comment
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
|
|
||
| expect(callOrder).toEqual(['shutdown', 'outroError']); | ||
| expect(process.exit).toHaveBeenCalledWith(1); | ||
| expect(callOrder).toEqual(['shutdown', 'outroError', 'exit']); |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
The test pushes 'exit' only after await ended() returns, and waitForOutroDismissed resolves at once, so the order is ['shutdown','outroError','exit'] even if wizardAbort called host.exit.end() before awaiting host.present(...). Nothing pins the contract in src/host/wizard-abort.ts that the host ends only after the presenter settles. Make waitForOutroDismissed a deferred promise and assert exit.ended stays false until it is released, as the shutdown test above does.
| const store = stream.store.current as { | ||
| session: { outroData: { kind: string; errorCode?: string } | null }; | ||
| }; | ||
| expect(store.session.outroData).toMatchObject({ |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
This reads back the outroData that the test's own runProgram mock wrote at line 222; run.ts never touches outroData on this path, so the assertion only proves the stream and the run share a store, not that the failure reaches the stream. Assert the stream settled with the failure instead, e.g. expect(stream.shutdown).toHaveBeenCalledWith(2000, status === 'cancelled' ? 'cancelled' : 'failed').
| it('keeps a run a success when its terminal analytics flush fails', async () => { | ||
| const flushError = new Error('flush timed out'); | ||
| vi.mocked(analytics.shutdown).mockRejectedValueOnce(flushError); | ||
| await runHeadless(program(), launch()); |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: The name says the run stays a success, but the test never checks the exit code. Use await expect(runHeadless(program(), launch())).resolves.toBe(0) so a regression to exit 1 fails it.
| }, | ||
| ); | ||
|
|
||
| it("prints the gateway's 401 guidance before it ends the run", async () => { |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: The name says the 401 guidance prints before the run ends, but the test does not check order, and printAbortOutro also prints the 'Authentication failed (401)' line. Assert that the "Missing scope" line's index in lines is lower than the abort outro's line.
| }); | ||
| return Promise.resolve(settled(outcome, failure)); | ||
| }); | ||
| const stderr = vi |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: This process.stderr.write spy, and the one at line 264, are restored inline after assertions; if an assertion throws first the spy leaks into later tests, because afterEach only restores console.log. Call vi.restoreAllMocks() in afterEach, as signals.test.ts does.
| process.env.POSTHOG_WIZARD_REGION = 'us'; | ||
| process.env.POSTHOG_WIZARD_API_KEY = 'phx_env_key'; | ||
| process.env.POSTHOG_WIZARD_INSTALL_DIR = '/tmp/test'; | ||
| process.env.POSTHOG_WIZARD_LOG_FILE = '/tmp/wizard-cli-test.log'; |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: POSTHOG_WIZARD_LOG_FILE is not in WIZARD_ENV_KEYS, so clearWizardEnv() never resets it; this test overwrites the per-worker log path vitest.config.ts sets in test.env and the new value stays for the rest of the worker. Save the original value and restore it in a finally at the end of the test; adding it to WIZARD_ENV_KEYS would delete the per-worker value instead.
94dbe58 to
d48e5cd
Compare
4430bae to
f0141c7
Compare
d48e5cd to
0d958e3
Compare
Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
f0141c7 to
fda54fc
Compare
0d958e3 to
d38f714
Compare
edwinyjlim
left a comment
edwinyjlim
left a comment
There was a problem hiding this comment.
Review of #1390 at d38f7147. Items outside this PR's diff lines:
[P3] No CLI test for the slack tool command
src/cli/commands/slack.ts:23
Suggested fix: Add slack-cli.test.ts mirroring mcp-cli.test.ts: runTuiTool called with the Slack tool and the session args, then exit 0; a rejection exits 1 with the PHW line.
[P3] The command-tree conflict check covers 9 of the 17 registered commands
src/cli/__tests__/wizard.test.ts:83
An alias collision involving any of the eight omitted commands, the exact bug this test exists for, is invisible.
Suggested fix: Export the command list from src/cli/index.ts, the source of truth, and pass it to findConflicts.
Drafted with Claude Code from a review of the whole Release C stack, re-verified at the stack tip 2e31e438. Line numbers are at this PR's head.
| stream.destinations(opts.destinations.map((d) => d.name)); | ||
| stream.store.current = opts.store; | ||
| } | ||
| attach = vi.fn(); |
There was a problem hiding this comment.
[P3] The headless run test leaves three paths unasserted
attach is never checked, and without it nothing streams. The local-services-down abort and the Crashed outcome have no test, though the TUI twin tests its crash path.
Suggested fix: Assert attach in the destinations test; add a local-services-down case expecting 1 and the error line; add a Crashed case expecting 1 and the outro's error code.
Phase 2 of Release C (#1320), PR 14 of 16, after #1389. Tests only. PRs 1 to 14 of this phase split one change into reviewable pieces and compile only as a stack: review them one by one, then land the whole phase together.
Problem
The hosts, the tools' console runners and the CLI's signal handling are new in #1383 to #1385, and they decide every exit code. Each needs a test that fails when its code or order changes.
Changes
Tests only: 26 files, including the
scripts/*.no-jest.tstest scripts and the e2e harness tests.src/headless/__tests__/run.test.ts(a CI detection's lines print, the gateway's 401 guidance prints before the run ends, a failed analytics flush keeps a success, SIGTERM resolves 143) andrenderers/__tests__/progress-log.test.ts.runners/__tests__/signals.test.ts(SIGTERM aborts the host with the signal name, a host that rejects exits 1).cli.test.ts,mcp-cli.test.tsandprovision-cli.test.tsfollow the new handlers.src/host/__tests__/wizard-abort.test.tsfollowswizardAbortto the presenter andstartHostExit, and checks it ends the exit of the host it is passed.report.test.tsandconsole.test.tslockrunDoctorReportandaddMCPServerToClientsStepresolving 0 or 1.wizard-ci-driver.test.ts,e2e-flow-snapshot.test.tsandscripts/tui-host.no-jest.tsdrive the store and screens through the@tuientry.keyboard-equivalence.test.tsxand its snapshot move toe2e-harness/__tests__fromsrc/tuias a rename, beside the action registry they check.Test plan
pnpm typecheck0 errors,pnpm lint0 errors, 236 test files (3,515 tests) pass,pnpm buildok.Created with PostHog Desktop