fix: Parallelize integration test suite (338s -> 77s) - #1137
Conversation
- Move _meta from the notification's top level into params._meta. JSONRPCNotificationSchema is .strict(); a top-level _meta fails classification and the whole message is silently dropped as "Unknown message type" by any spec-compliant client (verified against the installed MCP SDK). This also fixes existing RELATED_TASK_META_KEY delivery, which had the same bug. - ProgressTracker gains setRunId(); notifications/progress now carries runId under the existing APIFY_ACTOR_RUN_META_KEY (com.apify/ActorRun) _meta key, set before the first emission in actor_run_response.ts. - Task-mode progress notifications route through the session transport (sendTaskProgressNotification, same pattern as emitTaskStatusNotification) instead of the request-scoped extra.sendNotification. Needed because task execution continues after the initial CreateTaskResult response is flushed, closing the per-request stream extra.sendNotification is bound to (webStandardStreamableHttp.js throws "No connection established for request ID", silently swallowed by ProgressTracker) — this made task-mode notifications/progress undeliverable over streamable-http. Lets clients (and now integration tests) learn a run's ID from the first progress notification instead of polling the run-list API.
Every test shared one client variable and a single afterEach(close) — fundamentally incompatible with running tests in parallel. Plus 3 abort/cancel tests discovered the Apify runId via a run-list-polling heuristic (captureInflightActorRunId) that races if any other test touches the same Actor concurrently. - Every test now declares its own local client and closes it explicitly; removed the shared client/afterEach. Tried vitest's onTestFinished() for automatic cleanup first — found it uses a single mutable module-level variable for the current test, not real per-test context, so it races under concurrency (misattributes cleanup, sometimes throws). Reverted to plain explicit client.close() per test, which needs nothing from vitest's test-tracking internals. - The 2 direct abort tests and the task-mode tasks/cancel test now use captureRunIdFromProgress (see prior commit) instead of captureInflightActorRunId, which is now fully unused and deleted. - describe(...) flipped from concurrent: false to concurrent: true. - Deleted 2 tests (get-dataset-list, get-key-value-store-list) whose "desc: true, limit: N" assertion assumed their beforeAll-created storage would still be on page 1 — true only when few other tests had run yet (i.e. under the old sequential order), false once other tests' concurrent Actor runs can push it off the page. Unit coverage for both tools already exists separately. Verified against the real Apify API: full suite (both transports), 236 passed / 19 skipped / 0 failed, before vs after: before (sequential): 338.45s after (concurrent): 77.24s
4efcbe4 to
3916b90
Compare
|
Clients leak on test failure now that cleanup moved into the test body. The old client = await createClientFn();
const prompts = await client.listPrompts();
expect(prompts.prompts.length).toBe(0); // if this throws…
await client.close(); // …never runs → leaked clientA leaked client is a child process (stdio) or a server session (streamable-http). Several tests carry Suggest guaranteeing cleanup with Generated by Claude Code |
What
Fixes
notifications/progressdelivery (a real MCP-spec-compliance bug:_metawas placed at the notification's top level instead of insideparams, so any strict client silently dropped the message) and uses it to give integration tests a deterministic way to learn a run'srunId. Uses that to remove the last blocker to runningtests/integration/suite.tsconcurrently instead of sequentially.Why
The integration suite forced
concurrent: falsefor two reasons: every test shared oneclientvariable/afterEach(close)(races under concurrency), and 3 abort/cancel tests discovered the Apify runId via a run-list-polling heuristic that isn't safe if another test touches the same Actor at the same time. Fixingnotifications/progress(see first commit) replaces that heuristic with a direct signal from the server, removing the need for isolation. Also deleted 2 tests whose "top of adesc: true, limit: Nlisting" assertion only held under the old sequential ordering — real concurrent Actor runs from other tests can push their target off page 1; unit coverage for both tools already exists separately.Testing
pnpm run type-check,lint,format,check:agents,test:unit(1137 passed) all clean. Full integration suite against the real Apify API, both transports, before vs after:Independently re-verified via a fresh subagent (re-ran the full suite against the live API itself, not just the reported numbers).