Repository navigation
feat(pgtap): replace skeleton with 3 proper pgTAP smoke tests - #421
Merged
Merged
Conversation
Replace the non-standard `ops.pgtap.sql` DO-block wrapper with three proper BEGIN/plan/finish pgTAP test files: - ops-parameterless-view: tests all_products view + zero-arg function - ops-parameterized-fn: tests products_by_name ILIKE filter - ops-nested-lateral: tests orders_with_items_by_user LATERAL join CI changes: - Apply *.view.sql alongside *.fn.sql (views first) - Expanded seed data with deterministic UUIDs (user, order, 2 items) - Run all *.pgtap.sql files instead of single file - EXPLAIN snapshots for all 4 ops - Added all_products.graphql to test fixtures Closes #416
Summary by CodeRabbit
WalkthroughApply ops SQL for views and functions transactionally, add deterministic DB seed, install and run pgTAP across multiple test files via docker-compose/CI, add three focused pgTAP suites and a GraphQL example, update workflow and CLI dependency, and adjust EXPLAIN snapshots to use explicit UUIDs. Changes
Sequence Diagram(s)sequenceDiagram
participant Actions as GitHub Actions
participant Runner as CI Runner / Shell
participant Docker as Docker Compose
participant Postgres as Postgres DB
participant pgtap as pgTAP Runner
Actions->>Runner: start workflow
Runner->>Docker: docker-compose up (postgres + pgtap)
Runner->>Postgres: ensure init scripts (02-wes-ops-schema, 03-seed, 04-tests) applied
pgtap->>Postgres: apply `*.view.sql` files (transactional)
pgtap->>Postgres: apply `*.fn.sql` files (transactional)
pgtap->>Runner: build & install pgTAP, load extension
pgtap->>Postgres: execute `/tests/*.pgtap.sql` (loop)
Postgres-->>pgtap: TAP output
pgtap->>Runner: parse TAP, aggregate failures
Runner->>Actions: exit non-zero if any test failures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
🚢 SHIPME[REALM] PASS — sha 32e030a |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d130cf467
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Fix op name: `op_allproducts` not `op_all_products` (sanitizeIdentBase strips underscores from GraphQL operation name "AllProducts") - Fix products_by_name test: assert no price_cents key (not in projection) - Fix CI EXPLAIN snapshot to use `op_allproducts` name - Add shared seed SQL file (test/fixtures/postgres/03-seed.sql) - Add wes_ops schema init (test/fixtures/postgres/02-wes-ops-schema.sql) - Update docker-compose.yml: pgtap service now applies ops SQL (views then functions), installs pgTAP via pgtap-static.sql, and runs all *.pgtap.sql files with pass/fail summary Verified: all 16 assertions pass locally via Docker compose.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/wesley-holmes.yml:
- Around line 500-509: The workflow currently runs pgTAP files with
PGPASSWORD=postgres psql ... -f "$f" which does not fail the job on TAP
assertion failures; replace that step to invoke pg_prove (or run a TAP parser)
against the same files so TAP failures return non-zero and fail CI.
Specifically, install/run pg_prove and call it with the same connection options
(e.g., --dbname/test, --host/--username) against
test/fixtures/examples/tests/*.pgtap.sql (or call pg_prove per file in the
existing loop) and ensure its exit code is used to fail the job; alternatively
capture psql output and parse TAP output for failed tests and exit non-zero when
any test fails. Ensure you modify the code referencing the files array and the
loop that currently executes PGPASSWORD=postgres psql -v ON_ERROR_STOP=1 -h
localhost -U postgres -d test -f "$f".
In `@CHANGELOG.md`:
- Line 19: There is a duplicate "### Added" heading under the [Unreleased]
section causing markdownlint MD024; locate the second "### Added" header in
CHANGELOG.md and merge its entries into the existing "### Added" subsection
under [Unreleased], then remove the duplicated "### Added" header so only one
"### Added" section remains with all added items combined.
In `@test/fixtures/examples/tests/ops-nested-lateral.pgtap.sql`:
- Around line 16-23: The test only asserts count for
'00000000-0000-0000-0000-000000000001' so it won’t catch a function that ignores
its user_id; add a second SELECT is(...) that calls op_orders_with_items_by_user
with a different UUID (e.g. '00000000-0000-0000-0000-000000000002') and expects
0::bigint, update the test description accordingly, and increment the pgtap plan
count to account for the additional assertion so the plan remains correct.
- Around line 38-46: The test only checks jsonb_array_length on row_data ->
'items' so malformed item objects can pass; update the test to assert each item
has the required keys by iterating jsonb_array_elements(row_data -> 'items') and
using bool_and to ensure every element contains 'id','product_id','quantity'
(e.g. use the ?& operator or explicit existence checks) against the result of
wes_ops.op_orders_with_items_by_user (the same row_data used now); keep the
length check but add a second assertion that jsonb_array_elements(... ) returns
elements where (elem ?& array['id','product_id','quantity']) is true for all
items.
In `@test/fixtures/examples/tests/ops-parameterized-fn.pgtap.sql`:
- Around line 16-28: Add a new test that proves the published=true filter by
querying op_products_by_name with a pattern that would match the seeded
unpublished "Beta" row (e.g., 'Be%') and assert the count is 0, then increment
the TAP plan count to reflect the added test; locate and update the existing
test block that calls op_products_by_name (the SELECT is(... FROM
wes_ops.op_products_by_name(...)) and the test plan declaration to add this
assertion.
In `@test/fixtures/examples/tests/ops-parameterless-view.pgtap.sql`:
- Around line 19-35: Add an assertion that the zero-arg function returns the
same rowset count as the view: compare (SELECT count(*)::bigint FROM
wes_ops.op_all_products) with (SELECT count(*)::bigint FROM
wes_ops.op_all_products()) and update the TAP plan count accordingly; locate the
existing checks around op_all_products and op_all_products() in the test and
insert this parity/is-count assertion so the function cannot silently drop rows
while preserving keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e533340-0c09-45d6-bb07-991bf5bfba37
📒 Files selected for processing (7)
.github/workflows/wesley-holmes.ymlCHANGELOG.mdtest/fixtures/examples/ops/all_products.graphqltest/fixtures/examples/tests/ops-nested-lateral.pgtap.sqltest/fixtures/examples/tests/ops-parameterized-fn.pgtap.sqltest/fixtures/examples/tests/ops-parameterless-view.pgtap.sqltest/fixtures/examples/tests/ops.pgtap.sql
💤 Files with no reviewable changes (1)
- test/fixtures/examples/tests/ops.pgtap.sql
The dynamic `import('graphql')` in generate.mjs for .graphql ops
compilation fails under pnpm strict module resolution. The package
was only available transitively via @wesley/core, which pnpm disallows.
This fixes the CI failure where all_products.graphql could not compile
because graphql was not resolvable from wesley-cli's module scope.
🔍 The Case of Pull Request #421🕵️ SHA-lock HOLMES's Investigation (click to expand)🕵️ SHA-lock HOLMES Investigation
🔍 Executive Deduction"Watson, after careful examination of the evidence, I deduce..." Weighted Completion: ██████░░░░ 60.0% 🧩 SCS Breakdown
🧪 TCI Breakdown
|
| Component | Risk Share | Points | Count |
|---|---|---|---|
| Drops | 100.0% | 20 | 0 |
| Renames Without Uid | 0.0% | 0 | 0 |
| Add Not Null Without Default | 0.0% | 0 | 0 |
| Non Concurrent Indexes | 0.0% | 0 | 0 |
📊 The Weight of Evidence
"Observe, Watson, how not all features carry equal importance..."
| Element | Weight | Status | Evidence | Deduction |
|---|---|---|---|---|
| schema | 5 | ✅ SQL & tests | out/schema.sql:1-9999@32e030a | Elementary! |
🚪 Security & Performance Gates
"Elementary security measures, Watson..."
| Gate | Status | Evidence | Holmes's Ruling |
|---|---|---|---|
| Migration Risk | ✅ | MRI: 20.0% | "Acceptable risk" |
| Test Coverage | TCI: 70.0% | "Insufficient coverage" | |
| Sensitive Fields | ✅ | 0 fields | "All secured" |
📋 The Verdict
"Some clues remain unclear. Address the noted issues."
Signed and sealed,
- S. Holmes, Consulting Detective
[END OF INVESTIGATION FOR COMMIT 32e030a]
🩺 Dr. WATSON's Verification (click to expand)
🩺 Dr. Watson's Independent Verification Report
Medical Examination of Evidence
- Examination Date: 2026-03-11T01:23:49.613Z
- Patient SHA: 32e030a
🔬 Citation Verification
"Let me examine each piece of evidence independently..."
- Citations Examined: 2
- Verified: 0 ✅
- Failed: 0 ❌
- Unable to Verify: 2
Verification Rate: 0.0%
📊 Mathematical Verification
"I shall recalculate Holmes's arithmetic..."
Holmes claimed SCS: 60.0%
Watson calculates: 100.0%
Difference:
🔍 Consistency Analysis
"Checking for contradictions in Holmes's deductions..."
✅ No logical inconsistencies detected
🩺 Dr. Watson's Medical Opinion
VERIFICATION: CONCERNS NOTED
"While Holmes's methods are generally sound, I have noted some"
"discrepancies that warrant further investigation."
Respectfully submitted,
- Dr. J. Watson, M.D.
Medical Examiner & Verification Specialist
🔮 Professor MORIARTY's Predictions (click to expand)
🧠 Professor Moriarty's Temporal Predictions
The Mathematics of Inevitability
- Analysis Date: 2026-03-11T01:24:13.436Z
🔮 Current State
SCS: ██████░░░░ 60.0%
TCI: ███████░░░ 70.0%
MRI: 20.0% risk
📈 Velocity Analysis
SCS Velocity: +0.00%/day
Git Activity (window): 24h · commits 19 (7 relevant) · ~19.00 commits/day
↳ Magnitude: ~339 relevant LOC/day across ~9.0 files/day
Activity Index: 38 / 100 (PR 0, Window 95)
Blended Velocity: +0.23%/day
Commit Size Burstiness: 63 / 100 (higher = more uneven commit sizes)
ℹ️ Low SCS movement, but recent Git activity suggests ongoing work. Plateau not flagged.
⏰ Completion Predictions
ETA: Cannot predict (insufficient velocity)
"At current velocity, completion is... improbable."
🧪 Readiness EXPLAIN
- SCS ≥ 80% → FAIL ❌ (actual 60.0%)
- TCI ≥ 70% → PASS ✅ (actual 70.0%)
- MRI ≤ 40% → PASS ✅ (actual 20.0%)
- CI Stability ≥ 90% (branch main) → PASS ✅ (actual 95% over ~168h)
- Delivery context (last 168h): 1 issues closed · 22 PRs merged (informational, not gating)
Signals blend: SCS velocity (70%) + Git activity (30%, branch-first). Activity only suppresses false plateaus; it never inflates readiness.
📊 Historical Trajectory
03-11: ██████░░░░ 60.0%
03-11: ██████░░░░ 60.0%
03-11: ██████░░░░ 60.0%
"Every problem becomes elementary when reduced to mathematics"
— Professor Moriarty
Machine-readable reports: holmes-report.json · watson-report.json · moriarty-report.json (see workflow artifacts).
Filed at 221B Repository Street
- Re-add tests.sql mount as 04-tests.sql in docker-compose (was dropped during wes_ops/seed mount additions) - Remove duplicate ### Added heading in CHANGELOG.md - Remove unused docker.sock mount from pgtap service - Add cross-reference comments between 03-seed.sql and CI workflow to prevent seed data drift Closes #416
psql -f exits zero even when pgTAP assertions fail — it only raises non-zero on SQL errors. Both the CI workflow and docker-compose pgtap runner now capture output and grep for "not ok" TAP lines, exiting non-zero when any assertion fails.
- ops-parameterized-fn: add 'Be%' test proving published=true filter excludes unpublished Beta product - ops-nested-lateral: add negative UUID case (0 rows for non-existent user) and item key shape assertion (id, product_id, quantity) - ops-parameterless-view: add view/function row count parity assertion
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/wesley-holmes.yml (1)
726-734:⚠️ Potential issue | 🔴 CriticalDelete the orphaned "Ensure history for MORIARTY" step — the
comment-reportjob has no repository checkout.The
comment-reportjob contains only artifact downloads and GitHub Actions; there is noactions/checkoutstep. The git commands at lines 726-734 will execute without a.gitdirectory, causing immediate failure. This is a copy-paste artifact from themoriarty-predictjob (lines 182-189) where checkout does exist.🗑️ Remove the orphaned step
- - name: "🔄 Ensure history for MORIARTY" - shell: bash - run: | - set -euo pipefail - # In case fetch-depth wasn't honored or repo is shallow, try to unshallow - if git rev-parse --is-shallow-repository >/dev/null 2>&1; then - git fetch --prune --unshallow --tags || true - fi - git fetch --prune origin '+refs/heads/*:refs/remotes/origin/*' || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/wesley-holmes.yml around lines 726 - 734, Remove the orphaned CI step named "🔄 Ensure history for MORIARTY" from the comment-report job: delete the entire step that runs the git commands (the bash block with set -euo pipefail and the git fetch/unshallow/fetch refs commands) since comment-report has no actions/checkout and this will fail; simply remove that step from the job to match its artifact-only workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 53-64: The container currently builds and installs pgTAP from
source on every run (the wget/tar/make/DESTDIR install sequence and psql -f
sql/pgtap-static.sql), which slows local iteration; fix this by pre-baking pgTAP
into a custom image (create a Dockerfile that runs apk add build-base perl,
downloads pgtap v1.3.0, make && make install, and copies the extension so the
server has it) or switch docker-compose to use an existing Postgres image with
pgTAP pre-installed (e.g., docker.io/supabase/postgres) and remove the runtime
build steps (the wget/tar/make/DESTDIR and psql -f sql/pgtap-static.sql
commands) from the compose service.
In `@test/fixtures/examples/tests/ops-nested-lateral.pgtap.sql`:
- Around line 47-56: The test currently inspects only the first row because of
LIMIT 1; update the assertion to validate the items array across all returned
orders from the function wes_ops.op_orders_with_items_by_user so it doesn't
silently ignore additional orders. Replace the LIMIT 1 pattern in the SELECT
that calls jsonb_array_length(row_data -> 'items') with an aggregate-style check
over all rows (for example using bool_and(jsonb_array_length(row_data ->
'items') = 2) or ensuring jsonb_array_length(...) > 0 for every row) so the test
asserts the expected shape for every order returned by
wes_ops.op_orders_with_items_by_user.
---
Outside diff comments:
In @.github/workflows/wesley-holmes.yml:
- Around line 726-734: Remove the orphaned CI step named "🔄 Ensure history for
MORIARTY" from the comment-report job: delete the entire step that runs the git
commands (the bash block with set -euo pipefail and the git
fetch/unshallow/fetch refs commands) since comment-report has no
actions/checkout and this will fail; simply remove that step from the job to
match its artifact-only workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7622368d-63b8-4a82-9670-7b12bfbe283d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/wesley-holmes.ymlCHANGELOG.mddocker-compose.ymlpackages/wesley-cli/package.jsontest/fixtures/examples/tests/ops-nested-lateral.pgtap.sqltest/fixtures/examples/tests/ops-parameterized-fn.pgtap.sqltest/fixtures/examples/tests/ops-parameterless-view.pgtap.sqltest/fixtures/postgres/02-wes-ops-schema.sqltest/fixtures/postgres/03-seed.sql
Replace the 40-line inline SQL heredoc in the CI workflow with a single psql -1 -f pointing at test/fixtures/postgres/03-seed.sql. Single source of truth for seed data used by both docker-compose and CI.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/fixtures/postgres/03-seed.sql`:
- Around line 7-11: The INSERTs use ON CONFLICT DO NOTHING without an explicit
conflict target which is ambiguous; update each INSERT into product, "user",
"order", and orderitem to specify the exact conflict target (either the
unique/index column(s) such as the primary key column like id or the specific
constraint name via ON CONSTRAINT constraint_name) so the intent is explicit and
future-proof; locate the INSERT INTO product (...) VALUES (...) ON CONFLICT DO
NOTHING and change to ON CONFLICT (id) DO NOTHING or ON CONFLICT ON CONSTRAINT
<constraint_name> DO NOTHING, and apply the same explicit conflict target
approach for the INSERTs into "user", "order", and orderitem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 809728a2-3d65-45a0-8d73-2f26f432591c
📒 Files selected for processing (3)
.github/workflows/wesley-holmes.ymlCHANGELOG.mdtest/fixtures/postgres/03-seed.sql
- ops-nested-lateral: replace LIMIT 1 with bool_and aggregate so items count assertion covers all returned orders, not just the first - 03-seed.sql: specify explicit conflict target (id) on all INSERT ON CONFLICT clauses for unambiguous intent
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/fixtures/postgres/03-seed.sql`:
- Around line 21-27: The seeded order row in the INSERT INTO "order" statement
has total_cents set to 300 but its corresponding orderitem rows sum to 400
(2×100 + 1×200); update the total_cents value in the "order" INSERT for the
order with id '00000000-0000-0000-0000-000000000201' to 400 so the fixture is
internally consistent with the orderitem totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcd5c6ad-b037-480c-a5f0-13fb71ae2919
📒 Files selected for processing (2)
test/fixtures/examples/tests/ops-nested-lateral.pgtap.sqltest/fixtures/postgres/03-seed.sql
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
Closes #416
ops.pgtap.sqlDO-block wrapper with 3 proper pgTAP test files: parameterless view, parameterized function (ILIKE), and nested LATERAL join*.view.sqlalongside*.fn.sql(views first)*.pgtap.sqlfiles instead of a single hardcoded pathall_products.graphqlto test fixtures for self-contained coverageTest plan
test-pgtapCI job goes green (all 16 assertions across 3 files)