chore: migrate more v2 tests #254
Conversation
WalkthroughMultiple migrated policy tests updated import paths. Several new Vitest suites were added for composite IDs, nested to-one/to-many relations, query reduction, and a petstore sample. A set of auto-generated petstore schema/types files were introduced. One existing test also updated a suite title and an additional import. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Claude finished @ymc9's task —— View job PR Review: Test MigrationReviewing this test migration PR with focus on: Review Checklist:
Review SummaryThis PR successfully migrates v2 tests to the new test structure. The changes are well-organized and follow good migration practices. ✅ Positive Findings1. Clean Migration Structure
2. Petstore Schema Implementation
3. Code Quality
🔍 Areas for Attention1. Skipped Test in Petstore
2. Generated File Management
🛡️ Security AssessmentNo security concerns identified:
⚡ Performance ConsiderationsNo performance issues:
🐛 No Bugs IdentifiedThe migration appears technically sound with no obvious bugs or breaking changes. 📋 Recommendations
🎯 ConclusionThis is a solid test migration that maintains test integrity while properly organizing code structure. The changes are safe to merge with the minor recommendations noted above. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates v2 tests to the new test structure as part of a test migration initiative. The changes involve moving test files from a v2-specific directory to a new migrated tests directory structure and updating import paths accordingly.
- Adds petstore schema files including the ZModel definition and generated TypeScript types
- Moves multiple test files from v2 test location to the migrated directory under
packages/runtime/test/policy/migrated/ - Updates import paths in all migrated test files to reference the correct utilities location
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/runtime/test/schemas/petstore/schema.zmodel |
New petstore schema definition with User, Pet, and Order models |
packages/runtime/test/schemas/petstore/schema.ts |
Generated schema definition file for the petstore example |
packages/runtime/test/schemas/petstore/models.ts |
Generated TypeScript model types for petstore schema |
packages/runtime/test/schemas/petstore/input.ts |
Generated TypeScript input types for database operations |
| Multiple test files | Migrated test files with updated import paths to reference ../utils instead of ./utils |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/runtime/test/policy/migrated/multi-id-fields.test.ts (1)
370-373: Type mismatch in skipped test: Int field assigned a stringEven though skipped,
yisIntbut receives'4'. Keep types consistent to avoid future flakiness when the test is enabled.- create: { x: '4', y: '4', value: 4 }, + create: { x: '4', y: 4, value: 4 },packages/runtime/test/schemas/petstore/schema.zmodel (1)
1-4: Prefer in-memory SQLite for test isolationA file-backed DB (
file:./petstore.db) can leak state across runs/CI shards. Consider an in-memory DSN for hermetic tests.datasource db { provider = 'sqlite' - url = 'file:./petstore.db' + // shared in-memory sqlite (works with Prisma/SQLite) + url = 'file:petstore?mode=memory&cache=shared' }packages/runtime/test/policy/migrated/petstore-sample.test.ts (1)
5-7: Enable this suite; it doesn’t depend on future().The test only does create/read; Pet’s future()-gated rule is for update. Safe to unskip to get coverage now.
-// TODO: `future()` support -describe.skip('Pet Store Policy Tests', () => { +// TODO: add update tests once `future()` is supported +describe('Pet Store Policy Tests', () => {packages/runtime/test/policy/migrated/nested-to-one.test.ts (2)
137-139: Fix typo: “hosting” → “hoisting”.Minor comment clarity in the skipped test.
- // including m3 causes m1 to be filtered due to hosting + // including m3 causes m1 to be filtered due to hoisting
82-88: Make the assertion explicit for stability.Instead of truthiness, assert the expected cardinality.
-await expect(db.m2.findMany({ include: { m1: true } })).toResolveTruthy(); +await expect(db.m2.findMany({ include: { m1: true } })).resolves.toHaveLength(1);packages/runtime/test/policy/migrated/nested-to-many.test.ts (1)
125-130: Fix comment: mentions the wrong relation.You’re including m4 here, not m3.
- // including m3 causes m2 to be filtered since m4 is not nullable + // including m4 causes m2 to be filtered since m4 is not nullablepackages/runtime/test/schemas/petstore/input.ts (1)
30-30: Confirm GetPayload constraint matches current Select/Include semanticsThe constraint Args extends $SelectIncludeOmit<$Schema, "", true> assumes the 3rd generic flag is supported and desired. Please validate against the runtime’s latest definition to avoid overly restrictive payload typing.
You can rely on the script above (last grep) to inspect SelectIncludeOmit’s signature and confirm whether the boolean flag is expected in v3.
Also applies to: 50-50, 70-70
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/runtime/test/policy/migrated/auth.test.ts(1 hunks)packages/runtime/test/policy/migrated/client-extensions.test.ts(1 hunks)packages/runtime/test/policy/migrated/connect-disconnect.test.ts(1 hunks)packages/runtime/test/policy/migrated/create-many-and-return.test.ts(1 hunks)packages/runtime/test/policy/migrated/cross-model-field-comparison.test.ts(1 hunks)packages/runtime/test/policy/migrated/current-model.test.ts(1 hunks)packages/runtime/test/policy/migrated/current-operation.test.ts(1 hunks)packages/runtime/test/policy/migrated/deep-nested.test.ts(1 hunks)packages/runtime/test/policy/migrated/empty-policy.test.ts(1 hunks)packages/runtime/test/policy/migrated/field-comparison.test.ts(1 hunks)packages/runtime/test/policy/migrated/multi-field-unique.test.ts(1 hunks)packages/runtime/test/policy/migrated/multi-id-fields.test.ts(1 hunks)packages/runtime/test/policy/migrated/nested-to-many.test.ts(1 hunks)packages/runtime/test/policy/migrated/nested-to-one.test.ts(1 hunks)packages/runtime/test/policy/migrated/petstore-sample.test.ts(1 hunks)packages/runtime/test/policy/migrated/query-reduction.test.ts(1 hunks)packages/runtime/test/schemas/petstore/input.ts(1 hunks)packages/runtime/test/schemas/petstore/models.ts(1 hunks)packages/runtime/test/schemas/petstore/schema.ts(1 hunks)packages/runtime/test/schemas/petstore/schema.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/test/policy/migrated/field-comparison.test.tspackages/runtime/test/policy/migrated/current-operation.test.tspackages/runtime/test/policy/migrated/empty-policy.test.tspackages/runtime/test/policy/migrated/client-extensions.test.tspackages/runtime/test/policy/migrated/petstore-sample.test.tspackages/runtime/test/policy/migrated/auth.test.tspackages/runtime/test/policy/migrated/deep-nested.test.tspackages/runtime/test/policy/migrated/connect-disconnect.test.tspackages/runtime/test/schemas/petstore/models.tspackages/runtime/test/policy/migrated/current-model.test.tspackages/runtime/test/policy/migrated/cross-model-field-comparison.test.tspackages/runtime/test/policy/migrated/create-many-and-return.test.tspackages/runtime/test/policy/migrated/query-reduction.test.tspackages/runtime/test/schemas/petstore/schema.tspackages/runtime/test/policy/migrated/multi-field-unique.test.tspackages/runtime/test/policy/migrated/nested-to-one.test.tspackages/runtime/test/policy/migrated/nested-to-many.test.tspackages/runtime/test/schemas/petstore/input.tspackages/runtime/test/schemas/petstore/schema.zmodelpackages/runtime/test/policy/migrated/multi-id-fields.test.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/runtime/test/schemas/petstore/schema.ts
**/schema.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
Name ZModel schema files
schema.zmodel
Files:
packages/runtime/test/schemas/petstore/schema.zmodel
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
packages/runtime/test/schemas/petstore/models.tspackages/runtime/test/schemas/petstore/schema.ts
🧬 Code graph analysis (6)
packages/runtime/test/policy/migrated/petstore-sample.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/test/schemas/petstore/schema.ts (1)
schema(9-155)
packages/runtime/test/policy/migrated/query-reduction.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/schemas/petstore/schema.ts (2)
packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
packages/runtime/test/policy/migrated/nested-to-one.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/src/client/crud/operations/base.ts (1)
read(146-187)
packages/runtime/test/policy/migrated/nested-to-many.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/src/client/crud/operations/base.ts (1)
read(146-187)
packages/runtime/test/policy/migrated/multi-id-fields.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (20)
packages/runtime/test/policy/migrated/empty-policy.test.ts (1)
2-2: LGTM: shared utils importConsistent with the reorg to policy/utils.ts.
packages/runtime/test/policy/migrated/auth.test.ts (1)
2-2: LGTM: path fixImporting from ../utils aligns with other migrated tests.
packages/runtime/test/policy/migrated/create-many-and-return.test.ts (1)
2-2: LGTM: centralizing test utilCorrect relative path from migrated/ to policy/utils.ts.
packages/runtime/test/policy/migrated/connect-disconnect.test.ts (1)
2-2: LGTM: utils importMatches the shared util location; no further changes needed.
packages/runtime/test/policy/migrated/cross-model-field-comparison.test.ts (1)
2-2: LGTM: import pathConsistent with the migration to ../utils.
packages/runtime/test/policy/migrated/deep-nested.test.ts (1)
2-2: LGTM: shared utilsRelative path is correct at this depth.
packages/runtime/test/policy/migrated/field-comparison.test.ts (1)
2-2: Import path update verifiedpackages/runtime/test/policy/utils.ts exports createPolicyTestClient; all files in packages/runtime/test/policy/migrated/ import from "../utils" and no "./utils" imports remain.
packages/runtime/test/policy/migrated/client-extensions.test.ts (1)
2-3: Imports verified — definePlugin exported & ../utils resolvesdefinePlugin is exported from packages/runtime/src/client/plugin.ts (export function definePlugin at line ~45) and createPolicyTestClient is exported from packages/runtime/test/policy/utils.ts; the imports in packages/runtime/test/policy/migrated/client-extensions.test.ts are correct.
packages/runtime/test/policy/migrated/multi-id-fields.test.ts (1)
1-58: Strong coverage for composite IDs and nested policy casesGood end-to-end assertions (CRUD, includes, auth switching, nested writes). The
$unuseAll()check validating read filtering is a nice touch.packages/runtime/test/schemas/petstore/schema.zmodel (1)
14-52: Schema structure and policies look consistent with tests
- File name and location comply with guidelines.
- Access rules align with described behaviors (unsold pet visibility; order ownership; restricted updates via
future()).packages/runtime/test/policy/migrated/current-model.test.ts (1)
2-2: Centralized utils import confirmedConsistent with other migrated tests.
packages/runtime/test/policy/migrated/multi-field-unique.test.ts (1)
3-4: Import paths and suite rename are correct; QueryError is exported.packages/runtime/src/client/errors.ts exports QueryError (export class QueryError extends Error at line 13).
packages/runtime/test/policy/migrated/current-operation.test.ts (1)
2-2: Centralized utils import confirmed for migrated policy testsAll files in packages/runtime/test/policy/migrated/ import from ../utils; packages/runtime/test/policy/utils.ts also imports ../utils, so migrated tests use the centralized helper. Non-migrated tests still import ./utils (policy-functions.test.ts, todo-sample.test.ts, mixin.test.ts, auth-equality.test.ts).
packages/runtime/test/schemas/petstore/models.ts (1)
8-12: LGTM: generated model result aliases match schema exports.Types line up with SchemaType from ./schema and ModelResult from @zenstackhq/runtime.
packages/runtime/test/policy/migrated/query-reduction.test.ts (1)
4-136: LGTM: good coverage for AND/OR/NOT and relation filters under policy.Assertions align with the seeded data and policy semantics.
packages/runtime/test/schemas/petstore/schema.ts (2)
155-156: LGTM: conforms to naming guideline and exports SchemaType.Generated schema is named schema.ts and provides SchemaType as expected.
8-10: Ensure build runs before tests — dist import missingFile packages/runtime/test/schemas/petstore/schema.ts (lines 8–10): imports "../../../dist/schema" but packages/runtime/dist/schema is absent (fd returned "Search path 'packages/runtime/dist/schema' is not a directory"). Run the build before running tests or change the test to import from source.
packages/runtime/test/policy/migrated/nested-to-many.test.ts (1)
52-61: LGTM: skip retained for v2 semantics check.Appropriate to keep this isolated until semantics are decided.
packages/runtime/test/schemas/petstore/input.ts (2)
1-7: Generated file header looks goodClear "do not modify" banner and global eslint-disable are appropriate for generated test artifacts.
8-10: Runtime type exports verified — no action requiredpackages/runtime/test/schemas/petstore/schema.ts exports SchemaType; all referenced @zenstackhq/runtime types (including CreateManyAndReturnArgs, UpdateManyAndReturnArgs, SelectIncludeOmit) are exported from packages/runtime (e.g., packages/runtime/src/client/crud-types.ts).
Summary by CodeRabbit
Tests
Refactor
Chores