chore: migrate some v2 regression cases #287
Conversation
|
Claude finished @ymc9's task —— View job PR Review: Migrating V2 Regression CasesI've completed a thorough review of this PR that migrates 7 regression test cases from v2 to v3. Overall, this is a solid migration with good test structure and necessary infrastructure improvements. Review Checklist:
✅ Positive Aspects1. Well-Structured Test Organization
2. Comprehensive Issue Coverage
3. Proper Test Infrastructure
🔧 Infrastructure Improvements1. testtools/schema.ts Additions // Good additions for schema testing
export async function loadSchema(schema: string) // lines 93-107
export async function loadSchemaWithError(schema: string, error: string | RegExp) // lines 109-132
2. turbo.json Fix "watch": {
- "dependsOn": ["^build"],
+ "dependsOn": ["^watch"],
🐛 Potential Issues & Recommendations1. Security: Temp File Management (testtools/schema.ts:99, 116) const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`);Issue: Temp files are created but never explicitly cleaned up 2. Test Quality: Missing Edge Cases
3. Performance: Test Database Strategy
4. Code Quality: Type Safety // testtools/schema.ts:102-104
expect(r).toSatisfy(
(r) => r.success,
`Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`,
);Issue: Using 📊 Test Coverage AssessmentExcellent Coverage For:
Missing Coverage:
🚀 Performance Considerations
🔒 Security Assessment
📝 Final Recommendation✅ APPROVE - This is a high-quality migration that:
Minor improvements suggested:
The migration successfully preserves the intent of the original regression tests while leveraging v3's improved testing infrastructure. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces two test utility exports for schema loading and error assertion, and adds multiple regression tests covering schema parsing/validation, relations, policies, quoted defaults, decimals, and enum defaults. Imports updated for temp file handling and assertions. No changes to core runtime logic beyond new test helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Pull Request Overview
Migrates a set of regression test cases into a new v2-migrated directory and adds helper utilities for loading schemas and expecting errors, plus a change to the Turbo pipeline configuration. Key changes focus on test portability and helper abstraction.
- Added multiple migrated regression test files using new schema loading helpers
- Introduced loadSchema and loadSchemaWithError utilities in testtools
- Modified turbo.json watch task dependency from ^build to ^watch
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| turbo.json | Alters watch task dependency, potentially affecting task execution order. |
| tests/regression/test/v2-migrated/*.test.ts | Adds migrated regression test cases using new helpers. |
| packages/testtools/src/schema.ts | Adds schema loading utilities for success and error expectations. |
Comments suppressed due to low confidence (2)
packages/testtools/src/schema.ts:1
- These two functions duplicate logic for prelude injection and temp file creation; extracting a shared helper (e.g., prepareSchemaTempFile) would reduce duplication and ease future changes (like adding cleanup). Refactor to a common internal utility returning the temp file path.
import { invariant } from '@zenstackhq/common-helpers';
packages/testtools/src/schema.ts:1
- These two functions duplicate logic for prelude injection and temp file creation; extracting a shared helper (e.g., prepareSchemaTempFile) would reduce duplication and ease future changes (like adding cleanup). Refactor to a common internal utility returning the temp file path.
import { invariant } from '@zenstackhq/common-helpers';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Pull Request Review: Migrating V2 Regression CasesI've completed a thorough review of this PR that migrates 7 regression test cases from v2 to v3. Overall, this is a solid migration with good test structure and necessary infrastructure improvements. Review Checklist:
✅ Positive Aspects1. Well-Structured Test Organization
2. Comprehensive Issue Coverage
3. Proper Test Infrastructure
🔧 Infrastructure Improvements1. testtools/schema.ts Additions // Good additions for schema testing
export async function loadSchema(schema: string) // lines 93-107
export async function loadSchemaWithError(schema: string, error: string | RegExp) // lines 109-132
2. turbo.json Fix "watch": {
- "dependsOn": ["^build"],
+ "dependsOn": ["^watch"],
🐛 Potential Issues & Recommendations1. Security: Temp File Management (testtools/schema.ts:99, 116) const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`);Issue: Temp files are created but never explicitly cleaned up 2. Test Quality: Missing Edge Cases
3. Performance: Test Database Strategy
4. Code Quality: Type Safety // testtools/schema.ts:102-104
expect(r).toSatisfy(
(r) => r.success,
`Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`,
);Issue: Using 📊 Test Coverage AssessmentExcellent Coverage For:
Missing Coverage:
🚀 Performance Considerations
🔒 Security Assessment
📝 Final Recommendation✅ APPROVE - This is a high-quality migration that:
Minor improvements suggested:
The migration successfully preserves the intent of the original regression tests while leveraging v3's improved testing infrastructure. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/testtools/src/schema.ts (1)
116-116: Duplicate: Missing crypto import.Same issue as line 99—
crypto.randomUUID()requires importingcrypto.
🧹 Nitpick comments (3)
packages/testtools/src/schema.ts (1)
93-108: Consider cleaning up temporary files.The function creates temporary files in
os.tmpdir()but never removes them. Over time, this could accumulate test artifacts.Consider adding cleanup:
export async function loadSchema(schema: string) { if (!schema.includes('datasource ')) { schema = `${makePrelude('sqlite')}\n\n${schema}`; } // create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); - fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile); + try { + fs.writeFileSync(tempFile, schema); + const r = await loadDocument(tempFile); - expect(r).toSatisfy( - (r) => r.success, - `Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`, - ); - invariant(r.success); - return r.model; + expect(r).toSatisfy( + (r) => r.success, + `Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`, + ); + invariant(r.success); + return r.model; + } finally { + try { + fs.unlinkSync(tempFile); + } catch { + // ignore cleanup errors + } + } }Apply similar cleanup to
loadSchemaWithErrorat lines 110-132.tests/regression/test/v2-migrated/issue-416.test.ts (1)
4-20: Consider adding a brief comment describing issue 416.While the test correctly verifies that the schema loads successfully with escaped quotes in default values, a one-line comment describing the original issue would improve maintainability and help future readers understand what bug this test prevents.
Example:
+// Issue 416: Verify that escaped quotes in default values are parsed correctly it('verifies issue 416', async () => { await loadSchema(tests/regression/test/v2-migrated/issue-632.test.ts (1)
4-25: Consider adding a brief comment describing issue 632.While the test correctly verifies that a client can be created with multiple enum fields sharing the same default value, a one-line comment describing the original issue would improve maintainability.
Example:
+// Issue 632: Verify that multiple fields can use the same enum default value it('verifies issue 632', async () => { await createTestClient(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/testtools/src/schema.ts(2 hunks)tests/regression/test/v2-migrated/issue-177.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-389.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-392.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-416.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-509.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-609.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-632.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-646.test.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/v2-migrated/issue-416.test.tspackages/testtools/src/schema.tstests/regression/test/v2-migrated/issue-389.test.tstests/regression/test/v2-migrated/issue-509.test.tstests/regression/test/v2-migrated/issue-392.test.tstests/regression/test/v2-migrated/issue-177.test.tstests/regression/test/v2-migrated/issue-646.test.tstests/regression/test/v2-migrated/issue-609.test.tstests/regression/test/v2-migrated/issue-632.test.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/schema.ts
🧬 Code graph analysis (9)
tests/regression/test/v2-migrated/issue-416.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(93-108)
packages/testtools/src/schema.ts (2)
packages/language/test/utils.ts (2)
loadSchema(8-19)loadSchemaWithError(21-39)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
tests/regression/test/v2-migrated/issue-389.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-509.test.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-136)
tests/regression/test/v2-migrated/issue-392.test.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-136)
tests/regression/test/v2-migrated/issue-177.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchemaWithError(110-132)
tests/regression/test/v2-migrated/issue-646.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(93-108)
tests/regression/test/v2-migrated/issue-609.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(177-188)
tests/regression/test/v2-migrated/issue-632.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-167)
⏰ 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). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (9)
turbo.json (1)
10-10: LGTM! Logical dependency change for watch tasks.The change from
["^build"]to["^watch"]makes sense for development workflows, allowing watch tasks to depend on upstream watch tasks for continuous rebuilding.tests/regression/test/v2-migrated/issue-509.test.ts (1)
4-29: Verify test intent: no assertion on schema load result.The test calls
loadDocumentbut doesn't assert or check the result.loadDocumentreturns a result object with{ success: true/false, ... }rather than throwing on failure, so the test will pass even if schema loading fails.Consider using the
loadSchemahelper from@zenstackhq/testtools(introduced in this PR) which automatically asserts success:-import { loadDocument } from '@zenstackhq/language'; +import { loadSchema } from '@zenstackhq/testtools'; import { it } from 'vitest'; it('verifies issue 509', async () => { - await loadDocument( + await loadSchema( ` model User { id Int @id @default(autoincrement()) email String @unique name String? posts Post[] } model Post { id Int @id @default(autoincrement()) title String content String? published Boolean @default(false) author User? @relation(fields: [authorId], references: [id]) authorId Int? deleted Boolean @default(false) @omit @@allow('all', true) @@deny('read', deleted) } `, ); });Alternatively, if the intent is to verify the schema loads successfully without assertion, please add a comment explaining this.
tests/regression/test/v2-migrated/issue-609.test.ts (1)
4-59: LGTM! Well-structured policy enforcement test.The test clearly verifies cross-user comment relationships under policy constraints. The setup, seeding, and assertions are well-organized.
tests/regression/test/v2-migrated/issue-392.test.ts (1)
4-63: Verify test intent: no assertions on schema load results.The test calls
loadDocumentfour times but never checks the results. SinceloadDocumentreturns a result object rather than throwing on failure, the test will pass even if schema loading fails.Consider using the
loadSchemahelper from@zenstackhq/testtoolswhich automatically asserts success:-import { loadDocument } from '@zenstackhq/language'; +import { loadSchema } from '@zenstackhq/testtools'; import { it } from 'vitest'; it('verifies issue 392', async () => { - await loadDocument( + await loadSchema( ` model M1 { m2_id String @id m2 M2 @relation(fields: [m2_id], references: [id]) } model M2 { id String @id m1 M1? } `, ); - await loadDocument( + await loadSchema( ` model M1 { id String @id m2_id String @unique m2 M2 @relation(fields: [m2_id], references: [id]) } model M2 { id String @id m1 M1? } `, ); - await loadDocument( + await loadSchema( ` model M1 { m2_id String m2 M2 @relation(fields: [m2_id], references: [id]) @@id([m2_id]) } model M2 { id String @id m1 M1? } `, ); - await loadDocument( + await loadSchema( ` model M1 { m2_id String m2 M2 @relation(fields: [m2_id], references: [id]) @@unique([m2_id]) } model M2 { id String @id m1 M1? } `, ); });tests/regression/test/v2-migrated/issue-389.test.ts (1)
4-15: LGTM! Clear policy constraint test.The test effectively validates field-level policy enforcement with the
value > 0constraint, asserting both rejection and success cases.tests/regression/test/v2-migrated/issue-646.test.ts (1)
4-11: LGTM! Test verifies Decimal default value support.The test uses
loadSchemawhich internally asserts successful schema loading, so it effectively verifies that the Decimal type with a small default value (0.00000001) parses correctly.tests/regression/test/v2-migrated/issue-177.test.ts (1)
4-25: LGTM! Effective relation constraint validation test.The test properly uses
loadSchemaWithErrorto verify that the schema parser correctly detects and reports the mismatch between non-optional relationbarand optional fieldbarId1.tests/regression/test/v2-migrated/issue-416.test.ts (1)
1-2: LGTM!The imports are correct and align with the test structure.
tests/regression/test/v2-migrated/issue-632.test.ts (1)
1-2: LGTM!The imports are correct and align with the test structure.
|
Claude finished @ymc9's task —— View job PR Review: Migrating V2 Regression CasesI've completed a thorough review of this PR that migrates 7 regression test cases from v2 to v3. Overall, this is a solid migration with good test structure and necessary infrastructure improvements. Review Checklist:
✅ Positive Aspects1. Well-Structured Test Organization
2. Comprehensive Issue Coverage
3. Proper Test Infrastructure
🔧 Infrastructure Improvements1. testtools/schema.ts Additions // Good additions for schema testing
export async function loadSchema(schema: string) // lines 94-109
export async function loadSchemaWithError(schema: string, error: string | RegExp) // lines 111-133
2. turbo.json Fix "watch": {
- "dependsOn": ["^build"],
+ "dependsOn": ["^watch"],
🐛 Potential Issues & Recommendations1. Security: Temp File Management (testtools/schema.ts:100, 117) const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`);Issue: Temp files are created but never explicitly cleaned up 2. Test Quality: Missing Edge Cases
3. Performance: Test Database Strategy
4. Code Quality: Type Safety // testtools/schema.ts:105
expect(r).toSatisfy(
(r) => r.success,
`Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`,
);Issue: Using 📊 Test Coverage AssessmentExcellent Coverage For:
Missing Coverage:
🚀 Performance Considerations
🔒 Security Assessment
📝 Final Recommendation✅ APPROVE - This is a high-quality migration that:
Minor improvements suggested:
The migration successfully preserves the intent of the original regression tests while leveraging v3's improved testing infrastructure. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/testtools/src/schema.ts (3)
129-129: Make string coercion explicit for clarity.Using
error.test(e)relies on implicit object-to-string coercion. Making it explicit improves clarity and avoids surprises if error object shapes change.Apply this diff:
- (r) => r.errors.some((e: any) => error.test(e)), + (r) => r.errors.some((e: any) => error.test(e.toString())),
100-108: Temp files accumulate without cleanup.Temporary files created via
crypto.randomUUID()are never deleted, which can accumulate during large test runs.Wrap the load/assert in try/finally and call
fs.unlinkSync(tempFile)in the finally block:// create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile); - expect(r).toSatisfy( - (r) => r.success, - `Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`, - ); - invariant(r.success); - return r.model; + try { + const r = await loadDocument(tempFile); + expect(r).toSatisfy( + (r) => r.success, + `Failed to load schema: ${(r as any).errors?.map((e: any) => e.toString()).join(', ')}`, + ); + invariant(r.success); + return r.model; + } finally { + try { + fs.unlinkSync(tempFile); + } catch { + // ignore cleanup errors + } + }
117-121: Temp files accumulate without cleanup.Temporary files created via
crypto.randomUUID()are never deleted, which can accumulate during large test runs.Wrap the load/assert in try/finally and call
fs.unlinkSync(tempFile)in the finally block:// create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile); - expect(r.success).toBe(false); - invariant(!r.success); - if (typeof error === 'string') { + try { + const r = await loadDocument(tempFile); + expect(r.success).toBe(false); + invariant(!r.success); + if (typeof error === 'string') { + expect(r).toSatisfy( + (r) => r.errors.some((e: any) => e.toString().toLowerCase().includes(error.toLowerCase())), + `Expected error message to include "${error}" but got: ${r.errors.map((e: any) => e.toString()).join(', ')}`, + ); + } else { + expect(r).toSatisfy( + (r) => r.errors.some((e: any) => error.test(e.toString())), + `Expected error message to match "${error}" but got: ${r.errors.map((e: any) => e.toString()).join(', ')}`, + ); + } + } finally { + try { + fs.unlinkSync(tempFile); + } catch { + // ignore cleanup errors + } + } +``` Note: The diff also includes the fix for line 129 (explicit `.toString()` call). </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>packages/testtools/src/schema.ts (2)</summary><blockquote> `94-109`: **Code duplication: consolidate with language/test/utils.ts.** This function is nearly identical to `loadSchema` in `packages/language/test/utils.ts` (lines 7-18). Consider consolidating by either: 1. Having `language/test/utils.ts` import and re-export this function, or 2. Moving the shared implementation to a common location This reduces maintenance burden and ensures consistent behavior across test utilities. --- `111-133`: **Code duplication: consolidate with language/test/utils.ts.** This function is nearly identical to `loadSchemaWithError` in `packages/language/test/utils.ts` (lines 20-38). Consider consolidating by either: 1. Having `language/test/utils.ts` import and re-export this function, or 2. Moving the shared implementation to a common location This reduces maintenance burden and ensures consistent behavior across test utilities. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f913d7a1d610a804f4592b6ef4e5bc0a02e9c113 and cd034a9bfd44c5650ff8b2f921ae61f03035a16a. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `packages/testtools/src/schema.ts` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (2)</summary> <details> <summary>{packages,samples,tests}/**</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Place packages only under `packages/`, `samples/`, or `tests/` Files: - `packages/testtools/src/schema.ts` </details> <details> <summary>**/schema.ts</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > The generated TypeScript schema should be named `schema.ts` Files: - `packages/testtools/src/schema.ts` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>packages/testtools/src/schema.ts (3)</summary><blockquote> <details> <summary>packages/language/test/utils.ts (2)</summary> * `loadSchema` (8-19) * `loadSchemaWithError` (21-39) </details> <details> <summary>packages/language/src/index.ts (1)</summary> * `loadDocument` (21-136) </details> <details> <summary>packages/common-helpers/src/tiny-invariant.ts (1)</summary> * `invariant` (4-14) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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). (2)</summary> * GitHub Check: build-test (20.x, sqlite) * GitHub Check: build-test (20.x, postgresql) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>packages/testtools/src/schema.ts (1)</summary><blockquote> `6-6`: **LGTM! Crypto import added.** The missing `crypto` import has been addressed, resolving the previous runtime error concern. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit