From 64b3d747f014159a8fc56c0381825dbe805e2a3a Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sun, 9 Aug 2026 07:49:22 +0200 Subject: [PATCH] fix(snapshot): two false passes in the placeholder path, and cover its behaviour Both bugs made a snapshot assertion report success while comparing nothing, which is the failure a snapshot test exists to prevent. Without perl, the placeholder matcher fell back to grep. grep applies a pattern per line, and a pattern that itself contains newlines is read as several alternative patterns -- so a multi-line snapshot whose placeholder sat on its own line contributed a bare `.*`, which matches any line of any input. A snapshot of Report ::ignore:: Done matched the string "NOPE". The existing comment noted grep could not span lines; it did not notice that it matched everything instead. Now awk, with RS set to a byte the input cannot contain so the whole value is one record. Verified at parity with perl across all twelve behaviours. If neither perl nor awk exists the assertion fails and says so, rather than guessing. resolve_file prefixed "./" unconditionally, so an absolute test path became a path relative to the caller's cwd. The real snapshot was never read, a stray one was recorded somewhere else, and every snapshot assertion in that run passed. It is what made me disbelieve the first bug for an hour: my own probes were running against auto-recorded snapshots rather than the files I had written. Fourteen tests added. Half of them are negative, deliberately: a placeholder must narrow what is compared, never turn the assertion into one that always passes. They cover mid-line, start, end, empty region, several placeholders, a lone placeholder, multi-line spans, literal regex metacharacters, and the three rejection cases. All pass under both perl and awk. resolve_file takes an optional source override so a test can call it at any stack depth; BASH_SOURCE[2] is the test file only when reached through assert_match_snapshot. docs/snapshots.md now states what a placeholder does *not* relax, with a table of matching and non-matching values, the multi-line rule, why --snapshot-update skips a placeholder snapshot, and the perl/awk requirement. 1733 sequential / 1692 parallel; baseline + 14. --- CHANGELOG.md | 2 + docs/snapshots.md | 49 +++++++++++++- src/assert/snapshot.sh | 48 ++++++++++---- tests/unit/assert/snapshot_test.sh | 101 +++++++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7154ed16..3174615f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ - Internal: Split `src/runner.sh` and `src/coverage.sh` into focused modules with no behavior change; see [ADR-010](adrs/adr-010-src-module-directories.md) (#924, #925) ### Fixed +- Snapshot placeholders no longer report a false match without `perl`. The fallback matched line by line, so a multi-line snapshot whose placeholder sat on its own line contributed a bare `.*` that matched any input; it now uses `awk` over the whole value, and fails loudly if neither tool is available +- Snapshots resolve correctly when the test file is given as an absolute path. The path was always prefixed with `./`, so the real snapshot was never read and a stray one was recorded under the current directory, silently passing every snapshot assertion in the run - `assert_false` no longer passes when the command does not exist; exit codes 127 and 126 fail both boolean assertions, because the command never ran (#982) - `assert_have_been_called_times` and `assert_have_been_called_nth_with` report a usage error for a non-numeric count instead of leaking a raw `integer expression expected` (#984) - A failing test whose output quotes a shell-error phrase is no longer also reported as a runtime error (#992) diff --git a/docs/snapshots.md b/docs/snapshots.md index 73e1a253..542a371d 100644 --- a/docs/snapshots.md +++ b/docs/snapshots.md @@ -101,9 +101,11 @@ with ANSI escape sequences removed. ## Placeholders -Snapshot files can contain placeholder tokens to ignore variable parts of the output. -By default the token `::ignore::` will match any text. You can override it with the -`BASHUNIT_SNAPSHOT_PLACEHOLDER` environment variable. +Output that changes between runs — a timestamp, a PID, a temp path, a duration — would +make a snapshot fail every time. A placeholder marks the part you have decided not to +pin, so the rest still is. + +The default token is `::ignore::`; override it with `BASHUNIT_SNAPSHOT_PLACEHOLDER`. ```bash [Example] # snapshot file content @@ -113,6 +115,47 @@ echo 'Run at ::ignore::' > snapshots/example.snapshot assert_match_snapshot "Run at $(date)" ``` +### What a placeholder does and does not relax + +Everything outside the placeholder still has to match **exactly**. A placeholder is not +a wildcard for the whole snapshot: `a::ignore::b` matches `a123b`, and does *not* match +`XXX` or `aXXXc`. + +| Snapshot | Matches | Does not match | +|---|---|---| +| `a::ignore::b` | `a123b`, `ab` (an empty region matches) | `XXX`, `aXXXc` | +| `::ignore::tail` | `anythingtail` | `anythingelse` | +| `a=::ignore:: b=::ignore::` | `a=1 b=2` | `a=1 c=2` | +| `::ignore::` alone | any value at all | — | + +Text around a placeholder is compared literally, so regex metacharacters need no +escaping: a snapshot reading `cost $5.00 (x) ::ignore::` matches that exact prefix and +nothing else. + +A placeholder may span several lines. Given this snapshot: + +``` +Report +::ignore:: +Done +``` + +`Report\nany\nnumber\nof lines\nDone` matches, and unrelated output does not. + +### Placeholders and `--snapshot-update` + +`--snapshot-update` **skips** any snapshot containing a placeholder and says so. The +placeholder is a deliberate decision about what not to pin, and re-recording would +silently replace it with whatever this run happened to produce. Remove the placeholder +first if you really want the literal value. + +### Requirements + +Matching a placeholder needs `perl` or `awk`; one of them is present on essentially any +system that runs bash. If neither is, the assertion **fails** with a message saying so +rather than reporting a pass it could not verify. Snapshots without a placeholder are a +plain string comparison and need neither. + ## Finding unused snapshots A snapshot file is named after its test file and test function, so renaming or deleting a diff --git a/src/assert/snapshot.sh b/src/assert/snapshot.sh index 811cad2f..dadb680c 100644 --- a/src/assert/snapshot.sh +++ b/src/assert/snapshot.sh @@ -220,23 +220,32 @@ function bashunit::snapshot::match_with_placeholder() { local regex="^${escaped//$token/(.|\\n)*}$" if command -v perl >/dev/null 2>&1; then - echo "$actual" | REGEX="$regex" perl -0 -e ' + printf '%s' "$actual" | REGEX="$regex" perl -0 -e ' my $r = $ENV{REGEX}; my $input = join("", ); exit($input =~ /$r/s ? 0 : 1); ' && return 0 || return 1 - else - # No perl: build the pattern exactly like the perl branch — swap the - # placeholder for a token that survives escaping, escape the regex - # metacharacters, then turn the token into `.*`. (The previous order, - # escaping after inserting `.*`, escaped the `.*` itself and broke every - # fallback match.) grep matches line-by-line, so unlike the perl branch a - # placeholder cannot span multiple lines here. - local fallback="${snapshot//$placeholder/$token}" - fallback=$(printf '%s' "$fallback" | sed -e 's/[.[\\^$*+?{}()|]/\\&/g') - fallback="^${fallback//$token/.*}$" - echo "$actual" | grep -Eq "$fallback" && return 0 || return 1 fi + + # awk, not grep. grep applies the pattern per line, and a pattern that itself + # contains newlines is read as several alternative patterns -- so a multi-line + # snapshot whose placeholder sits on its own line contributed a bare `.*`, + # which matches any line of any input. That made an unrelated value pass while + # comparing nothing. Setting RS to a byte the input cannot contain gives awk + # the whole value as one record, so the anchors and the placeholder behave as + # they do under perl. + if bashunit::dependencies::has_awk; then + printf '%s' "$actual" | REGEX="$regex" awk ' + BEGIN { RS = "\001"; re = ENVIRON["REGEX"] } + { exit !($0 ~ re) } + ' && return 0 || return 1 + fi + + # Neither available: refuse rather than guess. A placeholder snapshot that + # cannot be evaluated must not report success. + printf '%sCannot match a snapshot placeholder: neither perl nor awk is available.%s\n' \ + "${_BASHUNIT_COLOR_FAILED:-}" "${_BASHUNIT_COLOR_DEFAULT:-}" >&2 + return 1 } # Writes the resolved snapshot path into _BASHUNIT_SNAPSHOT_FILE_OUT (no fork). @@ -255,7 +264,11 @@ function bashunit::snapshot::resolve_file() { # dirname via parameter expansion. `dirname "foo.sh"` (no slash) is ".", which # `${src%/*}` cannot yield, so special-case the slashless path. - local src="${BASH_SOURCE[2]}" + # + # $4 exists for the tests: BASH_SOURCE[2] is the test file only when this is + # reached through assert_match_snapshot, so a test calling it directly would + # otherwise resolve against the runner's own source. + local src="${4:-${BASH_SOURCE[2]}}" local dir_part case "$src" in */*) dir_part="${src%/*}" ;; @@ -272,7 +285,14 @@ function bashunit::snapshot::resolve_file() { name="$name.$_BASHUNIT_HELPER_VARNAME_OUT" fi - _BASHUNIT_SNAPSHOT_FILE_OUT="./${dir_part}/snapshots/${test_file}.${name}.snapshot" + # An absolute directory must stay absolute. Prefixing "./" turned + # "/abs/dir" into a path relative to the caller's cwd, so the real snapshot + # was never read and a stray one was recorded elsewhere -- every snapshot + # assertion in that run passed while comparing nothing. + case "$dir_part" in + /*) _BASHUNIT_SNAPSHOT_FILE_OUT="${dir_part}/snapshots/${test_file}.${name}.snapshot" ;; + *) _BASHUNIT_SNAPSHOT_FILE_OUT="./${dir_part}/snapshots/${test_file}.${name}.snapshot" ;; + esac } function bashunit::snapshot::initialize() { diff --git a/tests/unit/assert/snapshot_test.sh b/tests/unit/assert/snapshot_test.sh index 4187e52e..724a5093 100644 --- a/tests/unit/assert/snapshot_test.sh +++ b/tests/unit/assert/snapshot_test.sh @@ -262,3 +262,104 @@ function test_snapshot_resolve_file_uses_explicit_hint_verbatim() { assert_same "/tmp/custom.snapshot" "$_BASHUNIT_SNAPSHOT_FILE_OUT" } + +# resolve_file used to prefix "./" unconditionally, so an absolute test path +# became "./" + "/abs/dir" -- a path relative to the *current* directory. The +# real snapshot was never read and a stray one was recorded under the caller's +# cwd, so every snapshot assertion in that run passed while comparing nothing. +function test_resolve_file_keeps_an_absolute_directory_absolute() { + bashunit::snapshot::resolve_file "" "test_example" "" "/abs/dir/my_test.sh" + + assert_same "/abs/dir/snapshots/my_test_sh.test_example.snapshot" \ + "$_BASHUNIT_SNAPSHOT_FILE_OUT" +} + +function test_resolve_file_keeps_a_relative_directory_relative() { + bashunit::snapshot::resolve_file "" "test_example" "" "tests/unit/my_test.sh" + + assert_same "./tests/unit/snapshots/my_test_sh.test_example.snapshot" \ + "$_BASHUNIT_SNAPSHOT_FILE_OUT" +} + +# A path with no slash yields dir_part "." and therefore a doubled "./". That is +# the shape this function has always produced and the comment above it says so +# deliberately; it resolves identically, so it is pinned rather than tidied. +function test_resolve_file_handles_a_slashless_path() { + bashunit::snapshot::resolve_file "" "test_example" "" "my_test.sh" + + assert_same "././snapshots/my_test_sh.test_example.snapshot" \ + "$_BASHUNIT_SNAPSHOT_FILE_OUT" +} + +function test_resolve_file_includes_the_snapshot_name_when_given() { + bashunit::snapshot::resolve_file "" "test_example" "stderr" "tests/my_test.sh" + + assert_same "./tests/snapshots/my_test_sh.test_example.stderr.snapshot" \ + "$_BASHUNIT_SNAPSHOT_FILE_OUT" +} + +# The placeholder marks a region of the snapshot the author chose not to pin. +# These cover the whole surface, and deliberately include the negative cases: +# a placeholder must not turn the assertion into one that always passes. +function snapshot_with() { # $1 snapshot content -> prints the path + local path="$(bashunit::temp_dir)/placeholder.snapshot" + printf '%s' "$1" >"$path" + printf '%s' "$path" +} + +function test_placeholder_matches_a_varying_region_mid_line() { + assert_empty "$(assert_match_snapshot "a123b" "$(snapshot_with 'a::ignore::b')")" +} + +function test_placeholder_matches_at_the_start_and_end() { + assert_empty "$(assert_match_snapshot "anythingtail" "$(snapshot_with '::ignore::tail')")" + assert_empty "$(assert_match_snapshot "headanything" "$(snapshot_with 'head::ignore::')")" +} + +function test_placeholder_matches_an_empty_region() { + assert_empty "$(assert_match_snapshot "ab" "$(snapshot_with 'a::ignore::b')")" +} + +function test_several_placeholders_in_one_snapshot() { + assert_empty "$(assert_match_snapshot "a=1 b=2" "$(snapshot_with 'a=::ignore:: b=::ignore::')")" +} + +function test_a_lone_placeholder_matches_any_value() { + assert_empty "$(assert_match_snapshot "literally anything" "$(snapshot_with '::ignore::')")" +} + +function test_placeholder_spans_multiple_lines() { + local snapshot actual + snapshot=$(snapshot_with "$(printf 'A\n::ignore::\nZ')") + actual=$(printf 'A\nq\nw\nZ') + + assert_empty "$(assert_match_snapshot "$actual" "$snapshot")" +} + +# The important half. Without these a placeholder could silently become +# "match anything" and the assertion would report success while comparing +# nothing -- which is exactly what the pre-awk fallback did for the multi-line +# case. +function test_placeholder_still_requires_the_surrounding_text() { + assert_not_empty "$(assert_match_snapshot "XXX" "$(snapshot_with 'a::ignore::b')")" +} + +function test_placeholder_rejects_a_differing_suffix() { + assert_not_empty "$(assert_match_snapshot "aXXXc" "$(snapshot_with 'a::ignore::b')")" +} + +function test_multi_line_placeholder_rejects_unrelated_output() { + local snapshot + snapshot=$(snapshot_with "$(printf 'A\n::ignore::\nZ')") + + assert_not_empty "$(assert_match_snapshot "NOPE" "$snapshot")" +} + +# Regex metacharacters in the snapshot are literal text, not pattern syntax. +function test_regex_metacharacters_around_a_placeholder_are_literal() { + local snapshot + snapshot=$(snapshot_with 'cost $5.00 (x) ::ignore::') + + assert_empty "$(assert_match_snapshot 'cost $5.00 (x) Z' "$snapshot")" + assert_not_empty "$(assert_match_snapshot 'cost 999 (x) Z' "$snapshot")" +}