Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Dec 1, 2025

Summary by CodeRabbit

  • New Features

    • Added an edge-runtime test suite with CRUD and access-control validation, plus generated schemas/types and supporting tooling.
  • Tests

    • Simplified test fixtures and schemas by removing unused fields and enums.
    • Improved test isolation and teardown across Bun tests.
    • Expanded test discovery to include nested test directories and added a tailored Vitest config for edge-runtime.
  • Chores

    • Added generation script, package configuration, and TypeScript settings for edge-runtime tests.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings December 1, 2025 23:14
@coderabbitai
Copy link

coderabbitai bot commented Dec 1, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Adds a new edge-runtime test package (schema, generated types, tests, tooling), simplifies Bun runtime schemas/tests by removing shared CommonFields/Role/timestamps/meta/content, updates Bun test teardown to disconnect clients, and broadens Vitest test discovery globs.

Changes

Cohort / File(s) Summary
Bun e2e test
tests/runtimes/bun/bun-e2e.test.ts
Added afterEach teardown to disconnect generated _db client, refactored client assignment to outer _db variable, and removed content from seeded Post fixtures.
Bun schema simplification
tests/runtimes/bun/schemas/schema.zmodel, tests/runtimes/bun/schemas/schema.ts, tests/runtimes/bun/schemas/models.ts
Removed typeDefs.CommonFields and enums.Role; dropped fields createdAt, updatedAt, role, meta, content; eliminated exported CommonFields/Role symbols; rewrote User/Post as standalone models.
Edge-runtime package & config
tests/runtimes/edge-runtime/package.json, tests/runtimes/edge-runtime/tsconfig.json, tests/runtimes/edge-runtime/vitest.config.ts
Added new package.json, tsconfig (extends base, noEmit), and Vitest config merging base with environment set to edge-runtime.
Edge-runtime schema & generated types
tests/runtimes/edge-runtime/schemas/schema.zmodel, tests/runtimes/edge-runtime/schemas/schema.ts, tests/runtimes/edge-runtime/schemas/models.ts, tests/runtimes/edge-runtime/schemas/input.ts
Added Postgres datasource and policy plugin; added User and Post models with relations and access policies; exported typed _schema/schema and Schema branding; added generated model types and CRUD input/type aliases.
Edge-runtime tests & tooling
tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts, tests/runtimes/edge-runtime/scripts/generate.ts
Added e2e tests (CRUD and policy enforcement) using per-test DB creation, pool-backed Postgres dialect, PolicyPlugin, and $setAuth checks; added script to discover .zmodel files and run codegen (invokes npx zen generate per schema).
Root vitest config
vitest.config.ts
Broadened test discovery globs from non-recursive (packages/*, tests/*) to recursive (packages/**, tests/**).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Extra attention:
    • Access-control policy semantics and relation foreign-key metadata in tests/runtimes/edge-runtime/schemas/*.
    • DB lifecycle, pool configuration, and teardown logic in edge-runtime-e2e.test.ts and Bun test changes (afterEach/_db).
    • Consistency between generated input.ts/models.ts and the concrete schema.ts exports.
    • Ensure removal of CommonFields/Role/timestamp fields doesn't break other generators or tests.

Possibly related PRs

Poem

🐰 I hopped through diffs and munched a line,
Trimmed fields and enums — the schema's fine.
New edge tests run, databases spin,
I twitched my nose and watched the builds begin. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'test: add edge-runtime e2e' directly and clearly describes the main change: adding end-to-end tests for the edge-runtime. It is specific, concise, and accurately reflects the primary objective of the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test/edge-runtime

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.

@claude
Copy link

claude bot commented Dec 1, 2025

Claude finished @ymc9's task —— View job


I'll review this pull request that adds edge-runtime e2e tests. Let me analyze the changes systematically.

Review Tasks

  • Analyze new edge-runtime test structure
  • Review test implementation and coverage
  • Check configuration files
  • Examine schema and models
  • Review refactored bun tests
  • Assess code quality and best practices
  • Check for potential issues

Code Review Summary

The PR successfully adds edge-runtime e2e testing capabilities alongside the existing bun tests. The implementation follows established patterns and maintains consistency across runtime environments.

✅ Strengths

Test Structure & Organization:

  • Well-structured test suite mirroring the bun tests pattern in tests/runtimes/edge-runtime/
  • Clear separation of concerns with dedicated schema, models, and config files
  • Proper vitest configuration for edge-runtime environment (vitest.config.ts:8)

Test Coverage:

  • Covers essential CRUD operations (create, read, update, delete, count)
  • Tests policy enforcement with authentication scenarios
  • Validates both anonymous and authenticated access patterns
  • Tests nested entity creation (users with posts)

Code Quality:

  • Clean, readable test cases in edge-runtime-e2e.test.ts
  • Proper async/await usage throughout
  • Good separation of test data setup and assertions
  • Consistent naming conventions

⚠️ Areas for Improvement

Configuration Issues:

  1. TypeScript Config Mismatch (tsconfig.json:6):

    • The edge-runtime tests include "types": ["bun-types"] which seems incorrect for edge-runtime environment
    • Should probably use edge-runtime specific types or remove this altogether
  2. Generate Script Inconsistency (scripts/generate.ts:17):

    • Uses bunx --bun zen generate which doesn't align with edge-runtime testing
    • Should use a more generic approach like npx zenstack generate

Test Implementation:

  1. Database Cleanup (edge-runtime-e2e.test.ts:83-85):

    • Tests drop/create databases but don't clean up after themselves
    • Could lead to test pollution or resource leaks in CI environments
    • Consider using unique database names with timestamps or cleanup hooks
  2. Error Handling (edge-runtime-e2e.test.ts:78-95):

    • The createClient function lacks error handling for database operations
    • PostgreSQL connection errors could cause tests to hang or fail unexpectedly

Schema Differences:

  1. Provider Inconsistency:
    • Bun tests use SQLite (bun/schemas/schema.zmodel:2) while edge-runtime uses PostgreSQL (edge-runtime/schemas/schema.zmodel:2)
    • This is intentional for testing different databases, but worth documenting

🔍 Potential Issues

Performance:

  • Each test creates a new database and schema, which adds overhead
  • Consider reusing databases with proper cleanup between test cases

Security:

  • Tests use hardcoded user IDs which is fine for testing but ensure production code doesn't follow this pattern
  • PostgreSQL connection uses TEST_PG_URL which appears properly configured

Reliability:

  • Edge-runtime environment dependencies (@edge-runtime/vm) should be stable
  • Tests depend on external PostgreSQL instance availability

📝 Minor Suggestions

  1. Test Descriptions: Could be more descriptive (e.g., "should perform basic CRUD operations" vs "works with simple CRUD")

  2. Magic Numbers: Test uses hardcoded counts (expect 0, 1, 2) - consider using constants for better maintainability

  3. Documentation: The PR lacks a description explaining the purpose and scope of edge-runtime testing

Overall Assessment

This is a solid addition that enhances the project's testing coverage across different runtime environments. The code quality is good and follows established patterns. The main concerns are around configuration consistency and test cleanup practices, but these are not blocking issues.

Recommendation: Approve with minor improvements suggested above.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
tests/runtimes/edge-runtime/scripts/generate.ts (2)

16-18: Unnecessary async keyword.

The generate function is marked async but only contains synchronous execSync code. Either remove async or consider using exec with promises for truly async execution.

-async function generate(schemaPath: string) {
+function generate(schemaPath: string) {
     execSync('bunx --bun zen generate', { cwd: path.dirname(schemaPath) });
 }

8-14: Consider adding error handling for generation failures.

If execSync fails for one schema, the script will crash without processing remaining schemas. Consider wrapping in try-catch if you want to report all failures at once.

 async function main() {
     const zmodelFiles = [...glob.sync(path.resolve(dir, '../schemas/*.zmodel'))];
+    const errors: string[] = [];
     for (const file of zmodelFiles) {
         console.log(`Generating TS schema for: ${file}`);
-        await generate(file);
+        try {
+            generate(file);
+        } catch (e) {
+            errors.push(`Failed to generate ${file}: ${e}`);
+        }
     }
+    if (errors.length > 0) {
+        throw new Error(errors.join('\n'));
+    }
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c22e34 and 5c9086d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (14)
  • tests/runtimes/bun/bun-e2e.test.ts (0 hunks)
  • tests/runtimes/bun/schemas/models.ts (1 hunks)
  • tests/runtimes/bun/schemas/schema.ts (0 hunks)
  • tests/runtimes/bun/schemas/schema.zmodel (1 hunks)
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts (1 hunks)
  • tests/runtimes/edge-runtime/package.json (1 hunks)
  • tests/runtimes/edge-runtime/schemas/input.ts (1 hunks)
  • tests/runtimes/edge-runtime/schemas/models.ts (1 hunks)
  • tests/runtimes/edge-runtime/schemas/schema.ts (1 hunks)
  • tests/runtimes/edge-runtime/schemas/schema.zmodel (1 hunks)
  • tests/runtimes/edge-runtime/scripts/generate.ts (1 hunks)
  • tests/runtimes/edge-runtime/tsconfig.json (1 hunks)
  • tests/runtimes/edge-runtime/vitest.config.ts (1 hunks)
  • vitest.config.ts (1 hunks)
💤 Files with no reviewable changes (2)
  • tests/runtimes/bun/bun-e2e.test.ts
  • tests/runtimes/bun/schemas/schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zmodel

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.zmodel: Always run zenstack generate after modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript via zenstack generate

Files:

  • tests/runtimes/edge-runtime/schemas/schema.zmodel
  • tests/runtimes/bun/schemas/schema.zmodel
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests

Applied to files:

  • tests/runtimes/edge-runtime/tsconfig.json
  • vitest.config.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

  • tests/runtimes/edge-runtime/tsconfig.json
  • tests/runtimes/edge-runtime/package.json
  • vitest.config.ts
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts
  • tests/runtimes/edge-runtime/scripts/generate.ts
  • tests/runtimes/bun/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/schema.ts
  • tests/runtimes/edge-runtime/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/input.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`

Applied to files:

  • tests/runtimes/edge-runtime/tsconfig.json
  • tests/runtimes/edge-runtime/scripts/generate.ts
  • tests/runtimes/edge-runtime/schemas/schema.zmodel
  • tests/runtimes/bun/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/schema.ts
  • tests/runtimes/edge-runtime/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/input.ts
  • tests/runtimes/bun/schemas/schema.zmodel
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts
  • tests/runtimes/edge-runtime/schemas/input.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization

Applied to files:

  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts
  • tests/runtimes/bun/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/schema.ts
  • tests/runtimes/edge-runtime/schemas/models.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : Always run `zenstack generate` after modifying ZModel schemas

Applied to files:

  • tests/runtimes/edge-runtime/scripts/generate.ts
  • tests/runtimes/edge-runtime/schemas/schema.zmodel
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible

Applied to files:

  • tests/runtimes/bun/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/schema.ts
  • tests/runtimes/edge-runtime/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/input.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.

Applied to files:

  • tests/runtimes/bun/schemas/models.ts
  • tests/runtimes/edge-runtime/schemas/schema.ts
🧬 Code graph analysis (2)
tests/runtimes/edge-runtime/scripts/generate.ts (1)
packages/cli/src/utils/exec-utils.ts (1)
  • execSync (7-16)
tests/runtimes/edge-runtime/schemas/schema.ts (3)
packages/schema/src/expression-utils.ts (1)
  • ExpressionUtils (19-123)
packages/schema/src/schema.ts (1)
  • SchemaDef (11-19)
tests/runtimes/bun/schemas/schema.ts (1)
  • schema (103-103)
⏰ 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). (4)
  • GitHub Check: Agent
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
  • GitHub Check: claude-review
🔇 Additional comments (12)
tests/runtimes/bun/schemas/models.ts (1)

1-11: LGTM!

The auto-generated models file correctly reflects the simplified schema with only User and Post type exports. The import structure is clean and follows the expected pattern for ZenStack-generated code.

tests/runtimes/bun/schemas/schema.zmodel (1)

1-31: Schema structure looks good.

The simplified schema is well-structured for e2e testing purposes:

  • User and Post models have proper field definitions with appropriate defaults
  • Access policies correctly test authentication-based access control
  • The relation between Post and User with cascade delete/update is appropriate

As per coding guidelines, ensure zenstack generate is run after any modifications to this schema to regenerate the TypeScript types.

vitest.config.ts (1)

5-5: LGTM!

The recursive glob patterns correctly enable discovery of nested test projects like the new edge-runtime tests.

tests/runtimes/edge-runtime/vitest.config.ts (1)

1-11: LGTM!

Clean configuration that properly extends the base config and sets the edge-runtime test environment.

tests/runtimes/edge-runtime/schemas/models.ts (1)

1-11: LGTM!

Auto-generated model types correctly correspond to the User and Post models defined in schema.zmodel.

tests/runtimes/edge-runtime/package.json (1)

1-26: LGTM!

Package configuration is well-structured with appropriate dependencies for edge-runtime e2e testing. The use of workspace:* for internal packages and catalog: for pg follows monorepo best practices.

tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts (2)

10-38: Good CRUD test coverage.

The test validates create, read (findUnique/findFirst), update, and delete operations with proper assertions. This aligns with the learning that e2e tests should validate real-world schema compatibility.


40-75: Excellent policy enforcement test.

The test thoroughly validates access control by checking:

  • Anonymous users see 0 posts (denied by @@deny('all', auth() == null))
  • Post owner sees all 2 posts (allowed by @@allow('all', auth().id == authorId))
  • Other authenticated users see only 1 published post (allowed by @@allow('read', published))

This aligns well with the learning that ORM tests should include policy tests.

tests/runtimes/edge-runtime/schemas/schema.zmodel (2)

1-31: Well-structured schema with clear access policies.

The schema defines appropriate models and policies that align with the test expectations:

  • User policies allow self-management and authenticated read access
  • Post policies deny anonymous access, allow owner full access, and allow public read of published posts

This correctly implements the policy behavior validated in the e2e tests.


5-7: The plugin path resolves correctly. The relative path "../../../packages/plugins/policy" from tests/runtimes/edge-runtime/schemas/ correctly points to packages/plugins/policy, which exists and is properly structured. When zen generate runs from the schema directory during the build process, this relative path resolution works as intended. No changes needed.

tests/runtimes/edge-runtime/schemas/schema.ts (1)

8-104: Generated edge schema structure and policies look solid

The User/Post model definitions, relations, and auth rules are coherent and minimal yet realistic for e2e edge-runtime coverage, and the branded Schema/schema exports follow the established pattern for runtime schemas. Since this file is CLI‑generated and type‑checked via satisfies SchemaDef, it looks good to keep as‑is and drive any changes from the .zmodel source instead.

tests/runtimes/edge-runtime/schemas/input.ts (1)

8-50: Edge runtime input types correctly wired to the schema

The User/Post argument and payload type aliases are consistently parameterized by the local schema and cover the full set of standard operations (find/create/update/delete/count/aggregate/groupBy). This matches the expected generated pattern and should give the edge-runtime e2e tests proper typing without needing manual edits.

@claude
Copy link

claude bot commented Dec 1, 2025

Claude finished @ymc9's task —— View job


Code Review of Edge-Runtime E2E Tests

Todo List

  • Analyze new edge-runtime test structure and implementation
  • Review test coverage and quality
  • Check configuration files for correctness
  • Examine schema definitions and generated code
  • Assess security and performance considerations
  • Provide comprehensive review feedback

Overall Assessment

This PR successfully adds edge-runtime e2e testing capabilities to complement the existing bun runtime tests. The implementation follows established patterns and provides good test coverage for both basic CRUD operations and access control policies.

✅ Strengths

Test Structure & Organization

  • Well-structured test suite: Mirrors the bun tests pattern with dedicated schema, models, and configuration files (tests/runtimes/edge-runtime/)
  • Clear separation of concerns: Proper vitest configuration for edge-runtime environment at vitest.config.ts:8
  • Comprehensive test coverage: Covers essential CRUD operations (create, read, update, delete, count) and policy enforcement with authentication scenarios

Code Quality

  • Clean, readable test implementation: Clear test cases in edge-runtime-e2e.test.ts with proper async/await usage
  • Good schema design: Consistent User/Post models with appropriate access control policies (schema.zmodel:15-31)
  • Proper separation of test data setup and assertions: Tests validate both anonymous and authenticated access patterns

Configuration & Setup

  • Proper package configuration: Well-structured package.json with appropriate dependencies and scripts
  • Edge-runtime environment setup: Correct vitest configuration using edge-runtime test environment

⚠️ Issues Identified

Critical: Resource Leaks

Location: edge-runtime-e2e.test.ts:86-95

The createClient function creates PostgreSQL connection pools but never closes them:

const dialect = new PostgresDialect({
    pool: new Pool({
        connectionString: `${TEST_PG_URL}/${dbName}`,
    }),
});

Impact: Connection pool leaks can exhaust database connections in CI environments
Recommendation: Return cleanup function or use test hooks to close pools after tests

Configuration Issues

1. Command Inconsistency (scripts/generate.ts:17):

execSync('bunx --bun zen generate', { cwd: path.dirname(schemaPath) });

Uses bunx --bun in edge-runtime context - should use generic npx zenstack generate

2. Error Handling (scripts/generate.ts:8-14):

  • No error handling for generation failures
  • If one schema fails, script crashes without processing remaining schemas
  • Consider wrapping in try-catch for better error reporting

Test Implementation Concerns

3. Database Cleanup (edge-runtime-e2e.test.ts:83-85):

await pgClient.query(`DROP DATABASE IF EXISTS "${dbName}"`);
await pgClient.query(`CREATE DATABASE "${dbName}"`);
  • Tests create unique databases but don't clean up after themselves
  • Could lead to test pollution in CI environments
  • Consider using unique database names with timestamps or cleanup hooks

4. Function Marked Async Unnecessarily (scripts/generate.ts:16):
The generate function is marked async but only contains synchronous code

🔍 Minor Observations

Schema & Design

  • Provider consistency: Edge-runtime uses PostgreSQL while bun uses SQLite - this is intentional for testing different databases
  • Simplified schema: Removal of CommonFields, Role enum from bun tests is a good simplification
  • Test data consistency: Both test suites use identical test scenarios which ensures consistent behavior validation

Performance Considerations

  • Database overhead: Each test creates a new database and schema, adding overhead
  • Edge-runtime dependencies: Tests depend on @edge-runtime/vm which appears properly configured

📋 Recommendations

  1. Fix resource leaks: Implement proper pool cleanup in createClient function
  2. Improve error handling: Add try-catch blocks in generation script
  3. Standardize tooling: Replace bunx --bun zen generate with npx zenstack generate
  4. Consider test optimization: Reuse databases with proper cleanup between test cases if overhead becomes problematic

🎯 Test Coverage Assessment

Excellent coverage for:

  • ✅ Basic CRUD operations (create, read, update, delete, count)
  • ✅ Policy enforcement with different authentication contexts
  • ✅ Nested entity creation (users with posts)
  • ✅ Access control validation (anonymous vs authenticated vs owner)

The access control tests particularly shine:

  • Anonymous users correctly see 0 posts (denied by @@deny('all', auth() == null))
  • Post owners see all posts (allowed by @@allow('all', auth().id == authorId))
  • Other authenticated users see only published posts (allowed by @@allow('read', published))

🏆 Conclusion

This is a solid addition that enhances the project's testing coverage across different runtime environments. The code quality is good and follows established patterns. The main concerns are around resource management and configuration consistency, but these don't block the core functionality.

Recommendation: Approve with the resource leak fix as a priority improvement.


Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds edge-runtime e2e tests and restructures the test organization by moving runtime-specific tests into a new tests/runtimes/ directory structure. The main vitest configuration is updated to support nested test projects, enabling better organization of runtime-specific test suites.

Key Changes

  • Added new edge-runtime e2e test suite to validate ZenStack behavior in edge runtime environments using Vitest with the edge-runtime environment
  • Restructured bun tests from tests/bun/ to tests/runtimes/bun/ with simplified schemas
  • Updated root vitest config to use recursive glob patterns (**) to discover nested vitest configurations

Reviewed changes

Copilot reviewed 14 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vitest.config.ts Changed glob patterns from packages/* to packages/** and tests/* to tests/** to support nested vitest project configurations
tests/runtimes/edge-runtime/vitest.config.ts Added vitest configuration for edge-runtime environment tests
tests/runtimes/edge-runtime/tsconfig.json Added TypeScript configuration for edge-runtime tests (contains incorrect bun-types reference)
tests/runtimes/edge-runtime/scripts/generate.ts Added schema generation script (uses bunx without bun dependency)
tests/runtimes/edge-runtime/schemas/*.zmodel Added simplified schema definitions matching bun test schemas
tests/runtimes/edge-runtime/schemas/*.ts Added auto-generated TypeScript schema files
tests/runtimes/edge-runtime/package.json Added package configuration with @edge-runtime/vm dependency for vitest environment
tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts Added e2e tests for CRUD operations and policy enforcement in edge runtime
tests/runtimes/bun/* Restructured bun tests with simplified schemas (removed enums, type definitions, and content field)
pnpm-lock.yaml Updated lockfile to reflect new test structure and @edge-runtime/vm dependency
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@claude
Copy link

claude bot commented Dec 1, 2025

Claude finished @ymc9's task —— View job


Code Review of Edge-Runtime E2E Tests

Review Tasks

  • Analyze new edge-runtime test structure and implementation
  • Review test coverage and quality
  • Check configuration files for correctness
  • Examine schema definitions and generated code
  • Assess security and performance considerations
  • Provide comprehensive review feedback

Overall Assessment

This PR successfully adds edge-runtime e2e testing capabilities to complement the existing bun runtime tests. The implementation follows established patterns and provides good test coverage for both basic CRUD operations and access control policies.

✅ Strengths

Test Structure & Organization

  • Well-structured test suite: Mirrors the bun tests pattern with dedicated schema, models, and configuration files (tests/runtimes/edge-runtime/)
  • Clear separation of concerns: Proper vitest configuration for edge-runtime environment at vitest.config.ts:8
  • Comprehensive test coverage: Covers essential CRUD operations (create, read, update, delete, count) and policy enforcement with authentication scenarios

Code Quality

  • Clean, readable test implementation: Clear test cases in edge-runtime-e2e.test.ts with proper async/await usage
  • Good schema design: Consistent User/Post models with appropriate access control policies (schema.zmodel:15-31)
  • Proper separation of test data setup and assertions: Tests validate both anonymous and authenticated access patterns

Configuration & Setup

  • Proper package configuration: Well-structured package.json with appropriate dependencies and scripts
  • Edge-runtime environment setup: Correct vitest configuration using edge-runtime test environment

⚠️ Issues Identified

Critical: Resource Leaks

Location: edge-runtime-e2e.test.ts:92-96

The createClient function creates PostgreSQL connection pools but never closes them:

const dialect = new PostgresDialect({
    pool: new Pool({
        connectionString: `${TEST_PG_URL}/${dbName}`,
    }),
});

Impact: Connection pool leaks can exhaust database connections in CI environments
Recommendation: Return cleanup function or use test hooks to close pools after tests

Configuration Issues

1. Command Inconsistency (scripts/generate.ts:17):

execSync('bunx --bun zen generate', { cwd: path.dirname(schemaPath) });

Uses bunx --bun in edge-runtime context - should use generic npx zenstack generate

2. Error Handling (scripts/generate.ts:8-14):

  • No error handling for generation failures
  • If one schema fails, script crashes without processing remaining schemas
  • Consider wrapping in try-catch for better error reporting

Test Implementation Concerns

3. Database Cleanup (edge-runtime-e2e.test.ts:89-91):

await pgClient.query(`DROP DATABASE IF EXISTS "${dbName}"`);
await pgClient.query(`CREATE DATABASE "${dbName}"`);
  • Tests create unique databases but don't clean up after themselves
  • Could lead to test pollution in CI environments
  • Consider using unique database names with timestamps or cleanup hooks

4. Function Marked Async Unnecessarily (scripts/generate.ts:16):
The generate function is marked async but only contains synchronous code

🔍 Minor Observations

Schema & Design

  • Provider consistency: Edge-runtime uses PostgreSQL while bun uses SQLite - this is intentional for testing different databases
  • Simplified schema: Removal of CommonFields, Role enum from bun tests is a good simplification
  • Test data consistency: Both test suites use identical test scenarios which ensures consistent behavior validation

Performance Considerations

  • Database overhead: Each test creates a new database and schema, adding overhead
  • Edge-runtime dependencies: Tests depend on @edge-runtime/vm which appears properly configured

📋 Recommendations

  1. Fix resource leaks: Implement proper pool cleanup in createClient function
  2. Improve error handling: Add try-catch blocks in generation script
  3. Standardize tooling: Replace bunx --bun zen generate with npx zenstack generate
  4. Consider test optimization: Reuse databases with proper cleanup between test cases if overhead becomes problematic

🎯 Test Coverage Assessment

Excellent coverage for:

  • ✅ Basic CRUD operations (create, read, update, delete, count)
  • ✅ Policy enforcement with different authentication contexts
  • ✅ Nested entity creation (users with posts)
  • ✅ Access control validation (anonymous vs authenticated vs owner)

The access control tests particularly shine:

  • Anonymous users correctly see 0 posts (denied by @@deny('all', auth() == null))
  • Post owners see all posts (allowed by @@allow('all', auth().id == authorId))
  • Other authenticated users see only published posts (allowed by @@allow('read', published))

🏆 Conclusion

This is a solid addition that enhances the project's testing coverage across different runtime environments. The code quality is good and follows established patterns. The main concerns are around resource management and configuration consistency, but these don't block the core functionality.

Recommendation: Approve with the resource leak fix as a priority improvement.


@claude
Copy link

claude bot commented Dec 1, 2025

Claude finished @ymc9's task —— View job


Code Review of Edge-Runtime E2E Tests

Review Tasks

  • Analyze new edge-runtime test structure and implementation
  • Review test coverage and quality
  • Check configuration files for correctness
  • Examine schema definitions and generated code
  • Assess security and performance considerations
  • Provide comprehensive review feedback

Overall Assessment

This PR successfully adds edge-runtime e2e testing capabilities to complement the existing bun runtime tests. The implementation follows established patterns and provides good test coverage for both basic CRUD operations and access control policies.

✅ Strengths

Test Structure & Organization

  • Well-structured test suite: Mirrors the bun tests pattern with dedicated schema, models, and configuration files (tests/runtimes/edge-runtime/)
  • Clear separation of concerns: Proper vitest configuration for edge-runtime environment at vitest.config.ts:8
  • Comprehensive test coverage: Covers essential CRUD operations (create, read, update, delete, count) and policy enforcement with authentication scenarios

Code Quality

  • Clean, readable test implementation: Clear test cases in edge-runtime-e2e.test.ts with proper async/await usage
  • Good schema design: Consistent User/Post models with appropriate access control policies (schema.zmodel:15-31)
  • Proper separation of test data setup and assertions: Tests validate both anonymous and authenticated access patterns

Configuration & Setup

  • Proper package configuration: Well-structured package.json with appropriate dependencies and scripts
  • Edge-runtime environment setup: Correct vitest configuration using edge-runtime test environment

⚠️ Issues Identified

Critical: Resource Leaks

Location: edge-runtime-e2e.test.ts:86-95

The createClient function creates PostgreSQL connection pools but never closes them:

const dialect = new PostgresDialect({
    pool: new Pool({
        connectionString: `${TEST_PG_URL}/${dbName}`,
    }),
});

Impact: Connection pool leaks can exhaust database connections in CI environments
Recommendation: Return cleanup function or use test hooks to close pools after tests

Configuration Issues

1. Command Inconsistency (scripts/generate.ts:17):

execSync('npx zen generate', { cwd: path.dirname(schemaPath) });

Good: Uses generic npx zen generate (this was recently fixed from bunx --bun)

2. Error Handling (scripts/generate.ts:8-14):

  • No error handling for generation failures
  • If one schema fails, script crashes without processing remaining schemas
  • Consider wrapping in try-catch for better error reporting

Test Implementation Concerns

3. Database Cleanup (edge-runtime-e2e.test.ts:83-85):

await pgClient.query(`DROP DATABASE IF EXISTS "${dbName}"`);
await pgClient.query(`CREATE DATABASE "${dbName}"`);
  • Tests create unique databases but don't clean up after themselves
  • Could lead to test pollution in CI environments
  • Consider using unique database names with timestamps or cleanup hooks

4. Function Marked Async Unnecessarily (scripts/generate.ts:16):
The generate function is marked async but only contains synchronous code

🔍 Minor Observations

Schema & Design

  • Provider consistency: Edge-runtime uses PostgreSQL while bun uses SQLite - this is intentional for testing different databases
  • Simplified schema: Removal of CommonFields, Role enum from bun tests is a good simplification
  • Test data consistency: Both test suites use identical test scenarios which ensures consistent behavior validation

Performance Considerations

  • Database overhead: Each test creates a new database and schema, adding overhead
  • Edge-runtime dependencies: Tests depend on @edge-runtime/vm which appears properly configured

📋 Recommendations

  1. Fix resource leaks: Implement proper pool cleanup in createClient function
  2. Improve error handling: Add try-catch blocks in generation script
  3. Consider test optimization: Reuse databases with proper cleanup between test cases if overhead becomes problematic

🎯 Test Coverage Assessment

Excellent coverage for:

  • ✅ Basic CRUD operations (create, read, update, delete, count)
  • ✅ Policy enforcement with different authentication contexts
  • ✅ Nested entity creation (users with posts)
  • ✅ Access control validation (anonymous vs authenticated vs owner)

The access control tests particularly shine:

  • Anonymous users correctly see 0 posts (denied by @@deny('all', auth() == null))
  • Post owners see all posts (allowed by @@allow('all', auth().id == authorId))
  • Other authenticated users see only published posts (allowed by @@allow('read', published))

🏆 Conclusion

This is a solid addition that enhances the project's testing coverage across different runtime environments. The code quality is good and follows established patterns. The main concerns are around resource management and configuration consistency, but these don't block the core functionality.

Recommendation: Approve with the resource leak fix as a priority improvement.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ceee37 and 4646028.

📒 Files selected for processing (2)
  • tests/runtimes/bun/bun-e2e.test.ts (3 hunks)
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
🧬 Code graph analysis (1)
tests/runtimes/bun/bun-e2e.test.ts (1)
packages/orm/src/client/contract.ts (1)
  • ClientContract (63-197)
⏰ 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, sqlite)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: claude-review
🔇 Additional comments (1)
tests/runtimes/bun/bun-e2e.test.ts (1)

2-2: Importing ClientContract and afterEach aligns with the new teardown pattern

Using ClientContract for the shared db handle and pulling in afterEach from bun:test is a good fit for typing the client against schema and ensuring connections are closed between tests. No issues here.

Also applies to: 7-7

@claude
Copy link

claude bot commented Dec 2, 2025

Claude finished @ymc9's task —— View job


Pull Request Review

Review Tasks

  • Analyze new edge-runtime test structure and implementation
  • Review test coverage and quality
  • Check configuration files for correctness
  • Examine schema definitions and generated code
  • Assess security and performance considerations
  • Provide comprehensive review feedback

Overall Assessment

This PR successfully adds edge-runtime e2e testing capabilities to complement the existing bun runtime tests. The implementation follows established patterns and provides good test coverage for both basic CRUD operations and access control policies.

✅ Strengths

Test Structure & Organization

  • Well-structured test suite: Mirrors the bun tests pattern with dedicated schema, models, and configuration files (tests/runtimes/edge-runtime/)
  • Clear separation of concerns: Proper vitest configuration for edge-runtime environment at vitest.config.ts:8
  • Comprehensive test coverage: Covers essential CRUD operations (create, read, update, delete, count) and policy enforcement with authentication scenarios

Code Quality

  • Clean, readable test implementation: Clear test cases in edge-runtime-e2e.test.ts with proper async/await usage
  • Good schema design: Consistent User/Post models with appropriate access control policies (schema.zmodel:15-31)
  • Proper separation of test data setup and assertions: Tests validate both anonymous and authenticated access patterns

Configuration & Setup

  • Proper package configuration: Well-structured package.json with appropriate dependencies and scripts
  • Edge-runtime environment setup: Correct vitest configuration using edge-runtime test environment

⚠️ Issues Identified

Critical: Resource Leaks

Location: edge-runtime-e2e.test.ts:92-96

The createClient function creates PostgreSQL connection pools but never closes them:

const dialect = new PostgresDialect({
    pool: new Pool({
        connectionString: `${TEST_PG_URL}/${dbName}`,
    }),
});

Impact: Connection pool leaks can exhaust database connections in CI environments
Recommendation: Return cleanup function or use test hooks to close pools after tests

Configuration Issues

1. Error Handling (scripts/generate.ts:8-14):

  • No error handling for generation failures
  • If one schema fails, script crashes without processing remaining schemas
  • Consider wrapping in try-catch for better error reporting

Test Implementation Concerns

2. Database Cleanup (edge-runtime-e2e.test.ts:89-91):

await pgClient.query(`DROP DATABASE IF EXISTS "${dbName}"`);
await pgClient.query(`CREATE DATABASE "${dbName}"`);
  • Tests create unique databases but don't clean up after themselves
  • Could lead to test pollution in CI environments
  • Consider using unique database names with timestamps or cleanup hooks

3. Function Marked Async Unnecessarily (scripts/generate.ts:16):
The generate function is marked async but only contains synchronous code

🔍 Minor Observations

Schema & Design

  • Provider consistency: Edge-runtime uses PostgreSQL while bun uses SQLite - this is intentional for testing different databases
  • Simplified schema: Removal of CommonFields, Role enum from bun tests is a good simplification
  • Test data consistency: Both test suites use identical test scenarios which ensures consistent behavior validation

Performance Considerations

  • Database overhead: Each test creates a new database and schema, adding overhead
  • Edge-runtime dependencies: Tests depend on @edge-runtime/vm which appears properly configured

📋 Recommendations

  1. Fix resource leaks: Implement proper pool cleanup in createClient function
  2. Improve error handling: Add try-catch blocks in generation script
  3. Consider test optimization: Reuse databases with proper cleanup between test cases if overhead becomes problematic

🎯 Test Coverage Assessment

Excellent coverage for:

  • ✅ Basic CRUD operations (create, read, update, delete, count)
  • ✅ Policy enforcement with different authentication contexts
  • ✅ Nested entity creation (users with posts)
  • ✅ Access control validation (anonymous vs authenticated vs owner)

The access control tests particularly shine:

  • Anonymous users correctly see 0 posts (denied by @@deny('all', auth() == null))
  • Post owners see all posts (allowed by @@allow('all', auth().id == authorId))
  • Other authenticated users see only published posts (allowed by @@allow('read', published))

🏆 Conclusion

This is a solid addition that enhances the project's testing coverage across different runtime environments. The code quality is good and follows established patterns. The main concerns are around resource management and configuration consistency, but these don't block the core functionality.

Recommendation: Approve with the resource leak fix as a priority improvement.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4646028 and 53ce6b7.

⛔ Files ignored due to path filters (1)
  • tests/runtimes/edge-runtime/.DS_Store is excluded by !**/.DS_Store
📒 Files selected for processing (3)
  • tests/runtimes/bun/bun-e2e.test.ts (3 hunks)
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts (1 hunks)
  • tests/runtimes/edge-runtime/scripts/generate.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/runtimes/edge-runtime/scripts/generate.ts
  • tests/runtimes/edge-runtime/edge-runtime-e2e.test.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

  • tests/runtimes/bun/bun-e2e.test.ts
⏰ 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, sqlite)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: claude-review
🔇 Additional comments (2)
tests/runtimes/bun/bun-e2e.test.ts (2)

18-20: Good practice: proper test cleanup.

The afterEach hook correctly disconnects the database client after each test, preventing connection leaks. The optional chaining safely handles cases where _db might be undefined.


23-23: Well done: assignment pattern addresses previous type-safety concern.

The pattern const db = (_db = await createClient(...)) gives each test a strongly-typed, non-optional local client while preserving the shared _db reference for cleanup. This elegantly resolves the previous review concern about using a possibly-undefined client directly in test assertions.

Also applies to: 53-53

Also applies to: 53-53

@ymc9 ymc9 merged commit d894f4b into dev Dec 2, 2025
7 checks passed
@ymc9 ymc9 deleted the test/edge-runtime branch December 2, 2025 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants