Migrate SDK HTTP tests to MSW and preserve FormData payloads - #145
Migrate SDK HTTP tests to MSW and preserve FormData payloads#145base44-os-gremlins[bot] wants to merge 10 commits into
Conversation
Migrate all unit tests from nock-based HTTP interception to a proper
MSW (Mock Service Worker) mock server using msw/node + @mswjs/interceptors.
Changes:
- Add msw@2 as devDependency, remove nock
- Create tests/mocks/server.ts: MSW server with documentation on adding handlers
- Update tests/setup.js: start/stop/reset MSW server in beforeAll/afterEach/afterAll
- Port all 7 nock-based test files to per-test server.use() handlers:
entities, auth, functions, connectors, integrations, custom-integrations, client
- Replace vi.stubGlobal("fetch", ...) in functions.test.ts with MSW handlers
- Add request capture pattern for asserting headers (Authorization, Base44-State)
- Use RegExp patterns for MSW handlers where operationId contains URL-unsafe chars
All 116 unit tests pass (npm run test:unit exits 0).
Goal: https://github.com/base44-dev/gremlins/issues/873
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/sdk@0.8.48-pr.145.e1cc9fePrefer not to change any import paths? Install using npm alias so your code still imports npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.48-pr.145.e1cc9fe"Or add it to your {
"dependencies": {
"@base44/sdk": "npm:@base44-preview/sdk@0.8.48-pr.145.e1cc9fe"
}
}
Preview published to npm registry — try new features instantly! |
There was a problem hiding this comment.
Pull request overview
This PR migrates the unit test suite from nock-based HTTP interception to an MSW (msw/node, v2) mock server, centralizing request mocking and enabling per-test handlers via server.use().
Changes:
- Added a shared MSW Node server (
tests/mocks/server.ts) and wired its lifecycle into the Vitest global setup (tests/setup.js). - Ported multiple unit test files from
nockto MSW handlers (http.*+HttpResponse), including request capture for header assertions. - Updated dependencies to introduce
msw(and intended to removenock).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/integrations.test.js | Migrates integration endpoint tests from nock to MSW handlers. |
| tests/unit/functions.test.ts | Replaces nock + fetch stubbing with MSW handlers; adds header capture assertions. |
| tests/unit/entities.test.ts | Migrates entities module tests to MSW with query/body validation in handlers. |
| tests/unit/custom-integrations.test.ts | Migrates custom integrations tests to MSW; adds URL matching helper for encoded operation IDs. |
| tests/unit/connectors.test.ts | Migrates connector token retrieval tests from nock to MSW. |
| tests/unit/client.test.js | Updates client tests to use MSW patterns and ensures cleanup is called. |
| tests/unit/auth.test.js | Migrates auth tests from nock to MSW; uses MSW to validate auth header behavior in some flows. |
| tests/setup.js | Starts/stops the MSW server for the suite and resets handlers after each test. |
| tests/mocks/server.ts | Introduces the shared MSW Node server and handler authoring documentation. |
| package.json | Adds msw devDependency (and removes nock). |
| package-lock.json | Adds MSW dependency entries, but still includes nock in root devDependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Global beforeAll and afterAll hooks | ||
| // MSW server lifecycle | ||
| beforeAll(() => { | ||
| server.listen({ onUnhandledRequest: 'warn' }); |
There was a problem hiding this comment.
server.listen({ onUnhandledRequest: 'warn' }) will warn and then passthrough unmatched requests to the real network in MSW v2. For unit tests, this can make the suite flaky and can accidentally hit real endpoints. Prefer onUnhandledRequest: 'error' (or a custom handler) so unmocked requests fail fast.
| server.listen({ onUnhandledRequest: 'warn' }); | |
| server.listen({ onUnhandledRequest: 'error' }); |
| import { createClient, createClientFromRequest } from '../../src/index.ts'; | ||
| import { describe, test, expect, beforeEach, afterEach } from 'vitest'; | ||
| import nock from 'nock'; | ||
| import { describe, test, expect, afterEach } from 'vitest'; |
There was a problem hiding this comment.
Unused import: afterEach is imported from vitest but never used in this file. Remove it to keep the test file clean and avoid lint warnings.
| import { describe, test, expect, afterEach } from 'vitest'; | |
| import { describe, test, expect } from 'vitest'; |
| // The SDK URL-encodes only curly braces in operationIds (not : or /) | ||
| function sdkOperationUrl(slug: string, operationId: string): string { | ||
| const encoded = operationId.replace(/{/g, '%7B').replace(/}/g, '%7D'); | ||
| return `${customBase}/${slug}/${encoded}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
sdkOperationUrl() is declared but never used. Consider removing it (or using it in the tests) to avoid dead code and keep the helper section focused.
| // The SDK URL-encodes only curly braces in operationIds (not : or /) | |
| function sdkOperationUrl(slug: string, operationId: string): string { | |
| const encoded = operationId.replace(/{/g, '%7B').replace(/}/g, '%7D'); | |
| return `${customBase}/${slug}/${encoded}`; | |
| } |
| test('should use appBaseUrl for login redirect when provided', () => { | ||
| const customAppBaseUrl = 'https://custom-app.example.com'; | ||
| const clientWithCustomUrl = createClient({ | ||
| serverUrl, | ||
| appId, | ||
| appBaseUrl: customAppBaseUrl, | ||
| }); | ||
| const clientWithCustomUrl = createClient({ serverUrl, appId, appBaseUrl: customAppBaseUrl }); | ||
|
|
||
| // Mock window.location | ||
| const originalWindow = global.window; | ||
| const mockLocation = { href: '' }; | ||
| global.window = { | ||
| location: mockLocation | ||
| }; | ||
| global.window = { location: mockLocation }; | ||
|
|
||
| const nextUrl = 'https://example.com/dashboard'; | ||
| clientWithCustomUrl.auth.redirectToLogin(nextUrl); | ||
| clientWithCustomUrl.auth.redirectToLogin('https://example.com/dashboard'); | ||
|
|
||
| // Verify the redirect URL uses the custom appBaseUrl | ||
| expect(mockLocation.href).toBe( | ||
| `${customAppBaseUrl}/login?from_url=${encodeURIComponent(nextUrl)}` | ||
| `${customAppBaseUrl}/login?from_url=${encodeURIComponent('https://example.com/dashboard')}` | ||
| ); | ||
|
|
||
| // Restore window | ||
| global.window = originalWindow; |
There was a problem hiding this comment.
This test creates an additional client (clientWithCustomUrl) but never calls cleanup(). Since createClient().cleanup() disconnects sockets/analytics, leaving it uncalled can leak resources across tests. Add a try/finally (or explicit cleanup at the end) to ensure the client is always cleaned up.
| test('should use relative URL for login redirect when appBaseUrl is not provided', () => { | ||
| // Create a client without appBaseUrl | ||
| const clientWithoutAppBaseUrl = createClient({ | ||
| serverUrl, | ||
| appId, | ||
| }); | ||
| const clientWithoutAppBaseUrl = createClient({ serverUrl, appId }); | ||
|
|
||
| // Mock window.location | ||
| const originalWindow = global.window; | ||
| const mockLocation = { href: '', origin: 'https://current-app.com' }; | ||
| global.window = { | ||
| location: mockLocation | ||
| }; | ||
| global.window = { location: mockLocation }; | ||
|
|
||
| const nextUrl = 'https://example.com/dashboard'; | ||
| clientWithoutAppBaseUrl.auth.redirectToLogin(nextUrl); | ||
| clientWithoutAppBaseUrl.auth.redirectToLogin('https://example.com/dashboard'); | ||
|
|
||
| // Verify the redirect URL uses a relative path (no appBaseUrl prefix) | ||
| expect(mockLocation.href).toBe( | ||
| `/login?from_url=${encodeURIComponent(nextUrl)}` | ||
| `/login?from_url=${encodeURIComponent('https://example.com/dashboard')}` | ||
| ); | ||
|
|
||
| // Restore window | ||
| global.window = originalWindow; | ||
| }); |
There was a problem hiding this comment.
This test creates an additional client (clientWithoutAppBaseUrl) but never calls cleanup(). Add explicit cleanup (ideally in a finally) to avoid leaking sockets/analytics state across the test suite.
| "eslint": "^9.39.2", | ||
| "eslint-plugin-import": "^2.32.0", | ||
| "nock": "^13.4.0", | ||
| "msw": "^2.12.11", |
There was a problem hiding this comment.
nock was removed from devDependencies, but the repo still contains at least one unit test that imports/uses nock (e.g. tests/unit/integrations.test.ts). This will break fresh installs/CI runs. Either migrate the remaining nock-based tests to MSW as well, or keep nock as a devDependency until the migration is complete.
| "msw": "^2.12.11", | |
| "msw": "^2.12.11", | |
| "nock": "^13.5.0", |
Decision summaryThis PR replaces Nock-style per-test responses with one reusable, stateful MSW Representative flow: platform.given.app(appId).entities.records("Todo", []);
const created = await base44.entities.Todo.create({ title: "New task" });
await expect(base44.entities.Todo.get(created.id)).resolves.toEqual(created);
await expect(base44.entities.Todo.list()).resolves.toContainEqual(created);Central handlers cover entities, auth/reset, functions, agents, integrations, Validation
The complete code/proof package and explicit backend-fidelity limits are in Scope and limitationsThe diff is limited to the MSW dependency/setup, centralized platform and Hosted test, lint, dependency audit, and package preview checks pass. The two Human review and acceptance remain outstanding. Do not merge automatically. |
|
Review remediation update for exact published head The previous ready recommendation is retracted. I audited all six open March 26 Copilot conversations individually. Each underlying finding is addressed in the current head, but I have not dismissed or resolved any conversation and do not infer acceptance:
Fresh focused proof at this head passes 5 files/92 tests (client, auth, custom integrations, core integrations and architecture guard); the published full proof remains 23 files/332 tests on default and Node 20. The scoped bridge does not expose GitHub's GraphQL-only thread-resolution mutation, so a maintainer must verify these links and click Resolve conversation for each genuinely addressed finding. The two red Claude jobs are policy failures before analysis, not source failures. Both reject bot actor No merge, review dismissal or human approval is claimed. The PR remains needs-work until all six conversations are resolved and both Claude checks are green. |
The SDK’s HTTP tests now run through MSW against the actual Axios/fetch clients. The migration preserves the current-main suite, enforces request/body/header/query expectations, and fails unmatched traffic even when SDK code catches the network error. The earlier migration omitted a Nock test file and weakened several body assertions; this update covers the current test surface and removes transport response stubs.
Stronger multipart assertions exposed a production bug:
functions.invoke(FormData)rebuilt an empty form and lost fields. A separate fix preserves caller-supplied FormData, including repeated keys, empty values and binary files. The old implementation fails the new wire assertions; the fixed implementation passes.Validation:
npm test.src, not the test files.tests/README.mddocuments adding handlers, request assertions, strict teardown, multipart inspection and the unit/live-E2E boundary. Goal c9/c10 evidence is prepared for human acceptance; this PR has not been merged.Review scope
The actual GitHub comparison has been verified against current main (
4ebcc76) and assessed head (1d574ef): 27 files, with matching file statuses and line counts. The earlier 92-file view used a stale March PR base; refreshing the base removed 65 unrelated paths without changing code.src/modules/functions.ts: preserve direct FormData (5 added/4 removed lines).The goal review package includes the declared scope, machine-checked PR diff, and review map. Human acceptance remains pending.