Skip to content

fix: Parallelize integration test suite (338s -> 77s) - #1137

Open
MQ37 wants to merge 4 commits into
masterfrom
fix/parallel-integration-tests
Open

fix: Parallelize integration test suite (338s -> 77s)#1137
MQ37 wants to merge 4 commits into
masterfrom
fix/parallel-integration-tests

Conversation

@MQ37

@MQ37 MQ37 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What

Fixes notifications/progress delivery (a real MCP-spec-compliance bug: _meta was placed at the notification's top level instead of inside params, so any strict client silently dropped the message) and uses it to give integration tests a deterministic way to learn a run's runId. Uses that to remove the last blocker to running tests/integration/suite.ts concurrently instead of sequentially.

Why

The integration suite forced concurrent: false for two reasons: every test shared one client variable/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. Fixing notifications/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 a desc: true, limit: N listing" 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:

  • before (sequential, original code): 338.45s, 240 passed / 19 skipped / 0 failed
  • after (concurrent, this PR): 77.24s, 236 passed / 19 skipped / 0 failed (the 4 fewer = the 2 deleted tests × 2 transports)

Independently re-verified via a fresh subagent (re-ran the full suite against the live API itself, not just the reported numbers).

MQ37 added 4 commits July 23, 2026 12:16
- 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
@MQ37
MQ37 force-pushed the fix/parallel-integration-tests branch from 4efcbe4 to 3916b90 Compare July 23, 2026 10:26
@github-actions github-actions Bot added t-ai Issues owned by the AI team. tested Temporary label used only programatically for some analytics. labels Jul 23, 2026

Copy link
Copy Markdown
Contributor

Clients leak on test failure now that cleanup moved into the test body.

The old afterEach(() => client?.close()) ran even when a test failed. The new pattern puts await client.close() as the last statement of each test body, so any earlier expect failure or thrown error skips it:

client = await createClientFn();
const prompts = await client.listPrompts();
expect(prompts.prompts.length).toBe(0);  // if this throws…
await client.close();                     // …never runs → leaked client

A leaked client is a child process (stdio) or a server session (streamable-http). Several tests carry { retry: 1..3 }, and with concurrent: true a flaky test now leaks a client per attempt, in parallel — exactly the resource pressure concurrency adds, which can cascade into more flakiness.

Suggest guaranteeing cleanup with try { … } finally { await client?.close() }, or a test.extend fixture with automatic teardown (also removes the repeated let client / await client.close() boilerplate). I know onTestFinished was tried and raced on a shared module-level variable — a try/finally or fixture sidesteps that since neither relies on vitest's current-test tracking.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-ai Issues owned by the AI team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants