Add a goosed over HTTP integration test, and test the developer tool PATH#7178
Add a goosed over HTTP integration test, and test the developer tool PATH#7178
Conversation
|
|
||
| // ... tests | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
there's a bunch of unnecessary content in the README, I'll clean that up
There was a problem hiding this comment.
Pull request overview
This PR scaffolds integration tests for the goosed binary, enabling HTTP requests via the auto-generated TypeScript API client. The test suite spawns a real goosed process, waits for it to be ready, and issues requests to verify the server works correctly end-to-end.
Changes:
- Created a new Vitest integration config for tests that spawn real processes
- Implemented test setup utilities for spawning and managing goosed processes
- Added initial integration tests covering status, configuration, and provider endpoints
- Added comprehensive README documentation for writing integration tests
- Added npm scripts for running integration tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/vitest.integration.config.ts | Separate Vitest config for integration tests using Node environment with 30s timeouts |
| ui/desktop/tests/integration/setup.ts | Test utilities for spawning goosed, managing process lifecycle, and creating authenticated API clients |
| ui/desktop/tests/integration/goosed.test.ts | Initial integration tests covering status, config, and provider endpoints |
| ui/desktop/tests/integration/README.md | Documentation for writing integration tests, including setup instructions and examples |
| ui/desktop/package.json | Added npm scripts for running integration tests in different modes |
DOsinga
left a comment
There was a problem hiding this comment.
great. I think we can delete the README since it is more about the goose plan and then setup.ts could use more of main.ts, but other than that, this is very good
| @@ -0,0 +1,178 @@ | |||
| /** | |||
There was a problem hiding this comment.
this could re-use a lot from main (and by way of that, also test it), like starting goosed etc
There was a problem hiding this comment.
goose made a little bit of a mess of this on first try, but I think it's in a good place now. Now the only bits specific to the test setup are the temp root and config creation, everything else is used from goosed.ts
| let portCounter = 13100; | ||
|
|
||
| function getNextPort(): number { | ||
| return portCounter++; |
There was a problem hiding this comment.
in main I think we actually find a free port?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
ui/desktop/src/utils/tests/parameterSubstitution.test.ts:5
- The outer
describe('providerUtils', ...)no longer matches the module under test (nowparameterSubtitution/substituteParameters); updating the describe label will make failures easier to interpret.
| const possiblePaths = [ | ||
| path.join(process.cwd(), 'src', 'bin', 'goosed'), | ||
| path.join(process.cwd(), 'bin', 'goosed'), | ||
| path.join(process.cwd(), '..', '..', 'target', 'debug', 'goosed'), | ||
| path.join(process.cwd(), '..', '..', 'target', 'release', 'goosed'), | ||
| ]; |
There was a problem hiding this comment.
findGoosedBinary() only checks for goosed (no extension), so it will fail on Windows where the binary is typically goosed.exe; consider appending .exe when process.platform === 'win32' (or checking both variants for each candidate path).
| goosedProcess.kill('SIGTERM'); | ||
|
|
||
| // Force kill after timeout | ||
| setTimeout(() => { | ||
| if (!goosedProcess.killed) { | ||
| goosedProcess.kill('SIGKILL'); | ||
| } |
There was a problem hiding this comment.
goosedProcess.kill('SIGTERM')/SIGKILL are not supported consistently on Windows and can cause cleanup to hang; consider using a platform-specific termination strategy (e.g., kill() without a signal on Windows, or taskkill) similar to ui/desktop/src/goosed.ts.
| const goosedProcess = spawn(goosedPath, ['agent'], { | ||
| env: { ...process.env, ...env }, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
spawn(goosedPath, ...) will crash if goosedPath is null; handle the not-found case explicitly (e.g., throw with a helpful message listing searched paths / the override env var).
ui/desktop/src/goosed.ts
Outdated
| if (isWindows && goosedProcess.unref) { | ||
| goosedProcess.unref(); | ||
| } | ||
| const goosedProcess = spawn(goosedPath, [], spawnOptions); |
There was a problem hiding this comment.
goosed requires the agent subcommand (see crates/goose-server/src/main.rs), but this spawns it with no args, so it will exit immediately with a usage error; pass ['agent'] here to actually start the HTTP server.
| const goosedProcess = spawn(goosedPath, [], spawnOptions); | |
| const goosedProcess = spawn(goosedPath, ['agent'], spawnOptions); |
ui/desktop/src/goosed-app.ts
Outdated
| } | ||
|
|
||
| try { | ||
| await status({ client }); |
There was a problem hiding this comment.
status({ client }) won’t throw on non-2xx responses unless throwOnError: true is set, so this can report the server as “ready” even if /status is returning an error payload; pass throwOnError: true (or explicitly check response.ok) inside the polling loop.
| await status({ client }); | |
| await status({ client, throwOnError: true }); |
This reverts commit 80fdc0d.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| app.on('will-quit', async () => { | ||
| log.info('App quitting, terminating goosed server'); | ||
| await goosedResult.cleanup(); | ||
| }); |
There was a problem hiding this comment.
The app.on('will-quit') handler is registered inside createChat, which can be called multiple times (once per window). This will register multiple cleanup handlers that all try to kill the same goosed process. Consider tracking goosed processes globally and cleaning them up once, or deregistering old handlers when new ones are added.
| const serverReady = await checkServerStatus(client, errorLog); | ||
| if (!serverReady) { | ||
| baseCleanup(); | ||
| console.error('Server stderr:', errorLog.join('\n')); | ||
| throw new Error('Failed to start goosed'); | ||
| } |
There was a problem hiding this comment.
If checkServerStatus fails at line 97, baseCleanup is called at line 99 but tempDir is not removed. This leaves temporary directories behind on test failures. Consider wrapping the cleanup in a try-finally or calling the full cleanup function instead of just baseCleanup.
* origin/main: (42 commits) fix: use dynamic port for Tetrate auth callback server (#7228) docs: removing LLM Usage admonitions (#7227) feat(otel): respect standard OTel env vars for exporter selection (#7144) fix: fork session (#7219) Bump version numbers for 1.24.0 release (#7214) Move platform extensions into their own folder (#7210) fix: ignore deprecated skills extension (#7139) Add a goosed over HTTP integration test, and test the developer tool PATH (#7178) feat: add onFallbackRequest handler to McpAppRenderer (#7208) feat: add streaming support for Claude Code CLI provider (#6833) fix: The detected filetype is PLAIN_TEXT, but the provided filetype was HTML (#6885) Add prompts (#7212) Add testing instructions for speech to text (#7185) Diagnostic files copying (#7209) fix: allow concurrent tool execution within the same MCP extension (#7202) fix: handle missing arguments in MCP tool calls to prevent GUI crash (#7143) Filter Apps page to only show standalone Goose Apps (#6811) opt: use static for Regex (#7205) nit: show dir in title, and less... jank (#7138) feat(gemini-cli): use stream-json output and re-use session (#7118) ...
A test that starts goosed an issues HTTP requests to it using the generated typescript SDK
The prompt was