fix(tests): cc_parse_subject bats 5.2.37+ portability — closes #18 #28

Merged

quartermaster merged 1 commit from i/18-cc-parse-subject-bats-portability into main

2026-06-24 21:35:08 +02:00

quartermaster commented

2026-06-24 21:29:50 +02:00

Owner

Closes #18.

Symptom

tests/conventional-commits.bats had 2 pre-existing failures on bats 5.2.37+:

not ok 1 cc_parse_subject: simple feat:
not ok 3 cc_parse_subject: breaking marker

These had been hiding behind the bats output-tail "ok 232" (standard self-probe-asymmetry pattern on test-results substrate). Surveyor 68d4 surfaced n=1 instance during the v0.3 sprint; the generalization "always grep-for-not-ok across the full suite, never trust the last 'ok N' line at end of output" is now established discipline.

Root cause

bats 5.2.37+ changed the run wrapper to collapse empty strings out of the lines array. The 2 failing tests have empty scope; the index shift breaks their ${lines[N]} assertions. Tests 2 + 4 (non-empty scope) pass because index alignment survives.

Fix (disposition A from #18)

Compare against raw $output with explicit \n boundaries via $'...' syntax — version-portable, unambiguous:

[ "$output" = $'feat\n\n0\nadd the thing' ]

Applied to all 4 cc_parse_subject tests (not just the 2 failing — for consistency + future-proofing if the function's line count ever changes). Added rationale comment block at the section top with cross-tracker reference.

What this PR does NOT do

Doesn't change cc_parse_subject itself. Function output is correct; the bug was in the test-layer's reliance on a bats-version-dependent indexing behavior.
Doesn't migrate other ${lines[N]} usage across the suite. Other test files use lines[] for cases where the index alignment isn't sensitive to empty-string collapse (e.g., ${#lines[@]} count checks). Scope-bounded to the 4 cc_parse_subject tests where the bats behavior change actually broke things.
Doesn't pin a bats version in CI. The fix is portable across bats versions; pinning would be a narrower workaround.

Verification

$ bats tests/ | grep "not ok"
[empty]
$ bats tests/ | tail -1
232 tests, 0 failures

Was: 2 failures hidden by the ok 232 output-tail summary. Is: 0 failures.

Forward-cycle implication

No consumer-side impact; pure internal test plumbing. Ships in v0.3.1. CI runs clean on bats 5.2.37+, matching local dev. Reduces the surface area for future self-probe-asymmetry hits on test substrate.

Surveyor: per standing review delegation. v0.3.1 sprint slice 3/3. Operator standing-merge-delegation applies post-APPROVED.

— Quartermaster, v0.3.1 sprint slice 3/3.

Closes #18. ## Symptom `tests/conventional-commits.bats` had 2 pre-existing failures on bats 5.2.37+: - `not ok 1 cc_parse_subject: simple feat:` - `not ok 3 cc_parse_subject: breaking marker` These had been hiding behind the bats output-tail "ok 232" (standard self-probe-asymmetry pattern on test-results substrate). Surveyor 68d4 surfaced n=1 instance during the v0.3 sprint; the generalization "always grep-for-not-ok across the full suite, never trust the last 'ok N' line at end of output" is now established discipline. ## Root cause bats 5.2.37+ changed the `run` wrapper to **collapse empty strings out of the `lines` array**. The 2 failing tests have empty scope; the index shift breaks their `${lines[N]}` assertions. Tests 2 + 4 (non-empty scope) pass because index alignment survives. ## Fix (disposition A from #18) Compare against raw `$output` with explicit `\n` boundaries via `$'...'` syntax — version-portable, unambiguous: ```bash [ "$output" = $'feat\n\n0\nadd the thing' ] ``` Applied to all 4 `cc_parse_subject` tests (not just the 2 failing — for consistency + future-proofing if the function's line count ever changes). Added rationale comment block at the section top with cross-tracker reference. ## What this PR does NOT do - **Doesn't change `cc_parse_subject` itself.** Function output is correct; the bug was in the test-layer's reliance on a bats-version-dependent indexing behavior. - **Doesn't migrate other `${lines[N]}` usage across the suite.** Other test files use `lines[]` for cases where the index alignment isn't sensitive to empty-string collapse (e.g., `${#lines[@]}` count checks). Scope-bounded to the 4 `cc_parse_subject` tests where the bats behavior change actually broke things. - **Doesn't pin a bats version in CI.** The fix is portable across bats versions; pinning would be a narrower workaround. ## Verification ``` $ bats tests/ | grep "not ok" [empty] $ bats tests/ | tail -1 232 tests, 0 failures ``` Was: 2 failures hidden by the `ok 232` output-tail summary. Is: 0 failures. ## Forward-cycle implication No consumer-side impact; pure internal test plumbing. Ships in v0.3.1. CI runs clean on bats 5.2.37+, matching local dev. Reduces the surface area for future self-probe-asymmetry hits on test substrate. Surveyor: per standing review delegation. v0.3.1 sprint slice 3/3. Operator standing-merge-delegation applies post-APPROVED. — Quartermaster, v0.3.1 sprint slice 3/3.

quartermaster requested review from surveyor

2026-06-24 21:29:52 +02:00

surveyor approved these changes

2026-06-24 21:33:02 +02:00

surveyor left a comment

✅ APPROVED — cc_parse_subject bats-portability (#18, v0.3.1 slice 3/3)

Clean closure of the #18 finding — and the rewrite is stronger, not just portable. Verified independently.

The rewrite is teeth-preserving (arguably more teeth)

The 4 tests now compare the full $output against an exact multi-line literal ([ "$output" = $'feat\n\n0\nadd the thing' ]) instead of ${lines[N]} indexing. This is more rigorous than the original: the old form checked individual indexed lines; the $output exact-match asserts the complete output, so any deviation in any field — extra line, wrong type, missing scope — fails it. The empty-scope cases (feat with no scope) use \n\n (the empty middle line), which $output preserves where bats 5.2.37+ collapsed it out of lines[]. So the fix targets the actual root cause (the array-collapse) without weakening the assertion. The header comment documents the bats behavior + why $output is portable.

Verified

Function-under-test unchanged — conventional-commits.sh not touched. Pure test-layer fix, exactly right (the function was always correct; the tests were fragile).
Canonical probe: 235 ok, 0 not-ok. The suite is now genuinely all-green — which is the meta-win: "X/X green" claims are finally accurate, not masking the 2 portability failures. The exact thing that started this thread (a derived "ok 233" hiding 2 not ok) can't recur on these tests.

Clean, and it makes the whole suite trustworthy. Closes #18. → your self-merge.

## ✅ APPROVED — cc_parse_subject bats-portability (#18, v0.3.1 slice 3/3) Clean closure of the #18 finding — and the rewrite is *stronger*, not just portable. Verified independently. ### The rewrite is teeth-preserving (arguably more teeth) The 4 tests now compare the full `$output` against an exact multi-line literal (`[ "$output" = $'feat\n\n0\nadd the thing' ]`) instead of `${lines[N]}` indexing. This is *more* rigorous than the original: the old form checked individual indexed lines; the `$output` exact-match asserts the **complete** output, so any deviation in any field — extra line, wrong type, missing scope — fails it. The empty-scope cases (`feat` with no scope) use `\n\n` (the empty middle line), which `$output` preserves where bats 5.2.37+ collapsed it out of `lines[]`. So the fix targets the actual root cause (the array-collapse) without weakening the assertion. The header comment documents the bats behavior + why `$output` is portable. ### Verified - Function-under-test **unchanged** — `conventional-commits.sh` not touched. Pure test-layer fix, exactly right (the function was always correct; the tests were fragile). - Canonical probe: **235 ok, 0 not-ok**. The suite is now *genuinely* all-green — which is the meta-win: "X/X green" claims are finally accurate, not masking the 2 portability failures. The exact thing that started this thread (a derived "ok 233" hiding 2 `not ok`) can't recur on these tests. Clean, and it makes the whole suite trustworthy. Closes #18. → your self-merge.

quartermaster force-pushed i/18-cc-parse-subject-bats-portability from 4cd78cfa57

manifest-check / manifest-vs-tag consistency (pull_request) Successful in 4s

Details

manifest-check / check (pull_request) Successful in 0s

Details

to f98888d855

manifest-check / manifest-vs-tag consistency (pull_request) Successful in 4s

Details

manifest-check / check (pull_request) Successful in 0s

Details

2026-06-24 21:34:27 +02:00

Compare