feat: add doctest tool and Bats harness for Markdown code blocks - #84
feat: add doctest tool and Bats harness for Markdown code blocks#84trevor-vaughan wants to merge 2 commits into
Conversation
✅ CRAP Load Analysis: PASS (no baseline)No baseline file found at How to Enable Regression DetectionGenerate 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
|
7422178 to
6c3596b
Compare
6c3596b to
9a3ecd9
Compare
9a3ecd9 to
84d90b9
Compare
84d90b9 to
8a4fde0
Compare
em-redhat
left a comment
There was a problem hiding this comment.
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.mdandspec.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
-
|| truemakes doc test failures completely invisible in CI — Bothci.ymlanddeploy-gh-pages.ymlusemake test-docs || true. Combined with the-prefix in the Makefile, there is no path through which a doctest failure surfaces to a developer. Usecontinue-on-error: trueon the GitHub Actions step instead — failures show as a yellow warning in the UI without blocking the pipeline. (See inline comments.) -
gosec G306:
os.WriteFilewith0o644— Extracted snippets and manifests should use0o600per least-privilege. On shared CI runners, world-readable build artifacts are unnecessary. (See inline comment onextract.go.) -
No output directory cleanup before extraction —
runExtractwrites tooutputDirbut never cleans it. If a code block is renamed or removed, stale snippets persist and Bats tests run against outdated content. Addrm -rf $(DOCTEST_DIR)before extraction in the Makefile, oros.RemoveAll(outputDir)in the Go code. -
run()inmain.gois untested — Themain() → run() intpattern exists specifically for testability, but there are zero tests forrun(). It has 6+ exit paths (no args, missing flags, unknown subcommand, extract error, coverage error, success). Additionally,run()usesos.Argsdirectly andflag.ExitOnError, making it difficult to test without global state mutation. Consider accepting[]stringargs and usingflag.ContinueOnError.
MEDIUM Issues
-
Doc tests in the deploy workflow —
deploy-gh-pages.ymlrunsmake test-docs || truebetween the Hugo build andactions/upload-pages-artifact. Tests should gate PRs (inci.yml), not deployments. When Phase 2 removes|| true, a flaky doc test would block production deploys. -
/tmp/doctest-snippetsas 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) ormktemp -d. -
Plan-to-implementation divergence — The 1590-line
plan.mdcontains full source code copies that are already stale:pageSlugbehavior differs (plan would cause slug collisions),runCoveragereturn behavior differs (plan returnsnilalways, code returns error),gitTrackedFilesis absent from the plan entirely,nonTestableLangsis defined in the plan but not implemented. Consider either updating the plan or removing full code listings (they're a maintenance burden). -
runCoveragehappy 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 andrunCoveragereturnsnil. -
-prefix in Makefile undocumented — Themake testtarget 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
-
Bats test file is a pure placeholder —
getting-started.batshas zero@testblocks.make test-docsruns 0 tests = always passes. Consider adding at least one real test to validate the end-to-end pipeline. -
lineNumbernever asserted in tests —TestExtractBlocksBasiccheckslang,testName, andcontentbut never assertsb.line. Line number accuracy matters for developer experience (error messages, coverage reports). -
parseFrontmatteredge case — Usesbytes.HasPrefix(source, []byte("---"))without requiring a trailing newline.---fooat the start of a file would match. Consider requiring---\nas the opening delimiter.
What's Good
- goldmark is the right dependency — Same Markdown parser Hugo uses internally, avoiding parser divergence bugs
gitTrackedFilesis a smart design choice — Prevents false positives from generated/synced content- Test isolation is thorough —
t.TempDir(),GIT_CONFIG_GLOBAL=/dev/null, isolated git repos - Code quality is clean — Good error messages, clear function boundaries, proper
run() intpattern - Spec was updated —
spec.mdcorrectly reflects implementation decisions (npm vs git submodules, exit codes)
- 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>
40d7509 to
6ef5745
Compare
|
@em-redhat Sorry this took so long. I've addressed your review items and run things through a couple more reviews. |
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.
Details:
Go tool (cmd/doctest/):
Bats harness (tests/docs/):
Build and CI:
Assisted-by: Claude Opus 4.6
Signed-off-by: Trevor Vaughan tvaughan@redhat.com