fix(mock): emit strict mock types in tags-split MSW+faker output - #3575
Conversation
Preserve strictMockSchemaTypeNames in tag targets. Scope array-item factory dedup per mock generator file. Fixes #3574 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds output-type-aware mock generation ( ChangesStrict Mock Types in Tags-Split Output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/writers/target-tags.ts (1)
50-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve accumulated
strictMockSchemaTypeNameswhen cloning merge state.At Line 50, the accumulator clone drops previously collected
strictMockSchemaTypeNames, so only later operations’ names survive. That can omit required strict aliases in finalized per-tag mock files.Proposed fix
function mergeOperationMockOutputs( accMockOutputs: GeneratorMockOutputFull[], opMockOutputs: GeneratorMockOutputFull[], ): GeneratorMockOutputFull[] { const result: GeneratorMockOutputFull[] = accMockOutputs.map((m) => ({ type: m.type, implementation: { ...m.implementation }, imports: [...m.imports], + strictMockSchemaTypeNames: m.strictMockSchemaTypeNames + ? [...m.strictMockSchemaTypeNames] + : undefined, }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/writers/target-tags.ts` around lines 50 - 69, The clone of accumulated mock outputs (result built from accMockOutputs) is dropping existing strictMockSchemaTypeNames so earlier collected names are lost; update the mapping of accMockOutputs to include cloning of strictMockSchemaTypeNames (e.g., copy m.strictMockSchemaTypeNames into the new object) so GeneratorMockOutputFull entries preserve that array when creating result, and keep the existing merge logic in the opMockOutputs loop (references: result, accMockOutputs, opMockOutputs, emptyMockOutputFull, strictMockSchemaTypeNames, GeneratorMockOutputFull).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/api-generation.spec.ts`:
- Around line 35-45: The test is using the non-fetch fixture name
'issue-3574-strict-mock-tags-split' which should point to the fetch fixture;
update the fixture identifier passed to the generated(...) calls (specifically
the petsMsw and petsFaker variable initializations) from
'issue-3574-strict-mock-tags-split' to 'issue-3574-strict-mock-tags-split-fetch'
so the regression test targets the fetch fixture directory.
---
Outside diff comments:
In `@packages/core/src/writers/target-tags.ts`:
- Around line 50-69: The clone of accumulated mock outputs (result built from
accMockOutputs) is dropping existing strictMockSchemaTypeNames so earlier
collected names are lost; update the mapping of accMockOutputs to include
cloning of strictMockSchemaTypeNames (e.g., copy m.strictMockSchemaTypeNames
into the new object) so GeneratorMockOutputFull entries preserve that array when
creating result, and keep the existing merge logic in the opMockOutputs loop
(references: result, accMockOutputs, opMockOutputs, emptyMockOutputFull,
strictMockSchemaTypeNames, GeneratorMockOutputFull).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a86c2c88-0f6b-4165-9b98-a714b3addbe5
📒 Files selected for processing (23)
packages/core/src/types.tspackages/core/src/writers/target-tags.tspackages/mock/src/faker/getters/array-item-factory.test.tspackages/mock/src/faker/getters/array-item-factory.tspackages/mock/src/index.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/index.faker.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/index.msw.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/pets/pets.faker.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/pets/pets.msw.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/pets/pets.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/schemas/index.faker.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/schemas/index.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/schemas/pet.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/index.faker.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/index.msw.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/pets/pets.faker.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/pets/pets.msw.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/pets/pets.service.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/schemas/index.tstests/__snapshots__/mock/issue-3574-strict-mock-tags-split/schemas/pet.tstests/api-generation.spec.tstests/configs/mock.config.tstests/specifications/issue-3574-strict-mock-tags-split.yaml
Use issue-3574-strict-mock-tags-split-fetch for the focused assertion. Allow getPetMock via import when schemas: true delegates to index.faker. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Can you verify the fix? This PR should fix this. |
|
Thanks for the fix @Hypenate. I verified the per-generator file scoping (B) and the first-op / wrapped-output preservation (A) — both work. But there's a remaining drop point in (A), and I think it still leaves the reporter's real-world case broken. Residual bug:
|
Test coverage gap (separate from the residual bug above)The regression fixtures ( To actually guard the merge path, I'd suggest a fixture where one tag has ≥2 operations returning distinct schemas, e.g. a |
Copy strictMockSchemaTypeNames in mergeOperationMockOutputs. Add multi-schema Store tag fixture and regression test for #3574. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu Applied your comments |
Scan generated implementation for MockWithNullableOverrides and SchemaMock refs. Merge with response schema names so nested helpers get type declarations. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop prefix captures when a longer nameMock schema was also collected. Fixes Widget extracted from WidgetMock schema references. Co-authored-by: Cursor <cursoragent@cursor.com>
|
LGTM — thanks for the quick turnaround, and nice catch going past my two notes to the actual nested-factory case. I rebuilt the PR at b490ff4 and verified:
🚢 from my side. |
|
Two non-blocking nits for a possible follow-up — feel free to ignore for this PR:
|
|
I'll do a follow-up PR with your remarks when the PR from @kfrederix has been merged. |
Summary
strictMockSchemaTypeNameswhen building tag targets intags-splitmode sofinalizeMockImplementation/dedupeStrictMockTypeDeclarationsprependsKeysWithNull,MockWithNullableOverrides, and{Schema}Mockaliases in per-tag.msw.tsand.faker.tsfiles.mswvsfaker) so faker output defines schema factories (e.g.getPetMock) it references instead of relying on the MSW file.api-generation.spec.ts.Fixes #3574
Test plan
bun testin@orval/mock(includes new array-item factory scope test)bun run generate:mockforissue3574StrictMockTagsSplitAngularandissue3574StrictMockTagsSplitFetchtests/__snapshots__/mock/issue-3574-*node tests/scripts/typecheck-generated.mjspasses (mock folder includes new generated output)issue-3525/issue-3525-multi/issue-3525-widget-mocksnapshots unchangedSummary by CodeRabbit
New Features
Tests