Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR implements neon-testing v3.0: the Vitest-only core is refactored into a runner-agnostic Changesneon-testing v3.0: Bun support, TS-native, runner-agnostic core
Sequence Diagram(s)sequenceDiagram
participant TestFile as Test file (Vitest or Bun)
participant Entry as neon-testing/vitest or /bun
participant Core as makeNeonTestingCore
participant Setup as neon-testing/setup (preload)
participant NeonAPI as Neon Branch API
Setup->>Setup: delete process.env.DATABASE_URL
TestFile->>Entry: import makeNeonTesting
Entry->>Core: makeNeonTestingCore({ hooks: runner.beforeAll/afterAll, ... })
Core-->>TestFile: neonTesting(overrides)
TestFile->>Core: neonTesting() called at file scope
Core->>Core: hooks.beforeAll(setup)
Core->>NeonAPI: createProjectBranch(parentId, expiresAt)
NeonAPI-->>Core: branch connection string
Core->>Core: process.env.DATABASE_URL = connectionString
TestFile->>TestFile: run tests using DATABASE_URL
Core->>Core: hooks.afterAll(teardown)
Core->>NeonAPI: deleteProjectBranch
Core->>Core: delete process.env.DATABASE_URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests-bun/drivers/ws-neon-drizzle.test.ts (1)
65-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winshared root cause: websocket teardown is not exception-safe
both suites rely on explicit close for bun, but
end()currently executes only on success paths. make teardown unconditional withtry/finallyin every test that opens a connection.
tests-bun/drivers/ws-neon-drizzle.test.ts#L65-L132: wrap each test body intry/finallyand callawait end()infinally.tests-bun/drivers/ws-neon.test.ts#L41-L108: apply the sametry/finallyteardown pattern for all tests usingmakeDb(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-bun/drivers/ws-neon-drizzle.test.ts` around lines 65 - 132, The websocket teardown via end() is not exception-safe because it only executes on the success path. At tests-bun/drivers/ws-neon-drizzle.test.ts lines 65-132, wrap the test body of each of the three tests ("create table", "tests are not isolated within a test file", "interactive transactions are supported") in a try/finally block, moving the await end() call from the end of the test body into the finally block to ensure the connection is unconditionally closed. Apply the same try/finally teardown pattern to all tests using makeDb(...) in tests-bun/drivers/ws-neon.test.ts lines 41-108.src/core.ts (1)
342-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winremove the uncaughtException listener after websocket cleanup
Line 348 installs a process-global listener but it is never removed. With multiple files using
autoCloseWebSockets, handlers accumulate and can causeMaxListenersExceededWarningplus unintended swallowing beyond teardown.proposed fix
factoryOptions.hooks.afterAll(async () => { delete process.env.DATABASE_URL; + let wsErrorHandlerAttached = false; + // Close all tracked Neon WebSocket connections before deleting the branch if (options.autoCloseWebSockets && neonSockets) { // Suppress Neon WebSocket "Connection terminated unexpectedly" error process.prependListener("uncaughtException", neonWsErrorHandler); + wsErrorHandlerAttached = true; // Close tracked Neon WebSocket connections before deleting the branch neonSockets.forEach((ws) => ws.close()); } - if (options.deleteBranch !== false) { - await deleteBranch(); + try { + if (options.deleteBranch !== false) { + await deleteBranch(); + } + } finally { + if (wsErrorHandlerAttached) { + process.removeListener("uncaughtException", neonWsErrorHandler); + } } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core.ts` around lines 342 - 352, The process.prependListener call for "uncaughtException" with neonWsErrorHandler on line 348 is never removed, causing the listener to accumulate across multiple test file executions when autoCloseWebSockets is enabled, leading to MaxListenersExceededWarning and unintended exception swallowing beyond the intended teardown phase. After the neonSockets.forEach loop completes its cleanup, remove the listener by calling process.removeListener with the same event name and handler function to ensure it does not persist beyond the WebSocket closure and does not interfere with subsequent test teardowns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish.yml:
- Line 24: Replace all floating version tags with immutable commit SHAs across
the workflow files to strengthen supply-chain integrity. In
.github/workflows/publish.yml, update line 24 to pin actions/checkout from `@v6`
to its full commit SHA, line 32 to pin actions/setup-node from `@v6` to its full
commit SHA, line 70 to pin actions/checkout from `@v6` to its full commit SHA, and
line 76 to pin actions/setup-node from `@v6` to its full commit SHA. In
.github/workflows/test.yml, update line 38 to pin actions/checkout from `@v6` to
its full commit SHA and line 49 to pin actions/setup-node from `@v6` to its full
commit SHA. Each uses statement should reference the complete commit SHA of the
action version instead of the semantic version tag.
- Around line 68-71: The checkout action in the publish job is persisting the
GITHUB_TOKEN to .git/config by default, which is a security risk since no
subsequent steps require git authentication. Add the `persist-credentials:
false` parameter to the `actions/checkout@v6` action in the publish job checkout
step to disable this credential persistence, matching the security pattern
already implemented in the check job.
In `@package.json`:
- Line 3: Update the version field in package.json from "2.7.0" to "3.0.0" to
reflect the breaking changes introduced in this PR. Since the PR contains
breaking changes to the public API and package surface, it requires a major
version bump according to semantic versioning conventions rather than a minor
version increment.
In `@tests-bun/smoke.test.ts`:
- Around line 9-13: The pool cleanup in the test case is not guaranteed to
execute if the query or assertion fails, leaving connections open for subsequent
tests. Wrap the pool.query() call and expect() assertion in a try block, and
move the pool.end() call to a finally block to ensure the pool is always
properly closed regardless of whether the query succeeds or the assertion
passes.
In `@tests-vitest/drivers/tcp-pg-drizzle.test.ts`:
- Around line 34-56: The connection teardown code is currently placed only in
the success path of each test function, causing resource leaks if any query or
assertion fails. Move the teardown code into a finally block to ensure it always
executes regardless of test outcome. In
tests-vitest/drivers/tcp-pg-drizzle.test.ts (lines 34-56), wrap await
pool.$client.end() in finally. In tests-vitest/drivers/tcp-pg.test.ts (lines
20-42), wrap await pool.end() in finally. In
tests-vitest/drivers/ws-neon-drizzle.test.ts (lines 65-87), wrap await end() in
finally. In tests-vitest/drivers/ws-neon.test.ts (lines 41-63), wrap await end()
in finally. This ensures proper cleanup even if test assertions or database
operations fail.
In `@tests-vitest/drivers/tcp-pg.test.ts`:
- Line 45: On line 45 of tests-vitest/drivers/tcp-pg.test.ts where the Pool is
instantiated, add a non-null assertion to DATABASE_URL (using the ! operator
after process.env.DATABASE_URL) to match the null guards already applied on
lines 21 and 66 in the same file. This prevents the pg Pool from falling back to
PostgreSQL environment variables when DATABASE_URL is undefined, which could
cause the integration test to connect to an unintended database.
In `@tests-vitest/smoke.test.ts`:
- Around line 9-13: The pool.end() call is only executed on the happy path when
all assertions pass, which means if the query or expect statements fail, the
connection remains open and leaks across the test process. Refactor the test by
wrapping the pool.query() call and the expect assertion in a try block, then
place the await pool.end() call in a finally block to ensure the database
connection is always closed whether the test succeeds or fails.
---
Outside diff comments:
In `@src/core.ts`:
- Around line 342-352: The process.prependListener call for "uncaughtException"
with neonWsErrorHandler on line 348 is never removed, causing the listener to
accumulate across multiple test file executions when autoCloseWebSockets is
enabled, leading to MaxListenersExceededWarning and unintended exception
swallowing beyond the intended teardown phase. After the neonSockets.forEach
loop completes its cleanup, remove the listener by calling
process.removeListener with the same event name and handler function to ensure
it does not persist beyond the WebSocket closure and does not interfere with
subsequent test teardowns.
In `@tests-bun/drivers/ws-neon-drizzle.test.ts`:
- Around line 65-132: The websocket teardown via end() is not exception-safe
because it only executes on the success path. At
tests-bun/drivers/ws-neon-drizzle.test.ts lines 65-132, wrap the test body of
each of the three tests ("create table", "tests are not isolated within a test
file", "interactive transactions are supported") in a try/finally block, moving
the await end() call from the end of the test body into the finally block to
ensure the connection is unconditionally closed. Apply the same try/finally
teardown pattern to all tests using makeDb(...) in
tests-bun/drivers/ws-neon.test.ts lines 41-108.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85df39a9-86ea-4c3f-87b7-8eaa45eb9e0e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.github/workflows/publish.yml.github/workflows/test.yml.vscode/settings.jsonAGENTS.mdCLAUDE.mdREADME.mdbunfig.tomlexamples/connection-uri.test.tsexamples/minimal.test.tsexamples/neon-testing.tsexamples/options-validation.test.tsexamples/recommended.test.tspackage.jsonsrc/browser-empty.tssrc/bun.tssrc/core.tssrc/lib/barrier.test.tssrc/lib/barrier.tssrc/lib/expires-in.test.tssrc/lib/expires-in.tssrc/lib/singleton.test.tssrc/lib/singleton.tssrc/lib/ssl.test.tssrc/lib/ssl.tssrc/lib/with-retry.test.tssrc/lib/with-retry.tssrc/lib/ws-error.test.tssrc/lib/ws-error.tssrc/setup.tssrc/utils.tssrc/vite-plugin.tssrc/vitest-setup.tssrc/vitest.tstests-bun/capture-database-url.tstests-bun/drivers/http-neon-drizzle.test.tstests-bun/drivers/http-neon.test.tstests-bun/drivers/tcp-pg-drizzle.test.tstests-bun/drivers/tcp-pg.test.tstests-bun/drivers/tcp-postgres-drizzle.test.tstests-bun/drivers/tcp-postgres.test.tstests-bun/drivers/ws-neon-drizzle.test.tstests-bun/drivers/ws-neon.test.tstests-bun/neon-testing.tstests-bun/poison-database-url.tstests-bun/preload.test.tstests-bun/smoke.test.tstests-vitest/capture-database-url.tstests-vitest/drivers/http-neon-drizzle.test.tstests-vitest/drivers/http-neon.test.tstests-vitest/drivers/tcp-pg-drizzle.test.tstests-vitest/drivers/tcp-pg.test.tstests-vitest/drivers/tcp-postgres-drizzle.test.tstests-vitest/drivers/tcp-postgres.test.tstests-vitest/drivers/ws-neon-drizzle.test.tstests-vitest/drivers/ws-neon.test.tstests-vitest/neon-testing.tstests-vitest/poison-database-url.tstests-vitest/preload.test.tstests-vitest/smoke.test.tstsconfig.build.jsontsconfig.jsonvitest.config.ts
💤 Files with no reviewable changes (4)
- src/vitest-setup.ts
- src/vite-plugin.ts
- src/browser-empty.ts
- tsconfig.build.json
| steps: | ||
| - name: Checkout | ||
| # https://github.com/actions/checkout | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# verify mutable action refs (non-SHA) in workflow files
rg -nP '^\s*uses:\s*[^@]+@(?![0-9a-fA-F]{40}\b)[^\s]+' .github/workflows/*.ymlRepository: starmode-base/neon-testing
Length of output: 684
Pin all workflow action references to immutable commit SHAs
Using floating action tags (@v*) weakens supply-chain integrity. Pin the following actions to full commit SHAs instead:
.github/workflows/publish.yml#L24:actions/checkout@v6.github/workflows/publish.yml#L32:actions/setup-node@v6.github/workflows/publish.yml#L38:oven-sh/setup-bun@v2.github/workflows/publish.yml#L70:actions/checkout@v6.github/workflows/publish.yml#L76:actions/setup-node@v6.github/workflows/publish.yml#L118:softprops/action-gh-release@v2.github/workflows/test.yml#L38:actions/checkout@v6.github/workflows/test.yml#L49:actions/setup-node@v6.github/workflows/test.yml#L55:oven-sh/setup-bun@v2
🧰 Tools
🪛 zizmor (1.25.2)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/publish.yml#L24-L24(this comment).github/workflows/publish.yml#L32-L32.github/workflows/publish.yml#L70-L70.github/workflows/publish.yml#L76-L76.github/workflows/test.yml#L38-L38.github/workflows/test.yml#L49-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/publish.yml at line 24, Replace all floating version tags
with immutable commit SHAs across the workflow files to strengthen supply-chain
integrity. In .github/workflows/publish.yml, update line 24 to pin
actions/checkout from `@v6` to its full commit SHA, line 32 to pin
actions/setup-node from `@v6` to its full commit SHA, line 70 to pin
actions/checkout from `@v6` to its full commit SHA, and line 76 to pin
actions/setup-node from `@v6` to its full commit SHA. In
.github/workflows/test.yml, update line 38 to pin actions/checkout from `@v6` to
its full commit SHA and line 49 to pin actions/setup-node from `@v6` to its full
commit SHA. Each uses statement should reference the complete commit SHA of the
action version instead of the semantic version tag.
Source: Linters/SAST tools
| - name: Checkout | ||
| # https://github.com/actions/checkout | ||
| uses: actions/checkout@v6 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/publish.ymlRepository: starmode-base/neon-testing
Length of output: 5142
Disable credential persistence in the publish checkout step
The publish job has write-capable permissions (contents: write). By default, checkout@v6 persists the GITHUB_TOKEN to .git/config, which is unnecessary since no subsequent steps require git authentication. This aligns with the security pattern already established in the check job (lines 25-28).
proposed fix
- name: Checkout
# https://github.com/actions/checkout
uses: actions/checkout@v6
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout | |
| # https://github.com/actions/checkout | |
| uses: actions/checkout@v6 | |
| - name: Checkout | |
| # https://github.com/actions/checkout | |
| uses: actions/checkout@v6 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 68-70: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 70-70: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/publish.yml around lines 68 - 71, The checkout action in
the publish job is persisting the GITHUB_TOKEN to .git/config by default, which
is a security risk since no subsequent steps require git authentication. Add the
`persist-credentials: false` parameter to the `actions/checkout@v6` action in
the publish job checkout step to disable this credential persistence, matching
the security pattern already implemented in the check job.
Source: Linters/SAST tools
| @@ -1,12 +1,14 @@ | |||
| { | |||
| "name": "neon-testing", | |||
| "version": "2.7.0", | |||
There was a problem hiding this comment.
bump package version to a new major before publishing
Line 3 is still 2.7.0, but this PR introduces breaking public API/package-surface changes. Publishing under 2.x would violate semver expectations for consumers.
suggested update
- "version": "2.7.0",
+ "version": "3.0.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "version": "2.7.0", | |
| "version": "3.0.0", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 3, Update the version field in package.json from
"2.7.0" to "3.0.0" to reflect the breaking changes introduced in this PR. Since
the PR contains breaking changes to the public API and package surface, it
requires a major version bump according to semantic versioning conventions
rather than a minor version increment.
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); | ||
| const result = await pool.query("SELECT 1 AS value"); | ||
| expect(result.rows[0].value).toBe(1); | ||
| await pool.end(); | ||
| }); |
There was a problem hiding this comment.
ensure the pool is always closed on failures.
if the query or assertion fails, the current flow skips cleanup and can leave an open connection for subsequent tests.
suggested fix
test("creates a branch and exposes a working DATABASE_URL", async () => {
expect(process.env.DATABASE_URL).toBeDefined();
const pool = new Pool({ connectionString: process.env.DATABASE_URL });
- const result = await pool.query("SELECT 1 AS value");
- expect(result.rows[0].value).toBe(1);
- await pool.end();
+ try {
+ const result = await pool.query("SELECT 1 AS value");
+ expect(result.rows[0].value).toBe(1);
+ } finally {
+ await pool.end();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); | |
| const result = await pool.query("SELECT 1 AS value"); | |
| expect(result.rows[0].value).toBe(1); | |
| await pool.end(); | |
| }); | |
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); | |
| try { | |
| const result = await pool.query("SELECT 1 AS value"); | |
| expect(result.rows[0].value).toBe(1); | |
| } finally { | |
| await pool.end(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests-bun/smoke.test.ts` around lines 9 - 13, The pool cleanup in the test
case is not guaranteed to execute if the query or assertion fails, leaving
connections open for subsequent tests. Wrap the pool.query() call and expect()
assertion in a try block, and move the pool.end() call to a finally block to
ensure the pool is always properly closed regardless of whether the query
succeeds or the assertion passes.
| test("create table", async () => { | ||
| const pool = makeDb(process.env.DATABASE_URL!); | ||
|
|
||
| await pool.execute(` | ||
| CREATE TABLE users ( | ||
| id SERIAL PRIMARY KEY, | ||
| name TEXT NOT NULL UNIQUE | ||
| ) | ||
| `); | ||
|
|
||
| const newUser = await pool.execute(` | ||
| INSERT INTO users (name) | ||
| VALUES ('Ellen Ripley') | ||
| RETURNING * | ||
| `); | ||
| expect(newUser.rows[0]).toStrictEqual({ id: 1, name: "Ellen Ripley" }); | ||
|
|
||
| const users = await pool.execute(`SELECT * FROM users`); | ||
| expect(users.rows).toStrictEqual([{ id: 1, name: "Ellen Ripley" }]); | ||
|
|
||
| // 👎 Have to manually end the connection unless disabling `deleteBranch` | ||
| await pool.$client.end(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the test files exist and examine the tcp-pg-drizzle.test.ts file
fd -t f "tcp-pg-drizzle.test.ts" tests-vitest/Repository: starmode-base/neon-testing
Length of output: 114
🏁 Script executed:
# Examine the tcp-pg-drizzle test file around the specified lines
wc -l tests-vitest/drivers/tcp-pg-drizzle.test.tsRepository: starmode-base/neon-testing
Length of output: 118
🏁 Script executed:
# Read the tcp-pg-drizzle.test.ts file to verify the snippet
cat -n tests-vitest/drivers/tcp-pg-drizzle.test.tsRepository: starmode-base/neon-testing
Length of output: 4060
🏁 Script executed:
# Check all 4 test files exist
fd -t f "tcp-pg-drizzle.test.ts|tcp-pg.test.ts|ws-neon-drizzle.test.ts|ws-neon.test.ts" tests-vitest/Repository: starmode-base/neon-testing
Length of output: 232
🏁 Script executed:
# Check tcp-pg.test.ts
cat -n tests-vitest/drivers/tcp-pg.test.tsRepository: starmode-base/neon-testing
Length of output: 3539
🏁 Script executed:
# Check ws-neon-drizzle.test.ts
cat -n tests-vitest/drivers/ws-neon-drizzle.test.tsRepository: starmode-base/neon-testing
Length of output: 4896
🏁 Script executed:
# Check ws-neon.test.ts
cat -n tests-vitest/drivers/ws-neon.test.tsRepository: starmode-base/neon-testing
Length of output: 4068
Move connection teardown into finally blocks to prevent resource leaks on test failures across all suites
All three test functions in each of these files place manual teardown (connection.end()) only at the success path, causing open connections to leak if any query or assertion fails:
tests-vitest/drivers/tcp-pg-drizzle.test.ts: Wrapawait pool.$client.end()infinally(affects all tests in this file)tests-vitest/drivers/tcp-pg.test.ts: Wrapawait pool.end()infinally(affects all tests in this file)tests-vitest/drivers/ws-neon-drizzle.test.ts: Wrapawait end()infinally(affects all tests in this file)tests-vitest/drivers/ws-neon.test.ts: Wrapawait end()infinally(affects all tests in this file)
📍 Affects 4 files
tests-vitest/drivers/tcp-pg-drizzle.test.ts#L34-L56(this comment)tests-vitest/drivers/tcp-pg.test.ts#L20-L42tests-vitest/drivers/ws-neon-drizzle.test.ts#L65-L87tests-vitest/drivers/ws-neon.test.ts#L41-L63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests-vitest/drivers/tcp-pg-drizzle.test.ts` around lines 34 - 56, The
connection teardown code is currently placed only in the success path of each
test function, causing resource leaks if any query or assertion fails. Move the
teardown code into a finally block to ensure it always executes regardless of
test outcome. In tests-vitest/drivers/tcp-pg-drizzle.test.ts (lines 34-56), wrap
await pool.$client.end() in finally. In tests-vitest/drivers/tcp-pg.test.ts
(lines 20-42), wrap await pool.end() in finally. In
tests-vitest/drivers/ws-neon-drizzle.test.ts (lines 65-87), wrap await end() in
finally. In tests-vitest/drivers/ws-neon.test.ts (lines 41-63), wrap await end()
in finally. This ensures proper cleanup even if test assertions or database
operations fail.
| }); | ||
|
|
||
| test("tests are not isolated within a test file", async () => { | ||
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests-vitest/drivers/tcp-pg.test.ts | head -100Repository: starmode-base/neon-testing
Length of output: 3539
🌐 Web query:
pg nodejs pool undefined connectionString fallback behavior
💡 Result:
In node-postgres, the Pool constructor does not have a "fallback" behavior that automatically looks for a specific connection string when the value is undefined [1]. If you pass an undefined connectionString to the Pool constructor (e.g., new Pool({ connectionString: undefined })), the library does not treat it as a trigger to look for a specific environment variable like DATABASE_URL or POSTGRES_URL [1]. Instead, the Pool will attempt to connect using its default parameters (e.g., localhost, default port 5432, default user, etc.), which are often combined with relevant environment variables if they exist in the process environment [1]. Key points regarding this behavior: 1. No Automatic Lookup: The Pool does not internally look for a "default" environment variable if the connectionString property is explicitly passed as undefined [1]. It behaves as if no connection string was provided [1]. 2. Default Connection Parameters: When no specific connection configuration is provided, the driver relies on standard PostgreSQL environment variables (such as PGHOST, PGUSER, PGDATABASE, PGPASSWORD, and PGPORT) if they are set in the system environment [1]. 3. Explicit Configuration is Best: If you need to support fallback behavior (e.g., using a local development default if an environment variable is missing), you must implement this logic in your application code before initializing the Pool [1]: const pool = new Pool({ connectionString: process.env.DATABASE_URL || 'postgresql://localhost:5432/my_default_db', }); Note that certain third-party libraries (like @vercel/postgres) may provide wrapper functions that include custom error handling or automatic environment variable lookups, but these are not features of the core node-postgres (pg) library itself [2].
Citations:
- 1: "Server does not support SSL connections" when connectionString is undefined brianc/node-postgres#1391
- 2: https://stackoverflow.com/questions/76175232/vercel-postgres-nuxt-starter-template-gives-missing-connection-string-when-r
🌐 Web query:
node-postgres Pool constructor connectionString undefined environment variables
💡 Result:
In node-postgres, the Pool constructor and environment variables interact to determine connection settings in a specific priority order [1][2]. When you instantiate a Pool without arguments (e.g., new Pool), node-postgres automatically looks for and uses standard PostgreSQL environment variables to configure the connection [1][2]. These include variables such as PGUSER, PGPASSWORD, PGHOST, PGPORT, and PGDATABASE [1][2]. If you provide a configuration object to the Pool constructor, those values take precedence over environment variables [1][2]. If you specifically pass a connectionString property within that object, it is parsed to establish the connection details [1][3]. Regarding your query about undefined values: 1. Environment Variable Fallbacks: If you initialize the Pool without specific configuration (new Pool), the library defaults to specific environment variables [1][3]. If those environment variables are also undefined in your environment, the library falls back to system defaults (e.g., PGHOST defaults to localhost, PGPORT to 5432, and PGUSER/PGDATABASE to the current OS user) [1][3]. 2. Constructor Configuration: If you pass an object to the Pool constructor, explicit values provided in that object will override any environment variables [1]. If a property in your configuration object is explicitly set to undefined (e.g., { host: undefined }), it will generally be treated as if that property was omitted, allowing the library to fall back to the relevant environment variable or system default for that specific setting [1][2]. 3. Connection Strings: If you use the connectionString property, it must be a valid string [1][3]. If you pass an undefined value as the connectionString, the pool may fail to initialize or behave unexpectedly because it expects a valid URI format [1][3]. If you are working in an environment (like some cloud platforms) where the connection string might be missing or undefined, you should check for its existence before passing it to the Pool constructor [1][2]. If you need to handle connection configurations dynamically, it is common practice to parse the connection string separately or check for the existence of your environment variables before instantiating the pool to ensure you are passing valid configuration objects [2][4].
Citations:
- 1: https://node-postgres.com/features/connecting
- 2: https://github.com/brianc/node-postgres/blob/master/packages/pg-pool/README.md
- 3: https://github.com/brianc/node-postgres/blob/master/docs/pages/features/connecting.mdx
- 4: pg-connection-string ConnectionOptions interface is not compatible with PoolConfig brianc/node-postgres#2280
Add null guard for DATABASE_URL to prevent unexpected database connections
Line 45 passes process.env.DATABASE_URL without a non-null assertion. When undefined, the pg Pool falls back to PostgreSQL environment variables (like PGHOST, PGDATABASE, etc.), potentially connecting to an unintended database—critical in integration tests. Lines 21 and 66 in this same file already guard with !, making line 45 inconsistent and risky.
proposed fix
- const pool = new Pool({ connectionString: process.env.DATABASE_URL });
+ const databaseUrl = process.env.DATABASE_URL;
+ if (!databaseUrl) throw new Error("DATABASE_URL is not set");
+ const pool = new Pool({ connectionString: databaseUrl });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); | |
| const databaseUrl = process.env.DATABASE_URL; | |
| if (!databaseUrl) throw new Error("DATABASE_URL is not set"); | |
| const pool = new Pool({ connectionString: databaseUrl }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests-vitest/drivers/tcp-pg.test.ts` at line 45, On line 45 of
tests-vitest/drivers/tcp-pg.test.ts where the Pool is instantiated, add a
non-null assertion to DATABASE_URL (using the ! operator after
process.env.DATABASE_URL) to match the null guards already applied on lines 21
and 66 in the same file. This prevents the pg Pool from falling back to
PostgreSQL environment variables when DATABASE_URL is undefined, which could
cause the integration test to connect to an unintended database.
| const pool = new Pool({ connectionString: process.env.DATABASE_URL }); | ||
| const result = await pool.query("SELECT 1 AS value"); | ||
| expect(result.rows[0].value).toBe(1); | ||
| await pool.end(); | ||
| }); |
There was a problem hiding this comment.
ensure the pool is always closed on failure paths
pool.end() currently runs only on the happy path. Wrap query/assertions in try/finally so failures do not leak open connections across the test process.
suggested patch
test("creates a branch and exposes a working DATABASE_URL", async () => {
expect(process.env.DATABASE_URL).toBeDefined();
const pool = new Pool({ connectionString: process.env.DATABASE_URL });
- const result = await pool.query("SELECT 1 AS value");
- expect(result.rows[0].value).toBe(1);
- await pool.end();
+ try {
+ const result = await pool.query("SELECT 1 AS value");
+ expect(result.rows[0].value).toBe(1);
+ } finally {
+ await pool.end();
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests-vitest/smoke.test.ts` around lines 9 - 13, The pool.end() call is only
executed on the happy path when all assertions pass, which means if the query or
expect statements fail, the connection remains open and leaks across the test
process. Refactor the test by wrapping the pool.query() call and the expect
assertion in a try block, then place the await pool.end() call in a finally
block to ensure the database connection is always closed whether the test
succeeds or fails.
1 participant
Fixes #15
Fixes #11
Summary by CodeRabbit
Release Notes
New Features
neon-testing/setup)Documentation
Chores