refactor(test): centralize integration test mocks #421

agusayerza · 2025-12-11T13:52:19Z

Describe your changes

Refactoring of how integration test mocks are managed. We are moving from a scattered, directory-based mock structure to a centralized, single-file approach using .test.json files co-located with their corresponding tests.

Key Changes:

Centralized Mocks: A new *.test.json file now holds all mock data (input, output, API calls, etc.) for a single test, making tests easier to read and maintain.
New Fixture Providers: We've refactored NangoActionMock to use a strategy pattern for loading mocks (now called "Fixture Providers"), ensuring full backward compatibility while enabling the new system.
- LegacyFixtureProvider: For old tests.
- UnifiedFixtureProvider: For new, centralized .test.json files.
- RecordingFixtureProvider: A new utility to automatically migrate old tests to the new format.
Async Loading: The mock loading process is now asynchronous to avoid synchronous I/O operations, with getFixtureProvider returning a promise.
Strict Error Handling: The new provider enforces strict error handling for missing critical mock data (like batchSave or output), matching the behavior of the legacy system.
Automated Migration: A new workflow allows developers to automatically generate the new .test.json files by running existing tests with a MIGRATE_MOCKS=true environment variable.

This new structure improves the developer experience by making test data easier to manage and reason about, and it provides a simple, automated path for migrating existing tests.

Issue ticket number and link

NAN-4463

Checklist before requesting a review (skip if just adding/editing APIs & templates)

I added tests, otherwise the reason is: This is a refactoring of the testing framework itself.
External API requests have retries
Pagination is used where appropriate
The built in nango.paginate call is used instead of a while (true) loop
Third party requests are NOT parallelized (this can cause issues with rate limits)
If the sync requires metadata the nango.yaml has auto_start: false
If the sync is a full sync then track_deletes: true is set
I followed the best practices and guidelines from the Writing Integration Scripts doc

Migration Guide

We have made migration as automated as possible. Follow these steps to migrate an integration to the new structure.

Step 1: Ensure Tests Pass

Before starting, make sure the existing tests for the integration are passing using the legacy mocks.

npx vitest run integration-templates/integrations/<integration-name>/tests/

Step 2: Run with Migration Mode

Run the tests again with the MIGRATE_MOCKS environment variable set to true. This will trigger the RecordingFixtureProvider, which will execute the tests against the old mocks and generate the new .test.json files.

MIGRATE_MOCKS=true npx vitest run integration-templates/integrations/<integration-name>/tests/

Note: You will see new *.test.json files appear next to your *.test.ts files.

Step 3: Verify and Clean Up

Run Tests Again: Run the tests without the environment variable. The system should now automatically pick up the new .test.json files and use the UnifiedFixtureProvider.
```
npx vitest run integration-templates/integrations/<integration-name>/tests/
```
Verify: Ensure all tests pass.
Delete Old Mocks: Once confirmed, you can safely delete the old mock folders for the migrated tests from the mocks/ directory.

Step 4: Troubleshooting

If a test fails after migration:

Check the JSON: Open the generated .test.json and verify the data looks correct.
API Matching: If an API call isn't matching, check the api section. The loader uses subset matching, so ensure the critical parameters in the request object match what the code is sending.
Re-record: You can delete the generated .test.json and re-run Step 2 to re-record the interaction.

Step 5: Commit

Commit the new .test.json files and the removal of the old mocks/ directories.

linear · 2025-12-11T13:52:23Z

NAN-4463 Simplify mock file structure in the Nango integrations folder

coderabbitai · 2025-12-11T13:52:32Z

Important

Review skipped

More than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review.

41 files out of 148 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch agus/NAN-4463/mock-structure-refactor

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

my-senior-dev-pr-review · 2025-12-11T13:56:31Z

🤖 My Senior Dev

150 files reviewed • 3 high risk • 50 need attention

🚨 High Risk:

integrations/hubspot/tests/hubspot-fetch-properties.test.json — Contains a large number of new test cases that could expose sensitive data.
integrations/hubspot/tests/hubspot-delete-contact.test.json — Introduces a new test for contact deletion with potential security implications.

⚠️ Needs Attention:

