feat(cli): route starter smoke through testkit-postgres - #673
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a DB-backed starter smoke path using Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Starter Smoke Test
participant Helper as createStarterPostgresTestkitClient
participant Config as ztd.config.json loader
participant Pool as pg.Pool
participant Postgres as Postgres DB
Test->>Helper: call createStarterPostgresTestkitClient(options?)
Helper->>Config: loadStarterPostgresDefaults(rootDir?)
Config-->>Helper: defaultSchema, searchPath, ztdRootDir
Helper->>Pool: create pg.Pool(connectionString)
Helper-->>Test: PostgresTestkitClient { query, defaultSchema, dispose }
Test->>Helper: client.query(sql, params)
Helper->>Pool: pool.query(sql, params)
Pool->>Postgres: execute SQL
Postgres-->>Pool: rows
Pool-->>Helper: result
Helper-->>Test: result.rows
Test->>Helper: client.dispose()
Helper->>Pool: pool.end()
Pool-->>Postgres: close connections
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ztd-cli/README.md (1)
51-51: Consider avoiding a repo-internal path here.For readers outside the monorepo context, referencing the package README directly is clearer than a repository path string.
✏️ Optional wording tweak
-If you want the fixture-resolution details, read `packages/testkit-postgres/README.md` after the starter smoke path. +If you want fixture-resolution details, read the `@rawsql-ts/testkit-postgres` README after the starter smoke path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/README.md` at line 51, The README line "If you want the fixture-resolution details, read `packages/testkit-postgres/README.md` after the starter smoke path." uses a repo-internal path; replace it with a repository-agnostic reference to the package README (for example, "see the testkit-postgres package README" or link to "packages/testkit-postgres (testkit-postgres) README") so external readers can find fixture-resolution details without relying on an internal path; update the sentence in packages/ztd-cli/README.md accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ztd-cli/templates/src/features/smoke/tests/smoke.queryspec.test.ts`:
- Around line 40-41: The teardown currently awaits client.close() then
pool.end(), which skips pool shutdown if client.close() throws; update the test
cleanup (the block using client.close() and pool.end()) to ensure pool.end()
always runs by handling client.close() errors—either await client.close() inside
a try/catch and always await pool.end() in a finally block, or use
Promise.allSettled([client.close(), pool.end()]) so both client.close() and
pool.end() are attempted and errors are surfaced appropriately (reference
client.close and pool.end in the test cleanup).
---
Nitpick comments:
In `@packages/ztd-cli/README.md`:
- Line 51: The README line "If you want the fixture-resolution details, read
`packages/testkit-postgres/README.md` after the starter smoke path." uses a
repo-internal path; replace it with a repository-agnostic reference to the
package README (for example, "see the testkit-postgres package README" or link
to "packages/testkit-postgres (testkit-postgres) README") so external readers
can find fixture-resolution details without relying on an internal path; update
the sentence in packages/ztd-cli/README.md accordingly.
🪄 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: 4a2a669b-cd28-427f-8486-2feb61d8b54c
📒 Files selected for processing (14)
.changeset/serious-pigs-fold.mddocs/guide/sql-first-end-to-end-tutorial.mdpackages/testkit-postgres/README.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/features/README.mdpackages/ztd-cli/templates/src/features/smoke/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/smoke.queryspec.test.tspackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/furtherReading.docs.test.tspackages/ztd-cli/tests/init.command.test.tspackages/ztd-cli/tests/sqlFirstTutorial.docs.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ztd-cli/templates/tests/support/postgres-testkit.ts`:
- Around line 82-85: Update the thrown Error where connectionString is validated
(the block using connectionString = options.connectionString ??
process.env.ZTD_TEST_DATABASE_URL) so the error message reflects the actual
inputs; mention both options.connectionString and the ZTD_TEST_DATABASE_URL env
var (and remove or avoid referencing the specific ZTD_DB_PORT or single test
path). Ensure the message instructs the caller to set either
options.connectionString or the ZTD_TEST_DATABASE_URL environment variable and
includes the symbol names connectionString, options.connectionString, and
ZTD_TEST_DATABASE_URL for clarity.
- Around line 46-49: The current catch around
JSON.parse(readFileSync(configPath, 'utf8')) in the helper silently swallows all
errors and returns {}; change it to only treat "file not found" as optional and
rethrow other errors so malformed JSON or read failures surface—i.e., in the
function that returns StarterProjectConfigFile, catch the error, if it's an
ENOENT (file missing) return {}, otherwise rethrow the error (preserve the
original error message) to match packages/ztd-cli/src/utils/ztdProjectConfig.ts
behavior.
🪄 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: 6216ac2b-1d18-4a3f-8738-cfc9597ca2dc
📒 Files selected for processing (9)
packages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/utils/ztdProjectConfig.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/features/smoke/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/smoke.queryspec.test.tspackages/ztd-cli/templates/tests/support/postgres-testkit.tspackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.ts
✅ Files skipped from review due to trivial changes (4)
- packages/ztd-cli/templates/src/features/smoke/tests/README.md
- packages/ztd-cli/templates/src/features/smoke/README.md
- packages/ztd-cli/templates/README.md
- packages/ztd-cli/templates/src/features/smoke/tests/smoke.queryspec.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ztd-cli/tests/directoryFinding.docs.test.ts
- packages/ztd-cli/tests/init.command.test.ts
Comment on lines
277
to
+280
| const STARTER_DEV_DEPENDENCIES: Record<string, string> = { | ||
| pg: '^8.13.1', | ||
| '@types/pg': '^8.15.6' | ||
| '@types/pg': '^8.15.6', | ||
| '@rawsql-ts/testkit-postgres': '^0.15.4' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba packages/ztd-cli/src/commands/init.ts | sed -n '277,285p;1853,1861p;2282,2300p'Repository: mk3008/rawsql-ts
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the file and get total lines
FILE="packages/ztd-cli/src/commands/init.ts"
TOTAL=$(wc -l < "$FILE")
echo "=== File: $FILE (total: $TOTAL lines) ==="
# Section 1: Lines 275-285 (STARTER_DEV_DEPENDENCIES)
echo -e "\n=== Section 1: Lines 275-285 ==="
sed -n '275,285p' "$FILE" | awk '{print NR+274 ": " $0}'
# Section 2: Find LOCAL_SOURCE_STACK_PACKAGE_DIRS
echo -e "\n=== Section 2: LOCAL_SOURCE_STACK_PACKAGE_DIRS definition ==="
grep -n "LOCAL_SOURCE_STACK_PACKAGE_DIRS" "$FILE" | head -5
# Section 3: Find where STARTER_DEV_DEPENDENCIES is merged with Object.assign
echo -e "\n=== Section 3: Object.assign with STARTER_DEV_DEPENDENCIES ==="
grep -n "Object.assign.*STARTER_DEV_DEPENDENCIES" "$FILE"
# Section 4: Find the starter conditional block
echo -e "\n=== Section 4: if (starter) block ==="
grep -n "if (starter)" "$FILE" | head -3Repository: mk3008/rawsql-ts
Length of output: 1163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/ztd-cli/src/commands/init.ts"
# Show context around line 1860 (Object.assign with STARTER_DEV_DEPENDENCIES)
echo "=== Context around line 1860 (Object.assign STARTER_DEV_DEPENDENCIES) ==="
sed -n '1850,1875p' "$FILE" | awk '{print NR+1849 ": " $0}'
# Find buildLocalSourceStackDependencies calls
echo -e "\n=== buildLocalSourceStackDependencies calls ==="
grep -n "buildLocalSourceStackDependencies" "$FILE"
# Find where stackDependencies is initialized
echo -e "\n=== stackDependencies initialization ==="
grep -n "stackDependencies.*=" "$FILE" | head -10Repository: mk3008/rawsql-ts
Length of output: 1346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/ztd-cli/src/commands/init.ts"
# Read buildLocalSourceStackDependencies function
echo "=== buildLocalSourceStackDependencies function (lines 2282+) ==="
sed -n '2282,2320p' "$FILE" | awk '{print NR+2281 ": " $0}'
# Check if testkit-postgres is referenced elsewhere in init.ts
echo -e "\n=== References to testkit-postgres in init.ts ==="
grep -n "testkit-postgres" "$FILE"Repository: mk3008/rawsql-ts
Length of output: 2154
Link @rawsql-ts/testkit-postgres through the --local-source-root path.
When scaffolding with --local-source-root, the new direct @rawsql-ts/testkit-postgres starter dependency (line 280) is never rewritten to a local path. The starter deps are merged after local-source dependency resolution, which means Object.assign(stackDependencies, STARTER_DEV_DEPENDENCIES) at line 1860 unconditionally overwrites any local mappings. Even if added to LOCAL_SOURCE_STACK_PACKAGE_DIRS, the published version will still be installed instead of the local one.
Suggested fix
const LOCAL_SOURCE_STACK_PACKAGE_DIRS: Record<string, string> = {
'@rawsql-ts/sql-contract': path.join('packages', 'sql-contract'),
- '@rawsql-ts/testkit-core': path.join('packages', 'testkit-core')
+ '@rawsql-ts/testkit-core': path.join('packages', 'testkit-core'),
+ '@rawsql-ts/testkit-postgres': path.join('packages', 'testkit-postgres')
}; if (starter) {
Object.assign(stackDependencies, STARTER_DEV_DEPENDENCIES);
+ if (scaffoldProfile.dependencyProfile === 'local-source') {
+ Object.assign(stackDependencies, buildLocalSourceStackDependencies(rootDir, scaffoldProfile));
+ }
}There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ztd-cli/src/commands/init.ts (1)
277-285:⚠️ Potential issue | 🟠 MajorLink
@rawsql-ts/testkit-postgresthrough the--local-source-rootpath.When scaffolding with
--local-source-root, the new@rawsql-ts/testkit-postgresstarter dependency (line 280) will be installed from the npm registry rather than linked to the local monorepo. This happens becauseSTARTER_DEV_DEPENDENCIESis merged afterbuildLocalSourceStackDependenciesis called (line 1862), andtestkit-postgresis not included inLOCAL_SOURCE_STACK_PACKAGE_DIRS.Suggested fix
const LOCAL_SOURCE_STACK_PACKAGE_DIRS: Record<string, string> = { '@rawsql-ts/sql-contract': path.join('packages', 'sql-contract'), - '@rawsql-ts/testkit-core': path.join('packages', 'testkit-core') + '@rawsql-ts/testkit-core': path.join('packages', 'testkit-core'), + '@rawsql-ts/testkit-postgres': path.join('packages', 'testkit-postgres') };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/commands/init.ts` around lines 277 - 285, The starter currently installs `@rawsql-ts/testkit-postgres` from npm because LOCAL_SOURCE_STACK_PACKAGE_DIRS lacks that package and STARTER_DEV_DEPENDENCIES is merged after buildLocalSourceStackDependencies; fix by adding an entry for "@rawsql-ts/testkit-postgres": path.join('packages','testkit-postgres') to LOCAL_SOURCE_STACK_PACKAGE_DIRS so buildLocalSourceStackDependencies will link the local package (ensure the mapping is present before STARTER_DEV_DEPENDENCIES is merged).
🧹 Nitpick comments (1)
packages/ztd-cli/src/utils/ztdProjectConfig.ts (1)
262-267: Cache the normalized search path to avoid redundant computation.
normalizeSchemaList(overrides.searchPath)is evaluated twice (lines 263 and 264), andnormalizeSchemaList(overrides.ddl?.searchPath)is similarly evaluated twice (lines 265 and 266).♻️ Proposed refactor to cache normalized values
+ const normalizedOverrideSearchPath = normalizeSchemaList(overrides.searchPath); + const normalizedOverrideDdlSearchPath = normalizeSchemaList(overrides.ddl?.searchPath); const searchPath = - normalizeSchemaList(overrides.searchPath).length > 0 - ? normalizeSchemaList(overrides.searchPath) - : normalizeSchemaList(overrides.ddl?.searchPath).length > 0 - ? normalizeSchemaList(overrides.ddl?.searchPath) + normalizedOverrideSearchPath.length > 0 + ? normalizedOverrideSearchPath + : normalizedOverrideDdlSearchPath.length > 0 + ? normalizedOverrideDdlSearchPath : baseConfig.searchPath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/utils/ztdProjectConfig.ts` around lines 262 - 267, The searchPath computation calls normalizeSchemaList multiple times; cache the normalized results into local consts (e.g., normalizedOverridesSearchPath = normalizeSchemaList(overrides.searchPath) and normalizedOverridesDdlSearchPath = normalizeSchemaList(overrides.ddl?.searchPath)) and then use those cached arrays in the ternary that assigns searchPath (falling back to baseConfig.searchPath) to avoid redundant calls to normalizeSchemaList and improve readability; update references to use these new local consts instead of repeating normalizeSchemaList calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 277-285: The starter currently installs
`@rawsql-ts/testkit-postgres` from npm because LOCAL_SOURCE_STACK_PACKAGE_DIRS
lacks that package and STARTER_DEV_DEPENDENCIES is merged after
buildLocalSourceStackDependencies; fix by adding an entry for
"@rawsql-ts/testkit-postgres": path.join('packages','testkit-postgres') to
LOCAL_SOURCE_STACK_PACKAGE_DIRS so buildLocalSourceStackDependencies will link
the local package (ensure the mapping is present before STARTER_DEV_DEPENDENCIES
is merged).
---
Nitpick comments:
In `@packages/ztd-cli/src/utils/ztdProjectConfig.ts`:
- Around line 262-267: The searchPath computation calls normalizeSchemaList
multiple times; cache the normalized results into local consts (e.g.,
normalizedOverridesSearchPath = normalizeSchemaList(overrides.searchPath) and
normalizedOverridesDdlSearchPath =
normalizeSchemaList(overrides.ddl?.searchPath)) and then use those cached arrays
in the ternary that assigns searchPath (falling back to baseConfig.searchPath)
to avoid redundant calls to normalizeSchemaList and improve readability; update
references to use these new local consts instead of repeating
normalizeSchemaList calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64b708d9-9ad3-47d3-b4da-1e2f413e6e89
📒 Files selected for processing (6)
packages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/utils/ztdProjectConfig.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/README.mdpackages/ztd-cli/templates/tests/support/postgres-testkit.tspackages/ztd-cli/tests/ztdProjectConfig.unit.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ztd-cli/templates/README.md
- packages/ztd-cli/templates/src/features/smoke/tests/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ztd-cli/templates/tests/support/postgres-testkit.ts
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@rawsql-ts/testkit-postgreswith a reusable starter helper.ztd.config.json(ztdRootDir,ddlDir,defaultSchema,searchPath) while keeping environment-specific connection details in env.smoke.queryspec.test.tsreadable by leaving fixtures and rows next to the test, while moving Pool/client setup and teardown into the shared helper.What changed
ztd.config.jsoncarries the starter defaults at the top level, with legacyddlvalues kept for compatibility.tests/support/postgres-testkit.tsas a thin helper that reads starter defaults from config, creates the PG pool, buildscreatePostgresTestkitClient(...), and handles cleanup.smoke.queryspec.test.tsso it only shows the sample table definitions, sample rows, and the DB-backed assertion.@rawsql-ts/testkit-postgres.Outcome
ztd.config.jsoninstead of repeating them in each DB-backed test.Evidence
pnpm --filter @rawsql-ts/ztd-cli buildpnpm --filter @rawsql-ts/ztd-cli test -- postgresTestkitHelper.unit.test.ts furtherReading.docs.test.ts directoryFinding.docs.test.ts sqlFirstTutorial.docs.test.ts init.command.test.tsinit --starter --yesdocker compose up -dnpx vitest run src/features/smoke/tests/smoke.test.ts src/features/smoke/tests/smoke.validation.test.tsnpx ztd ztd-confignpx vitest runnpx vitest run src/features/smoke/tests/smoke.queryspec.test.tsNotes
git commitpre-commit failed on a pre-existing workspace issue:packages/testkit-core/tests/SelectFixtureRewriter.test.tsusesafterEachwithout it being defined in the workspace test harness.--no-verifyafter confirming the targeted build/tests and the dogfooding flow.