feat(mock): add arrayItems faker option for reusable array item mocks - #3514
Conversation
Expose exported get<X>Mock factories for object-like array item schemas in operation responses so consumers can reuse item-level fakers outside of full response mocks. Closes #3513. 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:
📝 WalkthroughWalkthroughAdds an opt-in Faker generator option ChangesArray Item Mock Factories
Sequence Diagram(s)sequenceDiagram
participant getMockScalar
participant shouldExtractArrayItemFactories
participant extractArrayItemMock
participant getArrayItemFactoryNames
participant splitMockImplementations
getMockScalar->>shouldExtractArrayItemFactories: is extraction enabled?
shouldExtractArrayItemFactories-->>getMockScalar: boolean
getMockScalar->>extractArrayItemMock: attempt extraction(mapValue, items, context)
extractArrayItemMock->>getArrayItemFactoryNames: derive factory/type names
getArrayItemFactoryNames-->>extractArrayItemMock: names
extractArrayItemMock->>splitMockImplementations: append factory impl (if new)
extractArrayItemMock-->>getMockScalar: return `{...<factory>()}` or undefined
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/__snapshots__/mock/faker-array-items/endpoints.ts (1)
10-14: 💤 Low valueConsider consolidating type imports in the generator.
Both import statements pull types from
./modeland could be merged into a single import line. While not incorrect, consolidating would slightly improve readability of the generated output.♻️ Suggested consolidation (for generator logic)
-import type { GetTenants200, TenantListResponse } from './model'; - import { faker } from '`@faker-js/faker`'; - -import type { GetTenants200ValueItem, TenantResponseModelDto } from './model'; +import type { + GetTenants200, + GetTenants200ValueItem, + TenantListResponse, + TenantResponseModelDto +} from './model';🤖 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 `@tests/__snapshots__/mock/faker-array-items/endpoints.ts` around lines 10 - 14, Consolidate the duplicate type imports from './model' into a single import statement: merge GetTenants200, TenantListResponse, GetTenants200ValueItem, and TenantResponseModelDto into one `import type { ... } from './model'` so the two separate import lines are replaced by a single consolidated import (update the lines that currently import GetTenants200, TenantListResponse and the separate import of GetTenants200ValueItem, TenantResponseModelDto).tests/__snapshots__/mock/faker-array-items/model/tenantListResponse.ts (1)
9-12: 💤 Low valueMinor: Inconsistent use of
interfacevstypein generated models.This file uses
interfacewhilegetTenants200.tsusestypefor structurally similar response shapes. For consistency, consider having the generator use a uniform declaration style across similar schema patterns.🤖 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 `@tests/__snapshots__/mock/faker-array-items/model/tenantListResponse.ts` around lines 9 - 12, The generated model uses an interface (TenantListResponse) while similar response shapes (e.g., the shape emitted for getTenants200) use a type alias; update the generator that emits TenantListResponse so it produces a type declaration instead of an interface (or vice‑versa across both generators) to make declarations uniform—locate the generator logic that formats array/object response schemas and change the emission rule for TenantListResponse (and other list responses) to use a consistent "type" form matching getTenants200.
🤖 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 `@packages/mock/src/faker/getters/array-item-factory.ts`:
- Around line 52-54: isAlreadyFactoryCall currently uses an unanchored regex
that falsely matches nested factory calls; update the regex in
isAlreadyFactoryCall(mapValue: string) to anchor the match to the entire string
(start/end) and allow only an optional object spread wrapper around the call
(e.g. optional surrounding "{...}" with whitespace) so it only returns true when
mapValue is exactly a delegating spread like `{...getOwnerMock()}` or the plain
call `getOwnerMock()` and not when the call appears nested inside other text.
- Around line 144-160: array-item-factory.ts can push an exported factory
(splitMockImplementations.push(func)) that duplicates the unconditional export
emitted by faker/index.ts (factories), causing duplicate `export const
getXxxMock` declarations when hasOverrideTouchingSchema(...) suppressed
delegation; fix by checking the global set of factory names before adding the
split implementation: when preparing to push func, test whether the target
factory name (factoryName / `get${pascal(name)}Mock`) already exists in the
higher-level factories list or localFactoryNames and skip pushing if present (or
add the name to localFactoryNames and ensure faker/index.ts honors it), so only
one `export const get...Mock` is ever emitted.
---
Nitpick comments:
In `@tests/__snapshots__/mock/faker-array-items/endpoints.ts`:
- Around line 10-14: Consolidate the duplicate type imports from './model' into
a single import statement: merge GetTenants200, TenantListResponse,
GetTenants200ValueItem, and TenantResponseModelDto into one `import type { ... }
from './model'` so the two separate import lines are replaced by a single
consolidated import (update the lines that currently import GetTenants200,
TenantListResponse and the separate import of GetTenants200ValueItem,
TenantResponseModelDto).
In `@tests/__snapshots__/mock/faker-array-items/model/tenantListResponse.ts`:
- Around line 9-12: The generated model uses an interface (TenantListResponse)
while similar response shapes (e.g., the shape emitted for getTenants200) use a
type alias; update the generator that emits TenantListResponse so it produces a
type declaration instead of an interface (or vice‑versa across both generators)
to make declarations uniform—locate the generator logic that formats
array/object response schemas and change the emission rule for
TenantListResponse (and other list responses) to use a consistent "type" form
matching getTenants200.
🪄 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: cd1e7a78-799e-4daa-8f88-dd2b099b3f8d
📒 Files selected for processing (15)
docs/content/docs/guides/faker.mdxpackages/core/src/types.tspackages/mock/src/faker/getters/array-item-factory.test.tspackages/mock/src/faker/getters/array-item-factory.tspackages/mock/src/faker/getters/object.tspackages/mock/src/faker/getters/scalar.tspackages/mock/src/types.tstests/__snapshots__/mock/faker-array-items/endpoints.tstests/__snapshots__/mock/faker-array-items/model/getTenants200.tstests/__snapshots__/mock/faker-array-items/model/getTenants200ValueItem.tstests/__snapshots__/mock/faker-array-items/model/index.tstests/__snapshots__/mock/faker-array-items/model/tenantListResponse.tstests/__snapshots__/mock/faker-array-items/model/tenantResponseModelDto.tstests/configs/mock.config.tstests/specifications/faker-array-items.yaml
melloware
left a comment
melloware
left a comment
There was a problem hiding this comment.
Looking like linter issue
Fix eslint issues, tighten factory-call detection, skip extraction when schemas: true already emits consolidated factories, and add regression tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Partial test contexts omitted mock, causing runtime failures when extractArrayItemMock runs during array scalar tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
cc @wadakatu |
Merge mock-only schema types into the main import pass so single-file outputs do not emit duplicate import type lines from the same module. Co-authored-by: Cursor <cursoragent@cursor.com>
Refresh endpoint snapshots after single-mode mock import consolidation. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop orphaned v8.13.0 snapshot files with no master config or generated source. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the ping @melloware — took a look at the mock side. Nice feature 👍 One correctness issue though: the Repro ( paths:
/tenants-a:
get:
operationId: getTenantsA
responses:
'200':
description: ok
content:
application/json:
schema: { $ref: '#/components/schemas/TenantListResponse' }
/tenants-b:
get:
operationId: getTenantsB
responses:
'200':
description: ok
content:
application/json:
schema: { $ref: '#/components/schemas/TenantListResponse' }
components:
schemas:
TenantResponseModelDto:
type: object
required: [id, name]
properties: { id: { type: string }, name: { type: string } }
TenantListResponse:
type: object
properties:
value: { type: array, items: { $ref: '#/components/schemas/TenantResponseModelDto' } }
count: { type: integer }Generated output: // from getTenantsA
export const getTenantResponseModelDtoMock = (...) => ({ ... });
export const getGetTenantsAResponseMock = (...) => ({ value: [...].map(() => ({ ...getTenantResponseModelDtoMock() })), ... });
// from getTenantsB — same name again
export const getTenantResponseModelDtoMock = (...) => ({ ... });
export const getGetTenantsBResponseMock = (...) => ({ ... });Root cause: the dedup guard in Scope:
The current |
|
One more thing — this one isn't a functional bug but a scope question. The Most of that is harmless consolidation, but a few outputs now get spurious unused imports. The new filter matches an import name (and alias) against the combined Example 1 — import { HttpClient, HttpResponse as AngularHttpResponse } from '@angular/common/http';
Example 2 — import { ..., map } from 'rxjs';The rxjs CI stays green because the snapshot tests are string comparisons (no Since this import consolidation doesn't seem strictly required for |
Track factory names on ContextSpec. Revert single-mode import merge false positives. Co-authored-by: Cursor <cursoragent@cursor.com>
Fixes @typescript-eslint/prefer-nullish-coalescing in array-item-factory.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu I've made the requested changes. |
|
@Hypenate thanks for the quick turnaround 🙏 Confirmed the single-mode duplicate is gone and the regression test you added covers it. Reverting One follow-up though: the new file-level dedup breaks the other way in Repro ( paths:
/a:
get: { operationId: getA, tags: [alpha], responses: { '200': { description: ok, content: { application/json: { schema: { $ref: '#/components/schemas/TenantListResponse' } } } } } }
/b:
get: { operationId: getB, tags: [beta], responses: { '200': { description: ok, content: { application/json: { schema: { $ref: '#/components/schemas/TenantListResponse' } } } } } }
components:
schemas:
TenantResponseModelDto:
type: object
required: [id, name]
properties: { id: { type: string }, name: { type: string } }
TenantListResponse:
type: object
properties:
value: { type: array, items: { $ref: '#/components/schemas/TenantResponseModelDto' } }
count: { type: integer }Generated:
...Array.from(...).map(() => ({ ...getTenantResponseModelDtoMock() }))...Behavior per mode (all verified):
CI stays green because the For a direction: scoping the dedup per output file rather than per |
Dedup by tag bucket in tags/tags-split modes so each tag file defines its own shared factories. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu another try 😃 |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Confirmed the tags-split / tags issue is fixed on the latest head 👍 While I was at it I found a few more gaps when None of these are regressions — (1) names: { type: array, items: { $ref: '#/components/schemas/Name' } }
# components.schemas.Name: { type: string, format: email }export const getNameMock = (overrideResponse: Partial<Name> = {}): Name =>
({ ...faker.internet.email(), ...overrideResponse }); // Name = stringAt runtime it also returns (2) inline things: { type: array, items: { oneOf: [ { $ref: '#/.../Cat' }, { $ref: '#/.../Dog' } ] } }The item factory imports/types itself as (3) nullable object array item rows: { type: array, items: { type: object, nullable: true, properties: { id: { type: string } } } }
(4) same property name under two parents in one operation outer: { properties: { items: { type: array, items: { type: object, properties: { a: { type: string } } } } } }
inner: { properties: { items: { type: array, items: { type: object, properties: { b: { type: number } } } } } }Only one factory is emitted and reused for both: export const getGetCollideResponseItemsItemMock = (...): OuterItemsItem => ({ ...{ a: ... }, ... });
// both outer.items and inner.items call getGetCollideResponseItemsItemMock()So On the fix: in all four, turning |
|
@wadakatu Good catches!! Deeper into the 🐇 🕳️ I go 😆 |
|
@wadakatu Can you review it again please? |
|
Re-reviewed at Re-ran all four cases plus a few neighbours and everything compiles cleanly:
Nice work chasing all of these down. LGTM from the mock side 👍 (Minor, non-blocking: an inline object array item inside a |
3 participants
Summary
arrayItems: trueto export reusable mock factories for object-like array item schemas in operation responses$refarray items emitget<SchemaName>Mock(deduplicated across operations); inline object items emitget<OperationId>Response<PropertyName>ItemMocktyped to match Orval's generated item aliases (e.g.GetTenants200ValueItem).map()instead of inlining the full object bodyCloses #3513
Test plan
array-item-factory.test.ts)@orval/mockpackage tests passfaker-array-itemsspec + snapshot covering inline and$refarray itemsUsage
Summary by CodeRabbit
New Features
arrayItemsoption to the Faker generator to emit reusable mock factory functions for object-like array items in responses.Documentation
Tests / Examples
Improvements