-
Notifications
You must be signed in to change notification settings - Fork 617
feat(cli): Unified mock format and migration support #5183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): Unified mock format and migration support #5183
Conversation
🤖 My Senior Dev — Analysis Complete👤 For @agusayerza📁 Expert in View your contributor analytics → 📊 7 files reviewed • 1 high risk • 5 need attention 🚨 High Risk:
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
024943e to
07a1f7c
Compare
07a1f7c to
6cead33
Compare
6cead33 to
70da92c
Compare
70da92c to
a9b81ed
Compare
a9b81ed to
dec42a9
Compare
dec42a9 to
f3ac9c1
Compare
|
|
||
| let requestData = call.requestIdentity.data; | ||
| if (typeof requestData === 'string') { | ||
| requestData = JSON.parse(requestData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the parsing throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might, if the request isn't json. I have wrapped it with a try/catch
| import { parse } from './config.service.js'; | ||
| import { DiagnosticsMonitor, formatDiagnostics } from './diagnostics-monitor.service.js'; | ||
| import { loadSchemaJson } from './model.service.js'; | ||
| import * as responseSaver from './response-saver.service.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the 'response-saver.service.ts` file be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
|
|
||
| private computeRequestIdentity(config: AxiosRequestConfig): { requestIdentity: RequestIdentity; requestIdentityHash: string } { | ||
| const method = config.method?.toUpperCase() || 'GET'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous logic was using lowercase. Isn't using uppercase changing the identity hash and making it incompatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, although it wouldn't really matter as this is for new test generations, and when parsing the request at test runtime, we still LowerCase it. Regardless, I changed it back to use lowercase as there is no reason to modify our behaviour.
Updated the action implementation guide to fix a typo in the code example, changing '<ATION-NAME>' to '<ACTION-NAME>''
f3ac9c1 to
97b8043
Compare
97b8043 to
be82deb
Compare
be82deb to
4a75028
Compare
4a75028 to
8e60791
Compare
8e60791 to
9ed643c
Compare
TBonnin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested the performance with a huge test.json file?
|
|
||
| public onAxiosRequestFulfilled(response: AxiosResponse, connectionId: string): AxiosResponse { | ||
| // Handle getConnection/getMetadata calls | ||
| if (response.request.path.includes(`/connections/${connectionId}`)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the string doesn't match the comment.
| try { | ||
| requestData = JSON.parse(requestData); | ||
| } catch { | ||
| // ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we output a warning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a warning, this shouldn't happen but better to be safe than sorry
| if (nangoInstance instanceof NangoSyncCLI) { | ||
| const logMessages = nangoInstance.logMessages; | ||
| if (logMessages && logMessages.messages.length > 0) { | ||
| // ... (rest of the logging logic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI doing AI stuff. Good catch
|
|
||
| import { AxiosError } from 'axios'; | ||
| import chalk from 'chalk'; | ||
| import promptly from 'promptly'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is promptly used anywhere else? if not let's remove it from package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still being used
packages/cli/lib/testMocks/utils.ts
Outdated
| } | ||
| } | ||
|
|
||
| throw new Error(`No mock found for ${method.toUpperCase()} ${endpoint} (normalized: ${normalizedEndpoint}) with hash ${requestIdentityHash}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we can simplify this piece of code with some early return and utility function.
const apiMock = this.mockData.api?.[method.toUpperCase()]?.[normalizedEndpoint]
|| this.mockData.api?.[method.toUpperCase()]?.[`/${normalizedEndpoint}`];
if (!apiMock) { throw ... } ;
if (!Array.isArray(apiMock)) return apiMock;
const getFieldsMatch = (expected, actual, opts: { caseSensitive: bool } = {caseSensitive: true}) {
return Object.entries(expected).every(([key, value]) => {
const actualField = actual.find(([k]) =>
opts.caseSensitive ? k.toLowerCase() === key.toLowerCase() : k === key
);
return actualField && String(actualField[1]) === String(value);
});
}
// algo: hash > params > headers > data
const matchedMock = apiMock.find((mock) => {
if (mock.hash === requestIdentityHash) return true;
if (!mock.request) return false;
if (mock.request.params &&
!fieldsMatch(mock.request.params, identity.requestIdentity.params, { caseSensitive: false})) {
return false;
}
if (mock.request.headers &&
!fieldsMatch(mock.request.headers, identity.requestIdentity.headers)) {
return false;
}
if (mock.request.data !== undefined) {
const expectedDataIdentity = computeDataIdentity({ data: mock.request.data });
if (expectedDataIdentity !== identity.requestIdentity.data) {
return false;
}
}
return true;
});
return matchedMock;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Set the `MIGRATE_MOCKS` environment variable to `true` and run your tests: | ||
|
|
||
| ```bash | ||
| MIGRATE_MOCKS=true npm test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about something like MOCKS_MIGRATION=2026-01 or similar. In case we need to migrate again in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, much better
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why cannot we re-used the pagination logic?
|
|
||
| private getNextPageLinkFromBodyOrHeaders(linkPagination: LinkPagination, response: MockResponse, paginationConfig: Pagination) { | ||
| if (linkPagination.link_rel_in_response_header) { | ||
| const linkHeader = parseLinksHeader(response.headers['link']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need an extra dependency for parsing the link header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its already being used in the runners, I think it makes sense to use the same dependency to keep it consistent. This bugged pagination logic is slated to be removed once we migrate all tests on integration-templates
| 1. Run your tests using the old mock files. | ||
| 2. Intercept all mock data accessed during the test run. | ||
| 3. Save the data into a new `.test.json` file. | ||
| 4. The old mock directory can then be safely deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name the old mock directory to make it super clear what can be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
9ed643c to
dadeda5
Compare
dadeda5 to
f4c6207
Compare
Yes, actually I tested it on the whole |
f4c6207 to
0e6e8f5
Compare
TBonnin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @agusayerza
This PR introduces a unified structure for test mocks, consolidating scattered JSON files into a single
.test.jsonfile per test suite.Changes
Unified Mock Format
nango dryrun --saveto generate the new unified mock formatResponseCollectorto aggregate API calls during dry runsNangoActionMockandNangoSyncMockinnango/testto support the new formatMigration Path
MIGRATE_MOCKS=2026-01 npm testrecords legacy mock usage and generates the new unified file automaticallyPagination Bug Fix
The legacy test utilities had a bug where pagination would stop after the first page, causing:
Fixes implemented:
requestIdentity.paramsdirectlyDocumentation
nango dryrun <sync> <connection> --saveNote: Follow-up needed in integration-templates repo to update
vitest.setup.tsto import fromnango/testinstead of using local mock implementations.NAN-4463
Testing Instructions
nango dryrun <name> <connectionId> --saveand verify a single.test.jsonis creatednpm testto verify tests pass with the new formatMIGRATE_MOCKS=2026-01 npm testand verify the new mock file is generatedThe unified fixtures capture complete request and response metadata—including connection context and proxied traffic—so the CLI helpers can replay dry runs, manage legacy-to-unified migration inline, and actively close the long-standing pagination gap.
Affected Areas
•
packages/cli/lib/services/response-collector.service.ts•
packages/cli/lib/services/dryrun.service.ts•
packages/cli/lib/testMocks/utils.ts•
docs/implementation-guides/building-integrations/testing.mdx•
packages/cli/package.jsonThis summary was automatically generated by @propel-code-bot