integrations/.nango/schema.json — Modified schema file; important for ensuring test alignments.
integrations/hubspot/tests/hubspot-companies.test.json — New test cases for company functionality that require careful validation.
+2 more concerns...

View full analysis →

💬 Try: @my-senior-dev explain this change or summon @chaos-monkey 🐵 @security-auditor 🔒 @optimizer ⚡ for different perspectives

📖 All commands & personas

You can interact with me by mentioning @my-senior-dev in any comment:

In PR comments or on any line of code:

Ask questions about the code or PR
Request explanations of specific changes
Get suggestions for improvements

Slash commands:

/help — Show all available commands
/archeology — See the history and evolution of changed files
/profile — Performance analysis and suggestions
/expertise — Find who knows this code best
/personas — List all available AI personas

AI Personas (mention to get their perspective):

Persona	Focus
`@chaos-monkey` 🐵	Edge cases & failure scenarios
`@skeptic` 🤨	Challenge assumptions
`@optimizer` ⚡	Performance & efficiency
`@security-auditor` 🔒	Security vulnerabilities
`@accessibility-advocate` ♿	Inclusive design
`@junior-dev` 🌱	Simple explanations
`@tech-debt-collector` 💳	Code quality & shortcuts
`@ux-champion` 🎨	User experience
`@devops-engineer` 🚀	Deployment & scaling
`@documentation-nazi` 📚	Documentation gaps
`@legacy-whisperer` 🏛️	Working with existing code
`@test-driven-purist` ✅	Testing & TDD

For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews.

TBonnin · 2025-12-11T17:41:28Z

vitest.setup.ts

+    if (process.env.MIGRATE_MOCKS) {
+        const legacyLoader = new LegacyMockLoader(dirname, name);
+        return new RecordingMockLoader(legacyLoader, unifiedMockPath);
+    }


not sure I follow why a runtime migration is required?

It's just a dev utility to help migrate old tests to the new format. Added a comment to clarify

is the migration idempotent?

Migration itself is not idempotent, but restoring from an unsuccessful migration is as simple as deleting the new mock file (as we do not delete the old mocks, an the migration guide will state that a clean, successful run, should be tested before deleting those)

Actually, it should be idempotent as well, as we record everything the test require and create the new file once that is all done, so unless saving the file fails mid-write (could happen, but it would be extremely rare I think), the migration is idempotent

TBonnin · 2025-12-11T17:46:35Z

vitest.setup.ts

+        } catch (error) {
+            if (throwOnMissing) {
+                throw new Error(`Failed to load mock data from ${filePath}: ${error.message} ${identity ? JSON.stringify(identity, null, 2) : ''}`);
+            }


what do we return when throwOnMissing is false?

Fixed. It now explicitly returns undefined when throwOnMissing is false, matching the new provider's behavior

TBonnin · 2025-12-11T17:48:33Z

vitest.setup.ts

+
+        if (!apiMock) {
+            apiMock = this.mockData.api[method.toUpperCase()]?.[`/${normalizedEndpoint}`];
+        }


why the double attempt? are we not controlling the endpoint format

It handles endpoints stored with or without leading slashes (legacy vs new recordings). When testing migrating multiple of the existing tests this was a recurring issue. It seems like we don't have it standarized. Added a comment to clarify.

TBonnin · 2025-12-11T17:51:20Z

vitest.setup.ts

+                }
+            }
+        }
+    } catch (e) {}


can you explain what the stack trace parsing is for?

It auto-detects the test filename (e.g., foo.test.ts) to find the matching JSON mock (foo.test.json). Added a comment

TBonnin · 2025-12-11T17:54:52Z

vitest.setup.ts

