Skip to content

feat: add doctest tool and Bats harness for Markdown code blocks - #84

Open
trevor-vaughan wants to merge 2 commits into
mainfrom
feat/testable-documentaiton
Open

feat: add doctest tool and Bats harness for Markdown code blocks#84
trevor-vaughan wants to merge 2 commits into
mainfrom
feat/testable-documentaiton

Conversation

@trevor-vaughan

Copy link
Copy Markdown
Member

Go tool (cmd/doctest/) uses goldmark to parse fenced code blocks
annotated with {test="..."} from Markdown files. Bats test harness
runs extracted snippets. Motivated by 11 open issues where Getting
Started page commands were broken or untested.

  • Add cmd/doctest/ with extract and coverage subcommands
  • Add Bats test harness via npm (bats-core, bats-support, bats-assert)
  • Add Makefile targets: test-docs-extract, test-docs, test-docs-coverage
  • Wire doc tests into CI (informational, non-blocking until blocks are annotated)
  • Add spec and plan in specs/015-testable-documentation/
  • Update CONTRIBUTING.md and README.md; create AGENTS.md

Details:

Go tool (cmd/doctest/):

  • goldmark AST parser extracts annotated fenced code blocks to disk
  • coverage subcommand exits non-zero when untested executable blocks exist
  • Table-driven unit tests cover parsing, extraction, slug derivation, and coverage

Bats harness (tests/docs/):

  • Installed as npm devDependencies (project already uses npm for Hugo)
  • Helper scripts wrap snippet execution; skeleton test for getting-started guide

Build and CI:

  • DOCTEST_DIR variable decouples extract output path from test input path
  • Go tests run before doc tests so coverage failures don't block unit tests
  • Doc test step uses || true until remaining blocks are annotated
  • .gitignore: .test-output/, doctest binary, cmd/sync-content/sync-content

Assisted-by: Claude Opus 4.6
Signed-off-by: Trevor Vaughan tvaughan@redhat.com

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

✅ CRAP Load Analysis: PASS (no baseline)

No baseline file found at .gaze/baseline.json. Showing current scores without regression detection.

How to Enable Regression Detection

Generate and commit a baseline file to track CRAP score changes over time:

# 1. Install gaze
go install github.com/unbound-force/gaze/cmd/gaze@latest

# 2. Run tests and generate baseline
go test -coverprofile=coverage.out ./...
mkdir -p .gaze
gaze crap --format=json --coverprofile=coverage.out ./... > .gaze/baseline.json

# 3. Commit the baseline
git add .gaze/baseline.json
git commit -m "chore: add CRAP baseline for regression detection"

For more information:

Summary

Metric Value
Functions analysed 14
Avg complexity 8.4
Avg line coverage 83.1%
Avg CRAP score 9.1
CRAPload (>= 15) 2
Avg contract coverage 0%
Avg GazeCRAP score 0
GazeCRAPload (>= 15) 0

View full analysis logs

@trevor-vaughan
trevor-vaughan force-pushed the feat/testable-documentaiton branch from 7422178 to 6c3596b Compare July 6, 2026 19:52
@trevor-vaughan
trevor-vaughan marked this pull request as ready for review July 6, 2026 20:27
@trevor-vaughan
trevor-vaughan requested a review from a team as a code owner July 6, 2026 20:27
@trevor-vaughan
trevor-vaughan force-pushed the feat/testable-documentaiton branch from 6c3596b to 9a3ecd9 Compare July 7, 2026 20:25
@trevor-vaughan
trevor-vaughan force-pushed the feat/testable-documentaiton branch from 9a3ecd9 to 84d90b9 Compare July 28, 2026 18:41
@trevor-vaughan
trevor-vaughan requested a review from a team as a code owner July 28, 2026 18:41
@trevor-vaughan
trevor-vaughan requested a review from em-redhat July 28, 2026 18:41
@trevor-vaughan
trevor-vaughan force-pushed the feat/testable-documentaiton branch from 84d90b9 to 8a4fde0 Compare July 31, 2026 19:07

@em-redhat em-redhat left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #84 — Doctest Tool and Bats Harness

Overview

