diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md deleted file mode 100644 index 0091d8b..0000000 --- a/.claude/commands/commit.md +++ /dev/null @@ -1,229 +0,0 @@ ---- -description: Create a git commit following project standards and safety protocols -allowed-tools: Bash(git status:*), Bash(git log:*), Bash(git add:*), Bash(git diff:*), Bash(git commit:*), Bash(make test:*) ---- - -# commit - -Create a git commit following all project standards and safety protocols for pgxntool-test. - -**CRITICAL REQUIREMENTS:** - -1. **Git Safety**: Never update `git config`, never force push to `main`/`master`, never skip hooks unless explicitly requested - -2. **Commit Attribution**: Do NOT add "Generated with Claude Code" to commit message body. The standard Co-Authored-By trailer is acceptable per project CLAUDE.md. - -3. **Multi-Repo Commits**: If BOTH repositories have changes, commit BOTH (unless user explicitly says otherwise). Do NOT create empty commits - only commit repos with actual changes. - -4. **Testing**: ALL tests must pass before committing: - - Tests are run via test subagent (first step in workflow) - - Check subagent output carefully for any "not ok" lines - - **If ANY tests fail: STOP. Do NOT commit. Ask the user what to do.** - - There is NO such thing as an "acceptable" failing test - - Do NOT rationalize failures as "pre-existing" or "unrelated" - -**WORKFLOW:** - -1. **Launch test subagent** (unless user explicitly says not to): - - Use Task tool to launch test subagent to run `make test` - - Run in background so we can continue with analysis - - Tests will be checked before committing - -2. **While tests run**, gather information in parallel: - - `git status`, `git diff --stat`, `git log -10 --oneline` for both repos - -3. **Analyze changes** in BOTH repositories and draft commit messages for BOTH: - - **CRITICAL: Item Ordering** - - Order all items (changes, bullet points) by **decreasing importance** - - Importance = impact of the change × likelihood someone reading history will care - - Most impactful/interesting changes first, minor details last - - Think: "What would I want to see first when reading this in git log 2 years from now?" - - For pgxntool: - - Analyze: `git status`, `git diff --stat`, `git log -10 --oneline` - - Draft message with structure: - ``` - Subject line - - [Main changes in pgxntool, ordered by decreasing importance...] - - Related changes in pgxntool-test: - - [RELEVANT test change 1] - - [RELEVANT test change 2] - - Co-Authored-By: Claude - ``` - - Only mention RELEVANT test changes (1-3 bullets): - * ✅ Include: Tests for new features, template updates, user docs - * ❌ Exclude: Test refactoring, infrastructure, internal changes - - Wrap code references in backticks (e.g., `helpers.bash`, `make test`) - - No hash placeholder needed - pgxntool doesn't reference test hash - - For pgxntool-test: - - Analyze: `git status`, `git diff --stat`, `git log -10 --oneline` - - Draft message with structure: - ``` - Subject line - - Add tests/updates for pgxntool commit [PGXNTOOL_COMMIT_HASH] (brief description): - - [Key pgxntool change 1] - - [Key pgxntool change 2] - - [pgxntool-test specific changes, ordered by decreasing importance...] - - Co-Authored-By: Claude - ``` - - Use placeholder `[PGXNTOOL_COMMIT_HASH]` - - Include brief summary (2-3 bullets) of pgxntool changes near top - - Wrap code references in backticks - - **If only one repo has changes:** - Skip the repo with no changes. In the commit message for the repo that has changes, - add: "Changes only in [repo]. No related changes in [other repo]." before Co-Authored-By. - -4. **PRESENT both proposed commit messages to the user and WAIT for approval** - - Show both messages: - ``` - ## Proposed Commit for pgxntool: - [message] - - ## Proposed Commit for pgxntool-test: - [message with [PGXNTOOL_COMMIT_HASH] placeholder] - ``` - - **Note:** Mention any files that are intentionally not being committed and why. - - **Note:** If only one repo has changes, show only that message (with note about other repo). - -5. **Wait for tests to complete** - THIS IS MANDATORY: - - Check test subagent output for completion - - Verify ALL tests passed (no "not ok" lines) - - **If ANY tests fail: STOP. Do NOT commit. Ask the user what to do.** - - There is NO such thing as an "acceptable" failing test - - Do NOT rationalize failures as "pre-existing" or "unrelated" - - Only proceed to commit if ALL tests pass - -6. **After tests pass AND receiving approval, execute two-phase commit:** - - **Phase 1: Commit pgxntool** - - a. `cd ../pgxntool` - - b. Stage changes: `git add` (include ALL new files per guidelines below) - - Check `git status` for untracked files - - ALL untracked files that are part of the feature/change MUST be staged - - New scripts, new documentation, new helper files, etc. should all be included - - Do NOT leave new files uncommitted unless explicitly told to exclude them - - c. Verify staged files: `git status` - - Confirm ALL modified AND untracked files are staged - - STOP and ask user if staging doesn't match intent - - d. Commit using approved message: - ```bash - git commit -m "$(cat <<'EOF' - [approved pgxntool message] - EOF - )" - ``` - - e. Capture hash: `PGXNTOOL_HASH=$(git log -1 --format=%h)` - - f. Verify: `git status` - - g. Handle pre-commit hooks if needed: - - Check if hooks modified files - - Check authorship: `git log -1 --format='%an %ae'` - - Check branch status - - Amend if safe or create new commit - - **Phase 2: Commit pgxntool-test** - - a. `cd ../pgxntool-test` - - b. Replace `[PGXNTOOL_COMMIT_HASH]` in approved message with `$PGXNTOOL_HASH` - - Keep everything else EXACTLY the same - - c. Stage changes: `git add` (include ALL new files) - - d. Verify staged files: `git status` - - e. Commit using hash-injected message: - ```bash - git commit -m "$(cat <<'EOF' - [approved message with actual pgxntool hash] - EOF - )" - ``` - - f. Capture hash: `TEST_HASH=$(git log -1 --format=%h)` - - g. Verify: `git status` - - h. Handle pre-commit hooks if needed - - **Note:** If only one repo has changes, skip the phase for the other repo. - -**MULTI-REPO COMMIT CONTEXT:** - -The two repositories: -- **pgxntool** - The main framework -- **pgxntool-test** - Test harness (includes template files in `template/` directory) - -When committing changes that span repositories: - -1. **pgxntool-test commit MUST reference pgxntool commit hash** - - pgxntool commit format: - ``` - Subject line - - [Main changes...] - - Related changes in pgxntool-test: - - [RELEVANT test change] - - [Keep to 1-3 bullets] - - Co-Authored-By: Claude - ``` - - pgxntool-test commit format: - ``` - Subject line - - Add tests for pgxntool commit def5678 (brief description): - - [Key pgxntool change 1] - - [Key pgxntool change 2] - - [pgxntool-test specific changes...] - - Co-Authored-By: Claude - ``` - -2. **Relevance filter for pgxntool message:** - - ✅ Include: Tests for new features, template updates, user documentation - - ❌ Exclude: Test refactoring, infrastructure changes, internal improvements - - Keep it brief (1-3 bullets max) - -3. **Commit workflow:** - - Commit pgxntool first (no placeholder) - - Capture pgxntool hash - - Commit pgxntool-test (inject pgxntool hash) - - Result: pgxntool-test references pgxntool commit - -4. **Single-repo case:** - Add line: "Changes only in [repo]. No related changes in [other repo]." - -**REPOSITORY CONTEXT:** - -This is pgxntool, a PostgreSQL extension build framework. Key facts: -- Main Makefile is `base.mk` -- Scripts live in root directory -- Documentation is in `README.asc` (generates `README.html`) - -**RESTRICTIONS:** -- DO NOT push unless explicitly asked -- DO NOT commit files with actual secrets (`.env`, `credentials.json`, etc.) -- Never use `-i` flags (`git commit -i`, `git rebase -i`, etc.) diff --git a/.claude/settings.json b/.claude/settings.json index 134b554..fd12d9a 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -13,7 +13,27 @@ "Bash(DEBUG=3 test/bats/bin/bats:*)", "Bash(DEBUG=4 test/bats/bin/bats:*)", "Bash(DEBUG=5 test/bats/bin/bats:*)", - "Edit" + "Bash(bash .claude/skills/commit/scripts/*)", + "Bash(find:*)", + "Bash(gh pr list:*)", + "Bash(gh pr view:*)", + "Bash(gh repo view:*)", + "Bash(git checkout:*)", + "Bash(git fetch:*)", + "Bash(git ls-tree:*)", + "Bash(git merge:*)", + "Bash(git rebase:*)", + "Bash(git rev-parse:*)", + "Bash(git worktree:*)", + "Bash(grep:*)", + "Bash(ls:*)", + "Bash(make html:*)", + "Bash(make test-extra:*)", + "Bash(make test:*)", + "Bash(readlink:*)", + "Bash(tee:*)", + "Edit", + "Skill(building-claude-code-skills)" ], "additionalDirectories": [ "/tmp/", diff --git a/.claude/skills/commit/SKILL.md b/.claude/skills/commit/SKILL.md new file mode 100644 index 0000000..1e6a3ea --- /dev/null +++ b/.claude/skills/commit/SKILL.md @@ -0,0 +1,119 @@ +--- +name: commit +description: | + Create git commits across pgxntool and pgxntool-test repositories following project + standards. Preprocesses repo info, runs tests, drafts cross-referenced commit messages, + and executes two-phase commits with hash injection. + + Use when: "commit", "create commit", "commit changes", "/commit" +allowed-tools: Bash(git status:*), Bash(git log:*), Bash(git add:*), Bash(git diff:*), Bash(git commit:*), Bash(git branch:*), Bash(bash .claude/skills/commit/scripts/*), Bash(make test:*), Read, Edit, Task +--- + +# Commit Skill + +Create git commits following project standards and safety protocols for pgxntool-test. + +## Critical Requirements + +| Rule | Details | +|------|---------| +| **Git Safety** | Never update git config. Never force push. Never skip hooks unless requested. | +| **Attribution** | No "Generated with Claude Code" in body. Co-Authored-By trailer is OK. | +| **Multi-Repo** | Commit BOTH repos if both have changes (unless told otherwise). No empty commits. | +| **Testing** | ALL tests must pass. ANY failure = STOP and ask user. No rationalizing failures. | +| **HISTORY.asc** | Update for significant user-visible pgxntool changes. Propose entry, get confirmation. | + +## Workflow + +### 1. Launch Tests (Background) + +Use Task tool to launch test subagent to run `make test` in background. +Skip only if user explicitly says to. + +### 2. Gather Repository Info + +Run the preprocessing script to collect all git data in one call: + +```bash +bash .claude/skills/commit/scripts/gather-repo-info.sh ../pgxntool . +``` + +Review output. Identify which repos have changes. + +### 3. Check HISTORY.asc (pgxntool changes only) + +Read `../pgxntool/HISTORY.asc`. Determine if changes are significant and user-visible: +- Yes: New features, behavior changes, bug fixes users would notice +- No: Internal refactoring, test changes, cleanup, documentation fixes + +If update needed: +1. Propose entry in AsciiDoc format (use `==` for heading) +2. Ask for confirmation BEFORE proceeding +3. Add to STABLE section at TOP of file (create section if missing): + ``` + STABLE + ------ + == [Entry heading] + [Entry description] + + [existing content...] + ``` + +### 4. Draft Commit Messages + +Read the format guide for detailed templates and rules: +**`.claude/skills/commit/guides/commit-message-format.md`** + +Key principles: +- Order items by **decreasing importance** (impact x likelihood someone cares) +- pgxntool message includes relevant test changes (1-3 bullets) +- pgxntool-test message uses `[PGXNTOOL_COMMIT_HASH]` placeholder +- Single-repo: note "Changes only in [repo]. No related changes in [other repo]." +- Wrap code references in backticks + +### 5. Present for Approval + +Show proposed commit messages. Wait for user approval. +Mention any files intentionally excluded and why. +If only one repo has changes, show only that message (with note about other repo). + +### 6. Verify Tests Passed (MANDATORY) + +Check test subagent output for completion. +- Look for ANY "not ok" lines +- **If tests fail: STOP. Do NOT commit. Ask user what to do.** +- There is NO such thing as an "acceptable" failing test +- Do NOT rationalize failures as "pre-existing" or "unrelated" +- Only proceed if ALL tests pass + +### 7. Execute Two-Phase Commit + +After tests pass AND receiving user approval: + +1. **Phase 1: Commit pgxntool** (if it has changes) + - Stage files: `git add` specific files (include ALL new files for the feature) + - Verify staging: `git status` (STOP if mismatch) + - Commit with approved message using HEREDOC + - Capture hash: `PGXNTOOL_HASH=$(git log -1 --format=%h)` + - Verify: `git status` + +2. **Phase 2: Commit pgxntool-test** (if it has changes) + - Replace `[PGXNTOOL_COMMIT_HASH]` with captured hash + - Stage, verify, commit, verify (same pattern) + +For detailed execution steps including pre-commit hook handling, read: +**`.claude/skills/commit/guides/commit-message-format.md`** -> "Two-Phase Commit Execution" + +Always use HEREDOC format: +```bash +git commit -m "$(cat <<'EOF' +[message] +EOF +)" +``` + +## Restrictions + +- DO NOT push unless explicitly asked +- DO NOT commit files with secrets (.env, credentials.json) +- Never use `-i` flags (git commit -i, git rebase -i) diff --git a/.claude/skills/commit/guides/commit-message-format.md b/.claude/skills/commit/guides/commit-message-format.md new file mode 100644 index 0000000..ee8b8cd --- /dev/null +++ b/.claude/skills/commit/guides/commit-message-format.md @@ -0,0 +1,123 @@ +# Commit Message Format Guide + +## Item Ordering (CRITICAL) + +Order all items (changes, bullet points) by **decreasing importance**: +- Importance = impact of the change x likelihood someone reading history will care +- Most impactful/interesting changes first, minor details last +- Think: "What would I want to see first when reading this in git log 2 years from now?" + +## pgxntool Commit Format + +``` +Subject line + +[Main changes in pgxntool, ordered by decreasing importance...] + +Related changes in pgxntool-test: +- [RELEVANT test change 1] +- [RELEVANT test change 2] + +Co-Authored-By: Claude +``` + +**Relevance filter for test changes:** + +| Include | Exclude | +|---------|---------| +| Tests for new features | Test refactoring | +| Template updates | Infrastructure changes | +| User documentation | Internal improvements | + +Keep to 1-3 bullets max. Wrap code references in backticks (e.g., `helpers.bash`, `make test`). +No hash placeholder needed - pgxntool doesn't reference test hash. + +## pgxntool-test Commit Format + +``` +Subject line + +Add tests/updates for pgxntool commit [PGXNTOOL_COMMIT_HASH] (brief description): +- [Key pgxntool change 1] +- [Key pgxntool change 2] + +[pgxntool-test specific changes, ordered by decreasing importance...] + +Co-Authored-By: Claude +``` + +- Use placeholder `[PGXNTOOL_COMMIT_HASH]` (replaced with actual hash during execution) +- Include brief summary (2-3 bullets) of pgxntool changes near top +- Wrap code references in backticks + +## Single-Repo Case + +If only one repo has changes: +- Skip the repo with no changes +- In the commit message for the repo that has changes, add before Co-Authored-By: + ``` + Changes only in [repo]. No related changes in [other repo]. + ``` + +--- + +## Two-Phase Commit Execution + +### Phase 1: Commit pgxntool + +1. `cd ../pgxntool` +2. Stage changes: `git add` specific files + - Check `git status` for untracked files + - ALL untracked files that are part of the feature/change MUST be staged + - New scripts, documentation, helper files should all be included + - Do NOT leave new files uncommitted unless explicitly told to exclude them +3. Verify staged files: `git status` + - Confirm ALL modified AND untracked files are staged + - STOP and ask user if staging doesn't match intent +4. Commit: + ```bash + git commit -m "$(cat <<'EOF' + [approved pgxntool message] + EOF + )" + ``` +5. Capture hash: `PGXNTOOL_HASH=$(git log -1 --format=%h)` +6. Verify: `git status` +7. Handle pre-commit hooks if needed: + - Check if hooks modified files + - Check authorship: `git log -1 --format='%an %ae'` + - Check branch status + - Amend if safe or create new commit + +### Phase 2: Commit pgxntool-test + +1. Return to pgxntool-test directory +2. Replace `[PGXNTOOL_COMMIT_HASH]` in approved message with `$PGXNTOOL_HASH` + - Keep everything else EXACTLY the same +3. Stage changes: `git add` specific files (include ALL new files) +4. Verify staged files: `git status` +5. Commit: + ```bash + git commit -m "$(cat <<'EOF' + [approved message with actual pgxntool hash] + EOF + )" + ``` +6. Capture hash: `TEST_HASH=$(git log -1 --format=%h)` +7. Verify: `git status` +8. Handle pre-commit hooks if needed + +### Important Notes + +- If only one repo has changes, skip the phase for the other repo +- Always use HEREDOC format for commit messages +- Never use `-i` flags (git commit -i, git rebase -i) + +## Repository Context + +The two repositories: +- **pgxntool** (`../pgxntool/`) - The framework being tested. Main Makefile is `base.mk`, scripts in root, docs in `README.asc`. +- **pgxntool-test** (this repo) - Test harness with template files in `template/` directory. + +Commit pgxntool first (no placeholder), capture hash, then commit pgxntool-test (inject hash). +Result: pgxntool-test commit references the pgxntool commit it corresponds to. diff --git a/.claude/skills/commit/scripts/gather-repo-info.sh b/.claude/skills/commit/scripts/gather-repo-info.sh new file mode 100755 index 0000000..487021c --- /dev/null +++ b/.claude/skills/commit/scripts/gather-repo-info.sh @@ -0,0 +1,75 @@ +#!/bin/bash +set -e + +# gather-repo-info.sh - Collect git info for commit preparation +# +# Gathers branch, status, diff stats, and recent log for each repo in one +# call, reducing multiple git commands to a single preprocessed summary. +# +# Usage: gather-repo-info.sh [ ...] +# Output: Structured text summary per repository + +gather_info() { + local repo_path="$1" + local repo_name + repo_name=$(basename "$(cd "$repo_path" && pwd)") + + echo "## $repo_name" + echo "Path: $(cd "$repo_path" && pwd)" + + ( + cd "$repo_path" || exit 1 + + echo "Branch: $(git branch --show-current 2>/dev/null || echo 'detached')" + echo + + local status + status=$(git status -s 2>/dev/null) + if [ -z "$status" ]; then + echo "Status: CLEAN (no changes)" + echo + return + fi + + echo "### Changes" + echo "$status" + echo + + local diff_stat + diff_stat=$(git diff --stat 2>/dev/null) + if [ -n "$diff_stat" ]; then + echo "### Unstaged Diff" + echo "$diff_stat" + echo + fi + + local cached_stat + cached_stat=$(git diff --cached --stat 2>/dev/null) + if [ -n "$cached_stat" ]; then + echo "### Staged Diff" + echo "$cached_stat" + echo + fi + + echo "### Recent Commits" + git log -10 --oneline 2>/dev/null + echo + ) +} + +if [ $# -eq 0 ]; then + echo "Usage: gather-repo-info.sh [ ...]" >&2 + exit 1 +fi + +for repo in "$@"; do + if ! git -C "$repo" rev-parse --git-dir &>/dev/null; then + echo "## $(basename "$repo")" + echo "WARNING: Not a git repository: $repo" + echo + continue + fi + gather_info "$repo" + echo "---" + echo +done diff --git a/CLAUDE.md b/CLAUDE.md index f672342..354b50d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,7 +14,8 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Subagents are automatically discovered and loaded at session start from: - `.claude/agents/*.md` - Specialized domain experts (invoked via Task tool) -- `.claude/commands/*.md` - Command/skill handlers (invoked via Skill tool) +- `.claude/skills/*/SKILL.md` - Skills with preprocessing scripts and guides (invoked via Skill tool) +- `.claude/commands/*.md` - Simple command handlers (invoked via Skill tool) These subagents are already available in your context - you don't need to discover them. Just USE them whenever their expertise is relevant. @@ -26,9 +27,10 @@ These subagents are already available in your context - you don't need to discov **At the start of every session**: Invoke the pgtle subagent to check if there are any newer versions of pg_tle than what it has already analyzed. If new versions exist, the subagent should analyze them for API changes and update its knowledge of version boundaries. -## Claude Commands +## Claude Skills and Commands -The `/commit` Claude Command lives in this repository (`.claude/commands/commit.md`). pgxntool no longer has its own copy. +The `/commit` skill lives in `.claude/skills/commit/` with a preprocessing script and format guide. +Other commands (worktree, pr, pgxntool-update) remain in `.claude/commands/`. ## What This Repo Is @@ -103,7 +105,7 @@ pgxntool-test/ ├── template/ # Template extension files for test repos ├── tests/ # Test suite (see test subagent for details) ├── test/bats/ # BATS framework (git submodule) -├── .claude/ # Claude subagents and commands +├── .claude/ # Claude subagents, skills, and commands └── .envs/ # Test environments (gitignored) ``` diff --git a/Makefile b/Makefile index 2ecbab7..cb45b71 100644 --- a/Makefile +++ b/Makefile @@ -110,7 +110,8 @@ clean: clean-envs # Note: This works on the pgxntool source repository, not test environments ASCIIDOC_CMD := $(shell which asciidoctor 2>/dev/null || which asciidoc 2>/dev/null) PGXNTOOL_SOURCE_DIR := $(shell cd $(CURDIR)/../pgxntool && pwd) -.PHONY: readme +.PHONY: html readme +html: readme readme: ifndef ASCIIDOC_CMD $(error Could not find "asciidoc" or "asciidoctor". Add one of them to your PATH) diff --git a/template/test/build/.gitignore b/template/test/build/.gitignore new file mode 100644 index 0000000..1fc85ea --- /dev/null +++ b/template/test/build/.gitignore @@ -0,0 +1,5 @@ +# pg_regress generates sql/ from test/build/*.sql (cleaned by make clean) +sql/ +# pg_regress artifacts +*.out.diff +regression.diffs diff --git a/template/test/build/expected/simple_build_test.out b/template/test/build/expected/simple_build_test.out new file mode 100644 index 0000000..3641577 --- /dev/null +++ b/template/test/build/expected/simple_build_test.out @@ -0,0 +1,7 @@ +-- Verify extension function is available after install +SELECT "pgxntool-test"(1, 2); + pgxntool-test +--------------- + 3 +(1 row) + diff --git a/template/test/build/simple_build_test.sql b/template/test/build/simple_build_test.sql new file mode 100644 index 0000000..3bd2183 --- /dev/null +++ b/template/test/build/simple_build_test.sql @@ -0,0 +1,2 @@ +-- Verify extension function is available after install +SELECT "pgxntool-test"(1, 2); diff --git a/template/test/deps.sql b/template/test/deps.sql new file mode 100644 index 0000000..9bdc6ff --- /dev/null +++ b/template/test/deps.sql @@ -0,0 +1,15 @@ +-- IF NOT EXISTS will emit NOTICEs, which is annoying +SET client_min_messages = WARNING; + +-- Add any test dependency statements here +-- Note: pgTap is loaded by setup.sql +--CREATE EXTENSION IF NOT EXISTS ...; +/* + * Now load our extension. We don't use IF NOT EXISTS here because we want an + * error if the extension is already loaded (because we want to ensure we're + * getting the very latest version). + */ +CREATE EXTENSION "pgxntool-test"; + +-- Re-enable notices +SET client_min_messages = NOTICE; diff --git a/template/test/expected/verify_install_marker.out b/template/test/expected/verify_install_marker.out new file mode 100644 index 0000000..cce1b07 --- /dev/null +++ b/template/test/expected/verify_install_marker.out @@ -0,0 +1,25 @@ +/* + * Verify test/install state persists into the main test suite + * + * DO NOT MODIFY THIS FILE WITHOUT EXPLICIT HUMAN APPROVAL. + * + * This test exists to catch a specific, subtle bug: if test/install and the + * main test suite run in separate pg_regress invocations, pg_regress drops + * and recreates the database between them, destroying everything test/install + * set up. The ONLY way test/install state can persist is if install files and + * regular test files run within a SINGLE pg_regress invocation (via a schedule + * file that lists install files first). + * + * If this test fails with "relation pgxntool_install_marker does not exist", + * it means the test-install implementation is broken: it's running install + * files in a separate pg_regress invocation instead of the same one. + * + * This file is intentionally minimal to prevent well-meaning "improvements" + * from masking the failure it's designed to detect. + */ +SELECT marker FROM pgxntool_install_marker; + marker +-------- + alive +(1 row) + diff --git a/template/test/install/.gitignore b/template/test/install/.gitignore new file mode 100644 index 0000000..cc779d7 --- /dev/null +++ b/template/test/install/.gitignore @@ -0,0 +1,2 @@ +# pg_regress artifacts +*.out.diff diff --git a/template/test/install/create_install_marker.out b/template/test/install/create_install_marker.out new file mode 100644 index 0000000..e636a61 --- /dev/null +++ b/template/test/install/create_install_marker.out @@ -0,0 +1,2 @@ +CREATE TABLE pgxntool_install_marker AS SELECT 'alive'::text AS marker; +SELECT 1 diff --git a/template/test/install/create_install_marker.sql b/template/test/install/create_install_marker.sql new file mode 100644 index 0000000..9f0dea5 --- /dev/null +++ b/template/test/install/create_install_marker.sql @@ -0,0 +1 @@ +CREATE TABLE pgxntool_install_marker AS SELECT 'alive'::text AS marker; diff --git a/template/test/output/pgxntool-test.source b/template/test/output/pgxntool-test.source new file mode 100644 index 0000000..7ecdd9a --- /dev/null +++ b/template/test/output/pgxntool-test.source @@ -0,0 +1,2 @@ +1..1 +ok 1 diff --git a/template/test/sql/verify_install_marker.sql b/template/test/sql/verify_install_marker.sql new file mode 100644 index 0000000..fa7752f --- /dev/null +++ b/template/test/sql/verify_install_marker.sql @@ -0,0 +1,20 @@ +/* + * Verify test/install state persists into the main test suite + * + * DO NOT MODIFY THIS FILE WITHOUT EXPLICIT HUMAN APPROVAL. + * + * This test exists to catch a specific, subtle bug: if test/install and the + * main test suite run in separate pg_regress invocations, pg_regress drops + * and recreates the database between them, destroying everything test/install + * set up. The ONLY way test/install state can persist is if install files and + * regular test files run within a SINGLE pg_regress invocation (via a schedule + * file that lists install files first). + * + * If this test fails with "relation pgxntool_install_marker does not exist", + * it means the test-install implementation is broken: it's running install + * files in a separate pg_regress invocation instead of the same one. + * + * This file is intentionally minimal to prevent well-meaning "improvements" + * from masking the failure it's designed to detect. + */ +SELECT marker FROM pgxntool_install_marker; diff --git a/test/CLAUDE.md b/test/CLAUDE.md index efc6413..d6af98a 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -2,6 +2,31 @@ This file provides guidance for AI assistants (like Claude Code) when working with the BATS test system in this directory. +## Critical Performance Directive: Minimize Commands in Tests + +**Every command run in the test suite makes the entire suite longer and slower.** The test suite is already slow; do not make it worse. + +**ALWAYS try to reduce the number of commands.** Before adding any command to a test, ask: can this be eliminated by restructuring the test or fixing the template? + +Specific guidelines: +- **Run the fewest commands possible** to verify a behavior +- **Combine related assertions** into a single test rather than separate tests when both require the same setup +- **Use `make -n` (dry run)** instead of the real target when you only need to check what would run +- **Avoid redundant state setup**: if the previous test already put the environment in the right state, don't repeat it + +## Critical Template Quality Directive + +**Tests MUST be set up correctly by the template.** If the template is not in a good working state, it creates extra work in many tests and defeats the purpose of isolated test environments. + +The whole reason independent tests use isolated environments (one environment per test suite) is so one test suite can modify things freely without disrupting others. That isolation only works if the template starts in a clean, working state. + +**What "good working state" means:** +- `make test` succeeds (or would succeed if PostgreSQL is running) +- All expected output files exist and are correct +- No workarounds are needed in tests to compensate for template deficiencies + +**Consequence**: If a test needs to work around a broken template (e.g., creating empty expected files, or needing `|| true` on `make test`), fix the template instead. + ## Critical Architecture Understanding ### The Foundation and Sequential Test Pattern diff --git a/test/bats b/test/bats index cfdec8f..509e2d7 160000 --- a/test/bats +++ b/test/bats @@ -1 +1 @@ -Subproject commit cfdec8ffec045351512e03d27679b12ce9cfed29 +Subproject commit 509e2d7f06cf0b2fde791539dee7245fd15ce56c diff --git a/test/lib/dist-expected-files.txt b/test/lib/dist-expected-files.txt index b00c4e7..81be30f 100644 --- a/test/lib/dist-expected-files.txt +++ b/test/lib/dist-expected-files.txt @@ -41,9 +41,17 @@ sql/pgxntool-test.sql # Test files (root level, copied from t/) test/ test/deps.sql +test/expected/ +test/expected/verify_install_marker.out test/input/ test/input/pgxntool-test.source +test/install/ +test/install/.gitignore +test/install/create_install_marker.out +test/install/create_install_marker.sql test/pgxntool +test/sql/ +test/sql/verify_install_marker.sql # pgxntool framework (the build system itself) pgxntool/ diff --git a/test/lib/foundation.bats b/test/lib/foundation.bats index bbd2b02..a3c67ae 100644 --- a/test/lib/foundation.bats +++ b/test/lib/foundation.bats @@ -252,7 +252,8 @@ In a real extension, these would already exist before adding pgxntool." # Validate prerequisites before attempting git subtree # 1. Check PGXNREPO is accessible and safe - if [ ! -d "$PGXNREPO/.git" ]; then + # Use -e instead of -d: for git worktrees, .git is a file, not a directory + if [ ! -e "$PGXNREPO/.git" ]; then # Not a local directory - must be a valid remote URL # Explicitly reject dangerous protocols first @@ -267,7 +268,7 @@ In a real extension, these would already exist before adding pgxntool." fi # 2. For local repos, verify branch exists - if [ -d "$PGXNREPO/.git" ]; then + if [ -e "$PGXNREPO/.git" ]; then if ! (cd "$PGXNREPO" && git rev-parse --verify "$PGXNBRANCH" >/dev/null 2>&1); then error "Branch $PGXNBRANCH does not exist in $PGXNREPO" fi @@ -276,7 +277,7 @@ In a real extension, these would already exist before adding pgxntool." # 3. Check if source repo is dirty and use rsync if needed # This matches the legacy test behavior in tests/clone local source_is_dirty=0 - if [ -d "$PGXNREPO/.git" ]; then + if [ -e "$PGXNREPO/.git" ]; then # SECURITY: rsync only works with local paths, never remote URLs if [[ "$PGXNREPO" == *://* ]]; then error "Cannot use rsync with remote URL: $PGXNREPO" diff --git a/test/lib/helpers.bash b/test/lib/helpers.bash index 18aee12..713d3c0 100644 --- a/test/lib/helpers.bash +++ b/test/lib/helpers.bash @@ -1411,8 +1411,9 @@ build_test_repo_from_template() { } # Check if pgxntool repo is dirty + # Note: Use -e instead of -d to handle git worktrees where .git is a file local source_is_dirty=0 - if [ -d "$PGXNREPO/.git" ]; then + if [ -e "$PGXNREPO/.git" ]; then if [ -n "$(cd "$PGXNREPO" && git status --porcelain)" ]; then source_is_dirty=1 local pgxn_branch diff --git a/test/standard/doc.bats b/test/standard/doc.bats index cdce724..3cdb45e 100755 --- a/test/standard/doc.bats +++ b/test/standard/doc.bats @@ -75,20 +75,9 @@ setup() { fi } -@test "documentation source files exist" { - # OK to fail: ls returns non-zero if no files match, which would mean test should fail - local doc_files=$(ls "$TEST_DIR/doc_repo/doc"/*.adoc "$TEST_DIR/doc_repo/doc"/*.asciidoc 2>/dev/null || echo) - [ -n "$doc_files" ] -} - @test "ASCIIDOC='' make install does not create docs" { cd "$TEST_DIR/doc_repo" - # Remove any existing HTML files - local input=$(ls doc/*.adoc doc/*.asciidoc 2>/dev/null) - local expected=$(echo "$input" | sed -Ee 's/(adoc|asciidoc)$/html/') - rm -f $expected - # Install without ASCIIDOC (should fail, but we only care about HTML files not being created) run env ASCIIDOC='' make install # Don't check status - we're testing that HTML files aren't created, not that install succeeds @@ -98,6 +87,12 @@ setup() { [ -z "$html" ] } +@test "documentation source files exist" { + # OK to fail: ls returns non-zero if no files match, which would mean test should fail + local doc_files=$(ls "$TEST_DIR/doc_repo/doc"/*.adoc "$TEST_DIR/doc_repo/doc"/*.asc "$TEST_DIR/doc_repo/doc"/*.asciidoc 2>/dev/null || echo) + [ -n "$doc_files" ] +} + @test "ASCIIDOC='' make html fails" { cd "$TEST_DIR/doc_repo" @@ -108,9 +103,9 @@ setup() { @test "make test creates documentation" { cd "$TEST_DIR/doc_repo" - # Get expected HTML files - local input=$(ls doc/*.adoc doc/*.asciidoc 2>/dev/null) - local expected=$(echo "$input" | sed -Ee 's/(adoc|asciidoc)$/html/') + # Get expected HTML files (all asciidoc extensions: adoc, asc, asciidoc) + local input=$(ls doc/*.adoc doc/*.asc doc/*.asciidoc 2>/dev/null) + local expected=$(echo "$input" | sed -Ee 's/(adoc|asc|asciidoc)$/html/') # Run make test (may fail if PostgreSQL not running, but we only care about HTML generation) run make test diff --git a/test/standard/make-results.bats b/test/standard/make-results.bats index b8f7146..78fe09c 100755 --- a/test/standard/make-results.bats +++ b/test/standard/make-results.bats @@ -94,7 +94,8 @@ setup() { @test "make results updates expected output" { # Run make results to fix the expected output - run make results + # Disable verify-results since we intentionally created a mismatch to test this + run make PGXNTOOL_ENABLE_VERIFY_RESULTS=no results assert_success } diff --git a/test/standard/test-install-persistence.bats b/test/standard/test-install-persistence.bats new file mode 100644 index 0000000..0b56da1 --- /dev/null +++ b/test/standard/test-install-persistence.bats @@ -0,0 +1,50 @@ +#!/usr/bin/env bats + +# Test: test/install persistence - end-to-end validation +# +# This test verifies the CORE CONTRACT of test/install: that state created by +# test/install files persists into the main test suite. +# +# The template includes: +# test/install/create_install_marker.sql - Creates a table with a marker row +# test/sql/verify_install_marker.sql - Queries that table +# +# If test/install and regular tests run in the same pg_regress invocation +# (correct), the marker test passes. If they run in separate invocations +# (broken), the database gets dropped and recreated between them, and the +# marker test fails. + +load ../lib/helpers + +setup_file() { + setup_topdir + load_test_env "install-persistence" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "install-persistence" + cd "$TEST_REPO" +} + +@test "test/install is auto-detected as enabled" { + # With test/install/ files present, schedule files should be generated + run make -n test 2>&1 + assert_success + echo "$output" | grep -q "schedule" +} + +@test "install marker state persists into main test suite" { + skip_if_no_postgres + + run make test + + # The marker test verifies that test/install state (a table) survived into + # the main test suite — meaning both phases ran in the same pg_regress + # invocation. Check that it produced the expected output. + assert_file_exists test/results/verify_install_marker.out + run diff test/expected/verify_install_marker.out test/results/verify_install_marker.out + assert_success +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/test-test-build.bats b/test/standard/test-test-build.bats new file mode 100644 index 0000000..3c94c88 --- /dev/null +++ b/test/standard/test-test-build.bats @@ -0,0 +1,95 @@ +#!/usr/bin/env bats + +# Test: test-build feature +# +# Tests that the test-build feature works correctly. The template includes +# test/build/ with a working SQL file so most tests run against that baseline; +# later tests temporarily modify files as needed. +# +# - test-build auto-detects when test/build/ has SQL files +# - test-build runs successfully with the template SQL +# - test-build fails and reports errors when SQL has errors +# - test-build can be disabled via PGXNTOOL_ENABLE_TEST_BUILD +# - test-build runs before regular tests when enabled +# - test-build target is absent when test/build/ is removed +# - PGXNTOOL_ENABLE_TEST_BUILD=yes errors when test/build/ is empty or missing + +load ../lib/helpers + +setup_file() { + # Set TOPDIR + setup_topdir + + # Independent test - gets its own isolated environment with foundation TEST_REPO + load_test_env "test-build" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "test-build" + cd "$TEST_REPO" +} + +@test "test-build target exists when test/build/ has SQL files" { + # Template includes test/build/simple_build_test.sql + run make -n test-build 2>&1 + [ "$status" -eq 0 ] +} + +@test "test-build runs successfully with template SQL" { + skip_if_no_postgres + run make test-build + [ "$status" -eq 0 ] +} + +@test "test-build fails with invalid SQL" { + skip_if_no_postgres + # Temporarily add an invalid SQL file + cat > test/build/invalid_test.sql <<'EOF' +-- This SQL has a syntax error +SELECT FROM nonexistent_table WHERE; +EOF + + run make test-build + local status_code=$status + # Remove the invalid file so subsequent tests start clean + rm -f test/build/invalid_test.sql test/build/expected/invalid_test.out + + [ "$status_code" -ne 0 ] +} + +@test "test-build can be disabled via PGXNTOOL_ENABLE_TEST_BUILD" { + # Empty string on command line also disables (same as =no); see docs for why + run make list PGXNTOOL_ENABLE_TEST_BUILD= + assert_success + assert_not_contains "$output" "test-build" +} + +@test "test-build can be disabled via PGXNTOOL_ENABLE_TEST_BUILD=no" { + run make list PGXNTOOL_ENABLE_TEST_BUILD=no + assert_success + assert_not_contains "$output" "test-build" +} + +@test "test target includes test-build when enabled" { + run make -n test 2>&1 + assert_success + assert_contains "$output" "test-build" +} + +@test "test-build target does not exist when test/build/ is removed" { + rm -rf test/build + + run make list + assert_success + assert_not_contains "$output" "test-build" +} + +@test "PGXNTOOL_ENABLE_TEST_BUILD=yes errors when test/build/ is missing" { + # test/build/ was removed in the previous test + # Setting =yes explicitly should cause an error when no files are found + run make test-build PGXNTOOL_ENABLE_TEST_BUILD=yes + assert_failure +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/test-test-install.bats b/test/standard/test-test-install.bats new file mode 100644 index 0000000..2d7856b --- /dev/null +++ b/test/standard/test-test-install.bats @@ -0,0 +1,88 @@ +#!/usr/bin/env bats + +# Test: test/install feature +# +# Tests that the test/install feature works correctly. The template includes +# test/install/create_install_marker.sql so most tests run against that baseline. +# +# - test/install is auto-detected when test/install/ has SQL files +# - Schedule files reference install files with ../install/ prefix +# - test/install can be disabled via PGXNTOOL_ENABLE_TEST_INSTALL +# - make clean removes generated schedule files +# - test/install is not enabled when test/install/ is empty +# - test/install can be disabled even when test/install/ has SQL files + +load ../lib/helpers + +setup_file() { + # Set TOPDIR + setup_topdir + + # Independent test - gets its own isolated environment with foundation TEST_REPO + load_test_env "test-install" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "test-install" + cd "$TEST_REPO" +} + +@test "test/install is auto-detected when test/install/ has SQL files" { + # Template includes test/install/create_install_marker.sql + run make -n test 2>&1 + assert_success + echo "$output" | grep -q "schedule" +} + +@test "install schedule lists files with ../install/ prefix" { + run make test/install/schedule + assert_success + assert_file_exists "test/install/schedule" + + # Schedule should reference install files with relative path + run grep "../install/create_install_marker" test/install/schedule + assert_success +} + +@test "test/install can be disabled even when directory has SQL files" { + # Even with files present, disabled means no schedule in the plan + run make -n test PGXNTOOL_ENABLE_TEST_INSTALL=no 2>&1 + assert_success + assert_not_contains "$output" "install/schedule" +} + +@test "make clean removes install schedule file" { + # Generate schedule file first + make test/install/schedule + + assert_file_exists "test/install/schedule" + + run make clean + assert_success + + assert_file_not_exists "test/install/schedule" +} + +@test "test/install not enabled when test/install/ is empty" { + # Temporarily remove all SQL files from test/install + rm -f test/install/*.sql + + run make -n test 2>&1 + local status_code=$status + + # Restore committed template files + git checkout -- test/install/ + + assert_success # make -n should succeed + # Should NOT reference install schedule + ! echo "$output" | grep -q "install/schedule" +} + +@test "repository is still functional after test/install tests" { + assert_file_exists "Makefile" + run make --version + assert_success +} + +# vi: expandtab sw=2 ts=2 diff --git a/test/standard/test-verify-results.bats b/test/standard/test-verify-results.bats new file mode 100644 index 0000000..4c120e0 --- /dev/null +++ b/test/standard/test-verify-results.bats @@ -0,0 +1,95 @@ +#!/usr/bin/env bats + +# Test: verify-results feature +# +# Tests that the verify-results feature works correctly: +# - verify-results succeeds when no test failures exist +# - verify-results fails and blocks make results when tests are failing +# - verify-results can be disabled via PGXNTOOL_ENABLE_VERIFY_RESULTS +# - verify-results has no dependencies (doesn't run tests itself) + +load ../lib/helpers + +setup_file() { + # Set TOPDIR + setup_topdir + + # Independent test - gets its own isolated environment with foundation TEST_REPO + load_test_env "verify-results" + ensure_foundation "$TEST_DIR" +} + +setup() { + load_test_env "verify-results" + cd "$TEST_REPO" +} + +@test "make results succeeds when tests are passing" { + skip_if_no_postgres + # Template is in clean state, no regression.diffs should exist + run make results + assert_success +} + +@test "verify-results target exists (pgTap is default)" { + run make -n verify-results 2>&1 + assert_success +} + +@test "verify-results succeeds when no test failures exist" { + # Template is in clean state, no regression.diffs + run make verify-results + assert_success +} + +@test "verify-results fails and blocks make results when tests are failing" { + # COMBINED: test both verify-results failure AND make results being blocked + echo "test diff" > test/regression.diffs + + run make verify-results + assert_failure + + # Check the expected error messages using assert_contains + assert_contains "$output" "ERROR: Tests are failing. Cannot run 'make results'." + assert_contains "$output" "Fix test failures first, then run 'make results'." + assert_contains "$output" "See test/regression.diffs for details:" + assert_contains "$output" "test diff" + + # make results should also be blocked (regression.diffs still present from above) + run make results + assert_failure +} + +@test "verify-results can be disabled via PGXNTOOL_ENABLE_VERIFY_RESULTS" { + run make list PGXNTOOL_ENABLE_VERIFY_RESULTS= + assert_success + assert_not_contains "$output" "verify-results" +} + +@test "verify-results has no dependencies" { + # verify-results should not trigger make test or installcheck + run make -n verify-results 2>&1 + assert_success + # Should not print pg_regress or installcheck commands + assert_not_contains "$output" "pg_regress" + assert_not_contains "$output" "installcheck" +} + +@test "verify-results only checks file existence, not test results" { + # regression.diffs still present from "fails and blocks" test above — no need to recreate + start_time=$(date +%s) + run make verify-results 2>&1 + end_time=$(date +%s) + elapsed=$((end_time - start_time)) + + [ "$elapsed" -lt 2 ] + assert_failure +} + +@test "repository is still functional after verify-results tests" { + assert_file_exists "Makefile" + run make --version + assert_success +} + +# vi: expandtab sw=2 ts=2