+    let unifiedFileExists = false;
+    try {
+        // @ts-ignore
+        const fsSync = require('fs');


doing sync IO in what looks like a constructor is surprising. any reason to not make getMockLoader async?

Refactored getFixtureProvider to be async. The constructor now kicks off the promise, and methods await it. Originally I kept the whole constructor synchronized so there was no signature change. Initially I had an asycn factory method, but I was worried the sync constructor would be used. Kicking off the promise at the constructor and making the already async methods wait for that promise was something I hadn't thought at the time

TBonnin · 2025-12-11T17:56:31Z

vitest.setup.ts

+
+    async getBatchSaveData(modelName: string) {
+        await this.loadMockFile();
+        return this.mockData.nango?.batchSave?.[modelName];


are we ok returning undefined if data doesn't exist?

Good catch. Updated to throw errors for missing critical data (like batchSave), matching the legacy behavior

TBonnin · 2025-12-11T18:01:30Z

vitest.setup.ts

+
+    constructor(private mockFilePath: string) {}
+
+    private async loadMockFile() {


does it bring anything to load the data dynamically instead of when instantiating the loader?

You're right, it was overcomplicated. Refactored to load data once in the constructor

TBonnin · 2025-12-11T18:03:34Z

vitest.setup.ts

 import get from 'lodash-es/get.js';
 import type { Pagination, CursorPagination, LinkPagination, OffsetPagination, OffsetCalculationMethod } from '@nangohq/types';

+interface MockLoaderStrategy {


what about FixtureProvider and then LegacyFixtureProvider, ....

Yes, much better

TBonnin · 2025-12-16T01:21:53Z

vitest.setup.ts

    public async getBatchSaveData(modelName: string) {
-        const data = await this.getMockFile(`${this.name}/${modelName}/batchSave`, true);
-        return data;
+        return (await this.fixtureProvider).getBatchSaveData(modelName);


I am not a huge fan of awaiting the same promise but for a utility feature, why not. This or implementing some sort of Factory function

Agreed. Originally I had a factory function, but it would force migration to that factory method. I followed the same reasoning, this a utility so it doesn't hurt that much.

- Make fixture loading async to avoid sync I/O - Enforce strict error handling for missing mock data - Rename strategies to FixtureProviders - Add types for mock data structures

agusayerza force-pushed the agus/NAN-4463/mock-structure-refactor branch from f97f346 to fc098c1 Compare December 11, 2025 13:56

TBonnin requested review from a team and removed request for a team December 11, 2025 17:32

agusayerza requested a review from a team December 11, 2025 17:41

TBonnin reviewed Dec 11, 2025

View reviewed changes

agusayerza force-pushed the agus/NAN-4463/mock-structure-refactor branch 2 times, most recently from f6d8df4 to 9104300 Compare December 15, 2025 22:23

TBonnin approved these changes Dec 16, 2025

View reviewed changes

agusayerza added 4 commits December 16, 2025 15:25

refactor(test): centralize integration test mocks

67cabc1

Hubspot migration

0a1e830

refactor: async fixture loading and strict types

b9b2362

- Make fixture loading async to avoid sync I/O - Enforce strict error handling for missing mock data - Rename strategies to FixtureProviders - Add types for mock data structures

Version bump

c4f2759

agusayerza force-pushed the agus/NAN-4463/mock-structure-refactor branch from 9104300 to c4f2759 Compare December 16, 2025 18:26

agusayerza added 2 commits December 16, 2025 15:45

Nango compile

467f373

Nango version bump

6d65006

agusayerza merged commit 839857b into main Dec 16, 2025
5 checks passed

agusayerza deleted the agus/NAN-4463/mock-structure-refactor branch December 16, 2025 20:02


		constructor(private mockFilePath: string) {}

		private async loadMockFile() {

refactor(test): centralize integration test mocks #421

refactor(test): centralize integration test mocks #421

Uh oh!

Conversation

agusayerza commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue ticket number and link

Checklist before requesting a review (skip if just adding/editing APIs & templates)

Migration Guide

Step 1: Ensure Tests Pass

Step 2: Run with Migration Mode

Step 3: Verify and Clean Up

Step 4: Troubleshooting

Step 5: Commit

Uh oh!

linear bot commented Dec 11, 2025

Uh oh!

coderabbitai bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

my-senior-dev-pr-review bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 My Senior Dev

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TBonnin Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

agusayerza commented Dec 11, 2025 •

edited

Loading

coderabbitai bot commented Dec 11, 2025 •

edited

Loading

my-senior-dev-pr-review bot commented Dec 11, 2025 •

edited

Loading

TBonnin Dec 16, 2025 •

edited

Loading