CI: gate the test job on make verify-results, not make test

[jnasbyupgrade]

pgxntool/base.mk marks installcheck .IGNORE so a pg_regress regression never makes make test exit non-zero -- it only prints the diff. The test job's "Test on PostgreSQL" step ran make test, so CI reported green even when every regression test failed. make verify-results runs the same TEST_DEPS chain but then actually inspects the pgTAP results and exits non-zero on failure; PGXNTOOL_ENABLE_VERIFY_RESULTS defaults to yes and PGXNTOOL_VERIFY_RESULTS_MODE to pgtap already in the vendored pgxntool 2.2.0, matching this suite's pgTAP-based tests, so no Makefile change is needed. This mirrors the pattern cat_tools and pg_count_nulls already use for the same reason (both call make verify-results/make verify-results TEST_LOAD_SOURCE=update in their own CI, with cat_tools documenting it inline as "the real gate").

This PR will make CI report red on master. That's correct, not a regression from this change: every regression test is currently failing (cat_tools.function__arg_types_text() was deprecated in cat_tools 0.3.0, and the resulting WARNING pollutes every test's expected-output diff) but CI has been silently reporting green the whole time because of the make test gap described above. #28 (already open) fixes the underlying cat_tools deprecation and will turn this green again once merged; this PR only fixes CI's ability to detect real failures, it does not touch the failing test itself.

Confirmed locally: make test exits 0 while printing "7 of 7 tests failed"; make verify-results exits non-zero and reports ERROR: pgtap failure detected in test/results/all.out / not ok 1 - All object types are being tested. / plan mismatches.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f84b83ed-62d0-46ea-a047-e22c94a041df

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 57s —— View job

---

Code Review: CI: gate the test job on make verify-results, not make test

• Gather context (PR diff, description, repo CLAUDE.md)
• Understand the request (code review of CI workflow change)
• Review the diff in .github/workflows/ci.yml
• Verify claims in PR description against vendored pgxntool/base.mk
• Post review feedback

Summary

The change itself is correct and matches the pattern used in cat_tools/pg_count_nulls: verify-results runs the same TEST_DEPS chain as test (confirmed in pgxntool/base.mk), then additionally scans test/results/*.out for pgTAP not ok lines and plan mismatches via verify-results-pgtap.sh, exiting non-zero on failure — a strictly more thorough check than a plain diff test. PGXNTOOL_ENABLE_VERIFY_RESULTS (default yes) and PGXNTOOL_VERIFY_RESULTS_MODE (default pgtap) confirmed in the vendored base.mk, so no Makefile change is needed, as stated.

One thing worth fixing: the new comment is now stale

The comment added at .github/workflows/ci.yml#L115-L121 says:

make test alone never fails this step ... so a regression exits 0 and only prints the diff.

That was true when commit 565a11c was written (12:37), against the base.mk from #9. But 4cd9280 (the merge of master into this branch, 15:02) pulled in f2260f2 (#32, 15:01), which rewrote base.mk's test: recipe to:

test: $(TEST_DEPS)
	@if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; exit 1; fi

So, as currently vendored in this branch, make test does now exit non-zero when regression.diffs exists — the comment's central claim is no longer accurate for the code sitting right next to it.

This doesn't undermine the PR's conclusion — switching to verify-results is still the right call, because it catches a case plain diff-checking (both the old and the new test: recipe) can't: pgTAP not ok/plan-mismatch results where the expected file happens to already match the failing actual output byte-for-byte (e.g. a stale/incorrectly-regenerated expected file), so no regression.diffs is ever produced even though the test suite is genuinely failing. verify-results-pgtap.sh inspects the actual TAP output for that, independent of whether a diff exists.

Suggest updating the comment to lead with that reasoning (verify-results checks pgTAP semantics that a diff check — old or new — can miss) rather than the now-outdated "make test exits 0" framing, so a future reader diffing this comment against base.mk doesn't get confused.

Fix this →

No other issues found — the workflow diff is minimal, functionally sound, and doesn't touch anything else in the file.