refactor: Replace Scenario duplication with a shared Case model (test-kit) - #1202
Draft
MQ37 wants to merge 8 commits into
Draft
refactor: Replace Scenario duplication with a shared Case model (test-kit)#1202MQ37 wants to merge 8 commits into
MQ37 wants to merge 8 commits into
Conversation
commit: |
Splits tests/integration/suite.ts (3292 lines, 117 cases) into tests/integration/cases/*.cases.ts, grouped flat by capability (registration, tools, actors, apps, tasks, storage, payments), per the plan agreed in #776 (flat structure, no folder-per-tool mirror of src/). Every register*Cases(ctx) is a mechanical move — same test names, bodies, and transport gates; suite.ts now only wires the three transport dimensions and calls the register functions. Verified via AST title diff: 117 titles before, 117 after, zero difference. Adds src/test_kit/ (published behind the "./test-kit" package export, vitest optional peerDependency) so apify-mcp-server-internal can import and run a curated critical: true subset against its own live deploy instead of hand-duplicating assertions. Wires two scenarios (tests/integration/cases/shared_scenarios.ts) through it as a working example; growing that subset is a per-PR judgment call, left for follow-up. Updates AGENTS.md and DEVELOPMENT.md per #776's own requirement to document the new structure when the first capability module lands. Fixes #776 AI use disclosed: mechanical statement extraction (line-range slicing driven by a TypeScript AST parse) was scripted; grouping decisions, the test-kit design, and all verification were reviewed by hand.
MQ37
force-pushed
the
refactor/776-flat-integration-test-cases
branch
from
August 5, 2026 11:13
19bd50d to
c679b71
Compare
Moves createMcpStreamableClient/createMcpStatelessClient from tests/helpers.ts (unpublished test tree) into src/test_kit/mcp_client.ts, exported behind the existing "./test-kit" package export. Widens SuiteClientOptions to a strict superset of apify-mcp-server-internal's own MCPClientOptions: - explicit token?: string | null (omitted -> process.env.APIFY_TOKEN, unchanged default; null -> no Authorization header at all, needed for internal's negative-auth / payment-mode tests where the run's APIFY_TOKEN is set for everything else) - serverMode/payment as plain strings, matching internal's ui/ paymentProvider/skyfireMode fields (same ?ui=/?payment= query params under the hood, one canonical name instead of three) tests/helpers.ts now re-exports these instead of maintaining its own copy; createMcpStdioClient (spawns this repo's own dist/stdio.js, irrelevant to a hosted-deploy consumer) stays local and unpublished. Lets apify-mcp-server-internal delete test/integration/src/mcp-clients.ts entirely and import the same client instead of maintaining its own. Verified: type-check, lint, format, 1267 unit tests, full integration suite collection (356 tests, unchanged), build (dist/test_kit/mcp_client.js), check:agents all pass. Refs #776
github-actions
Bot
added
the
tested
…rename streamable-http dimension to 2025-11-25
Replaces the hand-duplicated Scenario/scenarios.ts sharing mechanism with a
single Case model: every integration case is now a plain { name, critical,
run, ... } object defined once in src/test_kit/cases/*.cases.ts (published
behind the ./test-kit export). Marking a case critical is a one-line edit on
its own definition — no second array or file to keep in sync. This repo's own
suite.ts registers every case via registerCases(name, cases, ctx); internal
will register the same arrays with criticalOnly: true and pick up every
current and future critical case automatically.
Folds in and supersedes the previous commit's Scenario-based
scenarios.ts/registerScenarios mechanism entirely — that intermediate step
never shipped; every line it added is deleted here in favor of the Case
model below.
- src/test_kit/types.ts: Scenario -> Case, ScenarioCtx -> CaseCtx, adds
hasTasksSupport/skipIf/retry so per-case transport gating and known-flaky
retries move with the case instead of living in a separate registration call.
- src/test_kit/register.ts: registerScenarios -> registerCases, honors skipIf.
- src/test_kit/helpers.ts (new): merges tests/integration/cases/shared.ts,
tests/const.ts, tests/integration/utils/task_waits.ts, and
tests/helpers.ts's asLegacyClient into one published module, plus the new
withClient(options, testFn) helper every simple case's run is built from.
- src/test_kit/cases/*.cases.ts (new): registration/tools/actors/apps/tasks/
storage/payments, migrated from tests/integration/cases/*.cases.ts. All 9
tasks cases and the 2 pre-existing shared scenarios are now critical: true,
folded directly into their group instead of a separate registerScenarios
call — this also removes one accidental duplicate run of "should list all
default tools and Actors" (previously registered twice: once as its own
itc() case, once via the shared-scenario describe block).
- payments.cases.ts: widened from streamable-http-only to also run on the
2026-07-28 v2 client (plain listTools/callTool cases), except the one
task-mode case that needs the v1-only experimental.tasks API.
- storage: the 5 self-contained cases move to src/test_kit/cases/storage.
cases.ts; the 13 cases sharing one beforeAll-seeded Actor run don't fit the
flat Case model (would mean paying for 13x redundant Actor runs) and stay
local in tests/integration/cases/storage_grouped.cases.ts — not published,
not critical-eligible.
- Transport dimension literal 'streamable-http' -> '2025-11-25', symmetric
with '2026-07-28' and the 2025_11_25/2026_07_28 naming already used by the
conformance-test config. The MCP protocol transport-type string
('streamable-http' in server_card.ts/server.json) is untouched — only the
test-suite dimension label changes.
- oxlint.config.ts: scoped override disabling vitest/no-standalone-expect and
vitest/expect-expect for src/test_kit/** — withClient()'s indirection hides
expect() calls from the linter's static it()-nesting check even though they
run inside a real it() at runtime (see register.ts).
- Deletes tests/integration/cases/shared.ts, shared_scenarios.ts, and
src/test_kit/scenarios.ts (superseded).
Verified: type-check, lint, format, unit tests (91 files/1267 tests), build
(dist/test_kit/cases/*.js present), check:agents, and an AST-based diff of
every case's title against the pre-migration source — 117 unique tests
collected per entry file (was 118, exactly the one deduplicated case), 353
total across all 4 integration entry files (down from 356 by the same 3x1).
Adds a Fixture<T> = { key, setup(ctx) } type and ctx.getFixture(fixture),
memoized once per registerCases() call (i.e. once per transport dimension),
regardless of how many cases in the array ask for it or in what order. This
lets several cases share one expensive setup step without a vitest beforeAll,
which can't express "shared by only some cases in this flat array" and forces
that whole group out of the Case model.
Uses it to fold storage's 13 cases that shared a beforeAll-seeded Actor run
(previously stuck in tests/integration/cases/storage_grouped.cases.ts — not
published, not critical-eligible) into ordinary Case objects in storage.cases.ts,
via a normalModeRunFixture + withNormalModeRun helper. All 18 storage cases
are now uniform, flat, and critical-eligible like every other group; the
setup Actor run still only executes once per dimension.
- src/test_kit/types.ts: Fixture<T>, CaseCtx.getFixture.
- src/test_kit/register.ts: builds one fixture cache per describe() block,
injects getFixture into the ctx passed to every case's run().
- src/test_kit/cases/storage.cases.ts: merges storage_grouped's 13 cases in,
now 18 total (was 5 + 13 local-only).
- Deletes tests/integration/cases/storage_grouped.cases.ts and its call site
in suite.ts.
- Docs (AGENTS.md, DEVELOPMENT.md) updated to describe getFixture instead of
the "one group doesn't fit the model" caveat.
Verified: type-check, lint, format, unit tests (91 files/1267 tests), build,
check:agents, and integration-suite collection count (117 tests per entry
file, 353 total — unchanged from before this change, confirming the 13
re-homed cases collect 1:1, zero loss).
… Case arrays apify-mcp-server-internal has 7 of its own hosted-only local groups (auth, payments, apps, misc, tool-loading, output-schema, rate-limiting) using the same register*Cases(ctx) function pattern this repo migrated away from. Exporting withClient lets it convert those to plain Case[] arrays too, reusing this repo's exact per-case client open/close helper instead of reinventing it or keeping a bespoke setClient + shared afterEach mechanism. Verified: type-check, lint, format, unit tests (91 files/1267 tests), build.
…env directly tasks.cases.ts (3x) and actors.cases.ts (1x) construct a raw ApifyClient to poll run status directly (waitForRunAborted), independent of any one MCP connection's token. Reading process.env.APIFY_TOKEN there hardcoded an assumption only true in this repo's own integration-test process. apify-mcp-server-internal uses a different token source (its own test-user helper, not a plain env var) for its MCP connections already — but these 4 call sites bypassed that and read the env var directly, so under internal they silently got an unauthenticated ApifyClient. waitForRunAborted's 60s poll then times out every time (retry: 2 makes that up to 3x = ~180s per test) instead of failing fast with a clear cause. CaseCtx.getApifyToken() fixes this the same way createClientFn's own token resolution already works: each caller wires it to its own real token source (this repo: process.env.APIFY_TOKEN; internal: its own token helper) instead of the case reaching for a global directly. Made required, not optional with an env-var fallback, so a caller missing it is a compile error, not a silent 180-second hang three retries later. Verified: type-check, lint, format, unit tests (91 files/1267 tests), build.
…t silently discarded it Direct-ApifyClient polling in tasks.cases.ts/actors.cases.ts (waitForRunAborted, run-status checks) had no baseUrl override, so it always resolved to the wrapper's own getApifyAPIBaseUrl() default — fine for this repo's own suite (real production token), wrong for internal's dev/staging suites, which run their own platform (e.g. http://localhost:3333) with environment-scoped test-user tokens that don't exist on production. getApifyApiBaseUrl() mirrors getApifyToken(): required, not defaulted, so a caller missing it is a compile error, not a silent wrong-environment API call. The first attempt at this wiring didn't actually work: src/apify_client.ts's ApifyClient wrapper built its super() call with `baseUrl: getApifyAPIBaseUrl()` unconditionally last, discarding whatever baseUrl the caller passed in clientOptions. Fixed here in the same commit — shipping the wiring without this fix would have left a commit that compiles but has no actual effect, which is exactly what caused the "User was not found or authentication token is not valid" failures to persist unchanged after the wiring alone was installed downstream. Now: caller-supplied baseUrl wins, getApifyAPIBaseUrl()'s env-var-or-prod default only applies when the caller didn't ask for a specific one. Every other call site in the repo omits baseUrl, so behavior elsewhere is unchanged. Verified beyond unit tests: ran the compiled wrapper directly against the real (unmocked) apify-client package — confirmed override wins, env-var fallback still works, prod default still applies with neither set. Traced apify-client's own resource_client/_url() to confirm this.baseUrl (with the override applied) is what every REST call is actually built from. Added one focused unit test asserting the override wins over the env-var default - the actual regression this guards.
…ases, mark critical The old single case asserted telemetry off (report-problem absent) via the suite's own default, never setting telemetry explicitly. internal's own tool-loading.cases.ts carried a near-duplicate asserting the opposite (report-problem present, since its hosted deploy runs telemetry on by default) - same scenario, diverging only because of which suite's default happened to apply, not a real behavioral difference worth two separate hand-maintained tests in two repos. Now: two cases, each forcing telemetry via withClient's own option instead of relying on any suite/environment default, both critical so internal gets both automatically. Each asserts the full tools list, not just report-problem's presence - confirms toggling telemetry changes only that one tool, nothing else shifts. Also adds 'should handle mixed categories and specific tools in tools param' (moved from internal's tool-loading.cases.ts as-is, marked critical) - pure tools_loader selector-merging logic with no hosting dependency and no existing similar case here.
MQ37
force-pushed
the
refactor/776-flat-integration-test-cases
branch
from
August 6, 2026 14:38
e9c3ff7 to
37db6bb
Compare
MQ37
changed the title
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
2 participants
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.
🚧 DO NOT REVIEW — WIP. May be split into multiple PRs once finished.
What
Replaces the hand-duplicated
Scenario/scenarios.tssharing mechanism with a singleCasemodel: every integration test is a plain{ name, critical, run, ... }object, defined once insrc/test_kit/cases/*.cases.tsand published behind the./test-kitpackage export. Marking a casecritical: trueis a one-line edit —apify-mcp-server-internalpicks it up automatically viaregisterCases(name, allCases, { criticalOnly: true }), no second array to keep in sync.Also:
Fixture<T>+ctx.getFixture(fixture)— lets several cases share one expensive setup step (e.g. one seeded Actor run), memoized once per transport dimension, without a vitestbeforeAll(which can't express "shared by only some cases in a flat array"). Removed the last case group that didn't fit the flat model.withClientexported fromtest-kit, so internal's own local case groups reuse the exact same per-case client open/close helper instead of a second implementation.CaseCtx.getApifyToken()/getApifyApiBaseUrl()— required fields for the handful of cases that poll the raw Apify REST API directly (waitForRunAborted), independent of any one MCP connection. Found and fixed two real bugs surfaced only once internal actually ran these against its own live deploy: a silent unauthenticated-client hang (180s per retry) from readingprocess.env.APIFY_TOKENdirectly instead of throughctx, and theApifyClientwrapper (src/apify_client.ts) unconditionally overwriting any caller-suppliedbaseUrlwith its own env-var/prod default — so the first fix for the latter had zero effect until this was found.'streamable-http'→'2025-11-25', symmetric with'2026-07-28'(the real MCP protocol transport-type string is untouched — only the test-suite dimension label changed).should list all default tools and Actors(internal's copy was an exact duplicate) is now removed there entirely, and the telemetry-dependent case is now two explicit critical cases here (telemetry forced on / off viawithClient's own option, not any suite default) instead of two suites each asserting the opposite based on their own environment default.Why
One 3292-line file, 117 cases, unmaintainable by more than one person at a time (#776). Beyond the split itself, internal had been hand-duplicating ~40 assertions from this suite instead of sharing a single source of truth — this closes that gap structurally (mark critical, done) rather than by convention.
Testing
pnpm run type-check,lint,format,test:unit(91 files / 1268 tests, 1 skipped),check:agents,buildall pass at every commit.getApifyApiBaseUrlwrapper fix verified beyond unit tests: ran the compiled wrapper directly against the real (unmocked)apify-clientpackage — confirmed the caller override wins, the env-var fallback still works, and the prod default still applies with neither set.Scenario-model scaffolding that got replaced by theCasemodel two commits later; agetApifyApiBaseUrlwiring commit that didn't actually work until its own fix landed) —git diffagainst a pre-squash backup tag is empty.AI use disclosure
Mechanical statement extraction (AST-driven line-range slicing) was scripted for the initial split and thrown away after use; the
Case/Fixturemodel design, both real bugs, and all verification were done and reviewed by hand.Fixes #776