Skip to content

refactor(mock): emit strict mock types once from structured names - #3543

Merged
melloware merged 4 commits into
orval-labs:masterfrom
Hypenate:refactor/strict-mock-structured-emit
Jun 5, 2026
Merged

refactor(mock): emit strict mock types once from structured names#3543
melloware merged 4 commits into
orval-labs:masterfrom
Hypenate:refactor/strict-mock-structured-emit

Conversation

@Hypenate

@Hypenate Hypenate commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up refactor from strict mock types (#3525). Aligns the faker schemas: true path with MSW/operation mocks: generators only emit factories, and finalizeMockImplementation prepends strict helpers and {Schema}Mock aliases once from strictMockSchemaTypeNames.

  • generateFakerForSchemas returns strictMockSchemaTypeNames and no longer inlines strict type blocks
  • writeFakerSchemaMocks calls finalizeMockImplementation (same as other writers)
  • dedupeStrictMockTypeDeclarations only prepends from the structured set (removed regex scraping/strip logic)

Closes #3542

Test plan

  • @orval/mock unit tests
  • orval-tests build (mock client typecheck)
  • CI green on PR

Summary by CodeRabbit

  • Improvements

    • Faker mock generation now exposes strict schema type names separately, producing leaner mock implementations and enabling predictable header insertion and ordering.
  • Bug Fixes

    • Finalized mock output is used when computing imports and emitting files so generated files reflect finalized implementation.
  • New Features

    • Generated mock types and factory functions added for additional schemas (e.g., Status, PetList, Score).
  • Tests

    • Updated/added tests covering strict-type exposure, deduplication/header behavior, and finalized-output flow.

generateFakerForSchemas returns strictMockSchemaTypeNames instead of
inlining helpers and aliases; writeFakerSchemaMocks finalizes like
operation mocks. Remove regex scraping from dedupeStrictMockTypeDeclarations.

Closes #3542

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

generateFakerForSchemas no longer inlines strict-mock helper/type declarations; it returns an optional strictMockSchemaTypeNames array. Regex scraping helpers were removed, deduplication now uses structured names, writers accept the names for finalization, and tests/exports/spec snapshots were updated.

Changes

Strict Mock Schema Type Name Extraction

Layer / File(s) Summary
Faker generation refactor
packages/mock/src/faker/index.ts, packages/mock/src/faker/index.test.ts, tests/__snapshots__/mock/issue-3525/model/*
generateFakerForSchemas omits inline strict-mock helper/type declarations and returns strictMockSchemaTypeNames?: string[]; tests and generated snapshots updated to match new output shape.
Mock types utilities refactor
packages/mock/src/mock-types.ts, packages/mock/src/mock-types.test.ts
Removed regex-based scraping helpers (collectStrictMockSchemaTypeNames, collectStrictMockSchemaNamesFromUsage). dedupeStrictMockTypeDeclarations now uses options.strictSchemaTypeNames (deduped), early-returns when empty, trims body, and prepends the generated strict-mock header.
Integration, writers, exports, and specs
packages/orval/src/write-specs.ts, packages/mock/src/index.ts, tests/specifications/issue-3525.yaml, tests/__snapshots__/mock/issue-3525/*
writeFakerSchemaMocks captures strictMockSchemaTypeNames and optionally passes them to builder.finalizeMockImplementation; finalized implementation is used for dependency import generation and file emission. Public re-export for scraping helper removed; spec and snapshot files updated to include additional model modules.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • melloware

Poem

🐰 I hopped through code with tidy paws,

strict names now leave no hidden cause.
No regex dust, no duplicated art,
a cleaner file — a happier heart. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately summarizes the main refactoring: moving strict mock type emission from inline generation to a structured output that is finalized once at file write time.
Linked Issues check ✅ Passed All coding requirements from #3542 are met: generateFakerForSchemas returns strictMockSchemaTypeNames, inline helpers are removed, dedupeStrictMockTypeDeclarations uses structured names without regex, and non-overridable schemas are now included.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the stated objectives: refactoring strict mock type emission in faker/MSW paths, updating corresponding tests, and fixing the non-overridable schema bug. No unrelated changes detected.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@melloware melloware added the mock Related to mock generation label Jun 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/mock/src/faker/index.ts (1)

168-170: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Track strict names for every strict schema factory.

Line 168 only records typeName for overridable schemas, but getMockFactorySignatureParts() in packages/mock/src/mock-types.ts still returns {Schema}Mock for strict non-overridable factories. That means a top-level array/scalar/enum schema can generate getFooMock(): FooMock while writeFakerSchemaMocks() never receives Foo in strictSchemaTypeNames, so the finalizer will skip export type FooMock = … and the emitted file stops typechecking.

Suggested fix
-    if (isStrictMock(mockOptions) && isOverridable) {
+    if (isStrictMock(mockOptions)) {
       strictMockTypeNames.add(typeName);
     }

Please add a regression test for a strict non-object schema when fixing this.

🤖 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/mock/src/faker/index.ts` around lines 168 - 170, The code only adds
typeName to strictMockTypeNames when isOverridable is true, causing
non-overridable strict schemas to be omitted; update the logic in
packages/mock/src/faker/index.ts (around isStrictMock and isOverridable) to
always add typeName to strictMockTypeNames when isStrictMock(mockOptions) is
true, regardless of isOverridable, so writeFakerSchemaMocks and
strictSchemaTypeNames receive the strict names expected by
getMockFactorySignatureParts (which returns {Schema}Mock for strict
non-overridable factories); after fixing, add a regression test that creates a
strict non-object schema (e.g., top-level enum/array/scalar) and asserts the
emitted file includes the corresponding exported type alias (e.g., FooMock) so
the emitted types typecheck.
🤖 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.

Outside diff comments:
In `@packages/mock/src/faker/index.ts`:
- Around line 168-170: The code only adds typeName to strictMockTypeNames when
isOverridable is true, causing non-overridable strict schemas to be omitted;
update the logic in packages/mock/src/faker/index.ts (around isStrictMock and
isOverridable) to always add typeName to strictMockTypeNames when
isStrictMock(mockOptions) is true, regardless of isOverridable, so
writeFakerSchemaMocks and strictSchemaTypeNames receive the strict names
expected by getMockFactorySignatureParts (which returns {Schema}Mock for strict
non-overridable factories); after fixing, add a regression test that creates a
strict non-object schema (e.g., top-level enum/array/scalar) and asserts the
emitted file includes the corresponding exported type alias (e.g., FooMock) so
the emitted types typecheck.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d0b9435d-6b6a-4cf3-ae6d-12a08d809da1

📥 Commits

Reviewing files that changed from the base of the PR and between 1739cdf and eaa2714.

📒 Files selected for processing (6)
  • packages/mock/src/faker/index.test.ts
  • packages/mock/src/faker/index.ts
  • packages/mock/src/index.ts
  • packages/mock/src/mock-types.test.ts
  • packages/mock/src/mock-types.ts
  • packages/orval/src/write-specs.ts
💤 Files with no reviewable changes (1)
  • packages/mock/src/index.ts

Hypenate and others added 2 commits June 4, 2026 13:41
Co-authored-by: Cursor <cursoragent@cursor.com>
…ories

Strict enum/array/scalar schema factories return {Schema}Mock but were
omitted from strictMockSchemaTypeNames when not overridable.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Hypenate

Hypenate commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@wadakatu We merged the previous PR.
This PR contains the simplification you wanted earlier (I hope)
If you have time, can you review it?

@wadakatu

wadakatu commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review — LGTM ✅

Beyond being a clean refactor, this actually fixes a latent TS2304 bug for non-overridable strict faker schemas. I regenerated and typechecked locally against a spec with top-level enum/array/scalar schemas.

The one real behavioral change: dropping && isOverridable (= a latent bug fix)

getMockFactorySignatureParts returns ${Type}Mock as the return type even for non-overridable schemas (top-level enum/array/scalar) under strict mode, but on master the guard means the XMock type declaration is never emitted:

  • master: error TS2304: Cannot find name 'StatusMock' / 'PetListMock' / 'ScoreMock' → doesn't compile
  • this PR: the same spec typechecks ✅

Note that keeping the guard and only doing the refactor would have left this just as broken as master, so commit 3 is an intentional, standalone bug fix rather than a side effect of the refactor. Dropping the guard makes "every XMock referenced by a strict factory" line up exactly with strictMockSchemaTypeNames, which is why removing the scraping safety nets (collectStrictMockSchemaNamesFromUsage, etc.) is safe here. The MSW path is sound for the same reason: applyStrictMockReturnType and strictMockSchemaTypeNames both derive from the same source (getSchemaTypeNamesFromResponses), so completeness is guaranteed by construction.

Suggestions (non-blocking)

  1. [Recommended] Add an e2e fixture for the non-overridable strict faker case. This bug fix isn't guarded by the current CI gates (snapshot / typecheck-generated): the existing strict fixtures (issue-3525 family) only contain Pet (an object = overridable). Adding a top-level enum + array schema to the issue-3525 spec would lock the regression down in CI.
  2. dedupeStrictMockTypeDeclarations is no longer idempotent (it prepends unconditionally without stripping an existing header). A one-line comment noting the "called once per file" invariant would make that safe to rely on.
  3. Nit: .replace(/^\n+/, '').trimStart().trimStart() alone already covers leading newlines. And the header ? ... : trimmedBody false branch is effectively dead since we early-return when there are no names.

Nice cleanup 👍

Add Status, PetList, and Score schemas to the strict fixture so CI
typechecks StatusMock/PetListMock/ScoreMock emission. Document the
once-per-file finalize invariant and simplify prepend logic.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Hypenate

Hypenate commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@wadakatu I summon you again :D
I've implemented the 3 suggestions you gave.

@wadakatu wadakatu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three are addressed cleanly — thanks for the quick turnaround! 🙌

Verified locally on the latest commit:

  • Suggestion 1 (fixture): regenerated issue-3525 and the new Status / PetList / Score schemas now emit StatusMock / PetListMock / ScoreMock and typecheck (incl. the odd-but-valid ScoreMock over number). The non-overridable strict-faker path is now locked down by both the snapshot and typecheck-generated gates.
  • Suggestion 2 (invariant): the "not idempotent / call once per file" note matches how the writers and writeFakerSchemaMocks use it. 👍
  • Suggestion 3 (cleanup): ${header}\n\n${implementation.trimStart()} is output-equivalent (header is always non-empty once we're past the empty-names early return, and .trimStart() subsumes the old .replace(/^\n+/, '')).

Full check: @orval/mock units (219) green, snapshot suite green after a fresh regen, and tsc --noEmit over all of generated/mock is clean — no output changes anywhere outside issue-3525, so no regressions.

LGTM 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mock Related to mock generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(mock): emit strict mock types once from structured names only

3 participants