This PR adds solid infrastructure for testing documentation code blocks — the Go extractor is well-designed (goldmark is the right choice, matching Hugo's own parser), the test suite is comprehensive for the parsing logic, and the git-tracking integration is smart. The motivation (11 open issues for broken Getting Started commands) is clear and the approach is sound.

However, there are several issues that need attention before merging, ranging from CI pipeline concerns to security considerations and test coverage gaps.


CI Status

The "Standardized CI / Run linters" check is failing on this PR (passes on main):

  • golangci-lint: 2 gosec findings (G122, G306) — addressed in inline comments below
  • markdownlint: 709 errors in specs/015-testable-documentation/plan.md and spec.md (tabs, missing code fence languages, table formatting)

The markdownlint errors in spec files should be cleaned up — 709 errors is a lot of noise for a linter check. At minimum, the hard tabs and missing code fence languages in plan.md need fixing if the project's markdownlint config applies to specs/.


Summary of Findings

Severity Count Category
HIGH 4 CI visibility, file permissions, stale output dir, test coverage
MEDIUM 5 Deploy workflow, /tmp path, plan divergence, output cleanup, run() testability
LOW 3 Bats placeholder, test assertion gaps, frontmatter edge case
NIT 2 Comment suggestions

HIGH Issues

  1. || true makes doc test failures completely invisible in CI — Both ci.yml and deploy-gh-pages.yml use make test-docs || true. Combined with the - prefix in the Makefile, there is no path through which a doctest failure surfaces to a developer. Use continue-on-error: true on the GitHub Actions step instead — failures show as a yellow warning in the UI without blocking the pipeline. (See inline comments.)

  2. gosec G306: os.WriteFile with 0o644 — Extracted snippets and manifests should use 0o600 per least-privilege. On shared CI runners, world-readable build artifacts are unnecessary. (See inline comment on extract.go.)

  3. No output directory cleanup before extractionrunExtract writes to outputDir but never cleans it. If a code block is renamed or removed, stale snippets persist and Bats tests run against outdated content. Add rm -rf $(DOCTEST_DIR) before extraction in the Makefile, or os.RemoveAll(outputDir) in the Go code.

  4. run() in main.go is untested — The main() → run() int pattern exists specifically for testability, but there are zero tests for run(). It has 6+ exit paths (no args, missing flags, unknown subcommand, extract error, coverage error, success). Additionally, run() uses os.Args directly and flag.ExitOnError, making it difficult to test without global state mutation. Consider accepting []string args and using flag.ContinueOnError.

MEDIUM Issues

  1. Doc tests in the deploy workflowdeploy-gh-pages.yml runs make test-docs || true between the Hugo build and actions/upload-pages-artifact. Tests should gate PRs (in ci.yml), not deployments. When Phase 2 removes || true, a flaky doc test would block production deploys.

  2. /tmp/doctest-snippets as default output path — Predictable path in world-writable /tmp. On shared runners or multi-user systems, concurrent runs could collide. Consider .test-output/doctest-snippets (already gitignored) or mktemp -d.

  3. Plan-to-implementation divergence — The 1590-line plan.md contains full source code copies that are already stale: pageSlug behavior differs (plan would cause slug collisions), runCoverage return behavior differs (plan returns nil always, code returns error), gitTrackedFiles is absent from the plan entirely, nonTestableLangs is defined in the plan but not implemented. Consider either updating the plan or removing full code listings (they're a maintenance burden).

  4. runCoverage happy path not tested — Tests cover the "untested blocks found" error path and the opt-out path, but not the case where all executable blocks are annotated and runCoverage returns nil.

  5. - prefix in Makefile undocumented — The make test target says "Run all tests" but silently ignores doc test and coverage failures via -. Add inline comments explaining the Phase 1/Phase 2 intent so future contributors understand why errors are swallowed.

LOW Issues

  1. Bats test file is a pure placeholdergetting-started.bats has zero @test blocks. make test-docs runs 0 tests = always passes. Consider adding at least one real test to validate the end-to-end pipeline.

  2. lineNumber never asserted in testsTestExtractBlocksBasic checks lang, testName, and content but never asserts b.line. Line number accuracy matters for developer experience (error messages, coverage reports).

  3. parseFrontmatter edge case — Uses bytes.HasPrefix(source, []byte("---")) without requiring a trailing newline. ---foo at the start of a file would match. Consider requiring ---\n as the opening delimiter.


What's Good

  • goldmark is the right dependency — Same Markdown parser Hugo uses internally, avoiding parser divergence bugs
  • gitTrackedFiles is a smart design choice — Prevents false positives from generated/synced content
  • Test isolation is thorought.TempDir(), GIT_CONFIG_GLOBAL=/dev/null, isolated git repos
  • Code quality is clean — Good error messages, clear function boundaries, proper run() int pattern
  • Spec was updatedspec.md correctly reflects implementation decisions (npm vs git submodules, exit codes)

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/deploy-gh-pages.yml
Comment thread cmd/doctest/extract.go Outdated
Comment thread cmd/doctest/extract.go
Comment thread Makefile
Comment thread cmd/doctest/main.go
trevor-vaughan added a commit that referenced this pull request Aug 11, 2026
- fix(security): tighten snippet and manifest permissions to 0o600 (G306)
- fix(security): skip symlinks in WalkDir callbacks (G122)
- refactor(doctest): accept explicit args slice with ContinueOnError for testable CLI
  - flag.ExitOnError called os.Exit, making run() untestable
- fix(doctest): clear output dir contents in place instead of removing the directory
  - rejects overlapping content/output dirs before touching the filesystem
  - preserves the directory inode for any process holding a handle
- fix(doctest): require "---\n" opening delimiter, not bare "---"
- ci: replace "|| true" with continue-on-error for real step status
  - step failures were invisible in GitHub Actions logs
- build(doctest): move default snippet dir from /tmp to .test-output/doctest-snippets
- test(doctest): add table-driven exit-code tests for run() and coverage happy path
- test(doctest): add end-to-end harness smoke tests and snippet_origin helper
- docs(spec): strip stale source listings from plan, fix paths, add markdownlint guards

Refs: #84
Assisted-by: Claude Opus 4.8
Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
Go tool (cmd/doctest/) uses goldmark to parse fenced code blocks
annotated with {test="..."} from Markdown files. Bats test harness
runs extracted snippets. Motivated by 11 open issues where Getting
Started page commands were broken or untested.

- Add cmd/doctest/ with extract and coverage subcommands
- Add Bats test harness via npm (bats-core, bats-support, bats-assert)
- Add Makefile targets: test-docs-extract, test-docs, test-docs-coverage
- Wire doc tests into CI (informational, non-blocking until blocks are annotated)
- Add spec and plan in specs/015-testable-documentation/
- Update CONTRIBUTING.md and README.md; create AGENTS.md

Details:

Go tool (cmd/doctest/):
- goldmark AST parser extracts annotated fenced code blocks to disk
- coverage subcommand exits non-zero when untested executable blocks exist
- Table-driven unit tests cover parsing, extraction, slug derivation, and coverage
- Ignores gitignored files during validation

Bats harness (tests/docs/):
- Installed as npm devDependencies (project already uses npm for Hugo)
- Helper scripts wrap snippet execution; skeleton test for getting-started guide

Build and CI:
- DOCTEST_DIR variable decouples extract output path from test input path
- Go tests run before doc tests so coverage failures don't block unit tests
- Doc test step uses || true until remaining blocks are annotated
- .gitignore: .test-output/, doctest binary, cmd/sync-content/sync-content

Assisted-by: Claude Opus 4.6
Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
- fix(security): tighten snippet and manifest permissions to 0o600 (G306)
- fix(security): skip symlinks in WalkDir callbacks (G122)
- refactor(doctest): accept explicit args slice with ContinueOnError for testable CLI
  - flag.ExitOnError called os.Exit, making run() untestable
- fix(doctest): clear output dir contents in place instead of removing the directory
  - rejects overlapping content/output dirs before touching the filesystem
  - preserves the directory inode for any process holding a handle
- fix(doctest): require "---\n" opening delimiter, not bare "---"
- ci: replace "|| true" with continue-on-error for real step status
  - step failures were invisible in GitHub Actions logs
- build(doctest): move default snippet dir from /tmp to .test-output/doctest-snippets
- test(doctest): add table-driven exit-code tests for run() and coverage happy path
- test(doctest): add end-to-end harness smoke tests and snippet_origin helper
- docs(spec): strip stale source listings from plan, fix paths, add markdownlint guards

Refs: #84
Assisted-by: Claude Opus 4.8
Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
@trevor-vaughan
trevor-vaughan force-pushed the feat/testable-documentaiton branch from 40d7509 to 6ef5745 Compare August 11, 2026 22:21
@trevor-vaughan

Copy link
Copy Markdown
Member Author

@em-redhat Sorry this took so long. I've addressed your review items and run things through a couple more reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants