fix(policy): sql execution error with before()
#373
Conversation
|
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. WalkthroughAdds a TEST_DB_PROVIDER environment variable to the Build & Test GH Actions step and updates one E2E test to import test utilities, log the active DB provider at runtime, and pass a test logger to the policy test client. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/e2e/orm/policy/crud/post-update.test.ts (1)
⏰ 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)
🔇 Additional comments (1)
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 Complete ✅I've analyzed this PR that fixes an SQL execution error with the 📋 SummaryThis PR addresses a test infrastructure issue that was preventing proper testing of the ✅ Code Quality & Best PracticesExcellent targeted fix:
Workflow Fix (
Test Enhancement (
🐛 Bug AnalysisRoot Cause: Fix Effectiveness: ✅ Excellent
🔒 Security Considerations✅ No security concerns - Changes are limited to:
⚡ Performance Considerations✅ Minimal performance impact:
🧪 Test Coverage AssessmentBefore Fix: ❌ Tests were likely running only against one database provider due to environment variable issue After Fix: ✅ Significantly Improved
Test Quality: ✅ Excellent
🎯 Architecture UnderstandingThe
The SQL generation for this likely differs between SQLite and PostgreSQL, which is why proper cross-database testing was crucial. 🏆 Overall AssessmentCode Quality: ⭐⭐⭐⭐⭐ (Excellent) ✅ RecommendationAPPROVE - This is a well-executed fix that:
The PR title accurately reflects the fix, and while a description would have been helpful, the changes are self-explanatory for a bug fix of this nature. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds debugging instrumentation and fixes environment variable configuration in the test workflow. The changes enable proper database provider selection across different test environments.
- Added
getTestDbProviderimport and console logging to debug database provider selection - Fixed environment variable passing in GitHub Actions workflow by setting it in both
envandrunsections
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/e2e/orm/policy/crud/post-update.test.ts | Added import for getTestDbProvider and console.log statement to debug provider selection |
| .github/workflows/build-test.yml | Added env block to properly set TEST_DB_PROVIDER environment variable for test step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| it('works with before function', async () => { | ||
| console.log('PROVIDER:', getTestDbProvider()); |
There was a problem hiding this comment.
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.
| console.log('PROVIDER:', getTestDbProvider()); |
| - name: Test | ||
| env: | ||
| TEST_DB_PROVIDER: ${{ matrix.provider }} | ||
| run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test |
There was a problem hiding this comment.
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.
| run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test | |
| run: pnpm run test |
|
Claude finished @ymc9's task —— View job PR Review: fix(policy): sql execution error with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-test.yml(1 hunks)tests/e2e/orm/policy/crud/post-update.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/orm/policy/crud/post-update.test.ts (1)
packages/testtools/src/client.ts (1)
getTestDbProvider(19-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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
| env: | ||
| TEST_DB_PROVIDER: ${{ matrix.provider }} | ||
| run: TEST_DB_PROVIDER=${{ matrix.provider }} pnpm run test |
There was a problem hiding this comment.
🛠️ 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:.
| }); | ||
|
|
||
| it('works with before function', async () => { | ||
| console.log('PROVIDER:', getTestDbProvider()); |
There was a problem hiding this comment.
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.
Summary by CodeRabbit
Chores
Tests