Skip to content

fix(policy): sql execution error with before() #373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fix(policy): sql execution error with before() #373

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2d2079
fix(policy): sql execution error with `before()`
ymc9 Nov 6, 2025
b34b64a
update
ymc9 Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/build-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,6 @@ jobs:
run: pnpm run lint

- name: Test
env:
TEST_DB_PROVIDER: ${{ matrix.provider }}
run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The environment variable is now set redundantly both in the env block (line 81) and inline in the run command. The inline TEST_DB_PROVIDER=${{ matrix.provider }} prefix should be removed since it's already defined in the env section above.

Suggested change
run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test
run: pnpm run test

Copilot uses AI. Check for mistakes.
Comment on lines +80 to 82
Copy link

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Remove redundant environment variable assignment.

The TEST_DB_PROVIDER environment variable is being set twice: once in the env: block (lines 80-81) and again as a command prefix on line 82. When an environment variable is declared in the step's env: block, it's automatically available to all commands in that step, making the command prefix redundant.

Apply this diff to remove the redundant prefix:

               env:
                   TEST_DB_PROVIDER: ${{ matrix.provider }}
-              run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test
+              run: pnpm run test
🤖 Prompt for AI Agents 
.github/workflows/build-test.yml around lines 80 to 82: the TEST_DB_PROVIDER env
var is set both in the step's env: block and again as a command prefix, so
remove the redundant command-prefix assignment; keep the env: entry
(TEST_DB_PROVIDER: ${{ matrix.provider }}) and change the run line to simply run
the tests (e.g., pnpm run test) so the environment variable is provided only via
env:.

3 changes: 2 additions & 1 deletion tests/e2e/orm/policy/crud/post-update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from 'vitest';
import { createPolicyTestClient } from '@zenstackhq/testtools';
import { createPolicyTestClient, getTestDbProvider } from '@zenstackhq/testtools';

describe('Policy post-update tests', () => {
it('allows post-update by default', async () => {
Expand Down Expand Up @@ -94,6 +94,7 @@ describe('Policy post-update tests', () => {
});

it('works with before function', async () => {
console.log('PROVIDER:', getTestDbProvider());
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Debug console.log statement should be removed before merging to production. This appears to be temporary debugging code that was left in the test file.

Suggested change
console.log('PROVIDER:', getTestDbProvider());

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove debug logging before merging.

This console.log statement appears to be temporary debugging code. Debug artifacts should be removed from test files before merging to production.

Apply this diff to remove the debug statement:

-        console.log('PROVIDER:', getTestDbProvider());

After removing this line, if getTestDbProvider is no longer used in the file, also remove it from the import statement on line 2:

-import { createPolicyTestClient, getTestDbProvider } from '@zenstackhq/testtools';
+import { createPolicyTestClient } from '@zenstackhq/testtools';

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tests/e2e/orm/policy/crud/post-update.test.ts around line 97, remove the
temporary debug statement "console.log('PROVIDER:', getTestDbProvider());" and
save the file; then check the import on line 2 and if getTestDbProvider is no
longer referenced anywhere in the file, remove it from that import to avoid an
unused import.

const db = await createPolicyTestClient(
`
model Foo {
Expand Down
Loading