Skip to content

Ignore whitespace-only lines in inline snapshot indentation - #949

Open
Shaisolaris wants to merge 1 commit into
mitsuhiko:masterfrom
Shaisolaris:fix-inline-whitespace-only-indent
Open

Shaisolaris wants to merge 1 commit into
mitsuhiko:masterfrom
Shaisolaris:fix-inline-whitespace-only-indent

Conversation

@Shaisolaris

Copy link
Copy Markdown

A value with a whitespace-only line that is less indented than the rest fails against its own accepted inline snapshot.

let value = TextSnapshotContents::new(" \n    b".to_string(), TextSnapshotKind::Inline);
let literal = value.to_inline("    ");
// reparse with from_inline_literal and compare: no match

min_indentation skips empty lines but counts whitespace-only lines. For " \n b" it picks one space, so the normalized value is "\n b". The whitespace-only line is now empty, so normalizing the written literal picks four spaces and gives "\nb". The two never compare equal, so accepting the snapshot does not make the test pass.

This makes min_indentation ignore lines that contain only spaces and tabs, so normalization gives the same result on the second pass. Lines shorter than the indentation already become empty in normalize_inline.

Testing:

  • Two new unit tests (test_min_indentation_ignores_whitespace_only_lines and test_inline_snapshot_whitespace_only_line_roundtrip) fail before the change and pass after.
  • cargo test -p insta with default, --all-features, --no-default-features and --features redactions, plus cargo test -p cargo-insta, all pass locally.
  • A local round-trip fuzz (value, to_inline, from_inline_literal, normalize; about 200k generated values, escaped format excluded) went from 3172 mismatches to 0. The fuzz is not part of this change.

@max-sixty

Copy link
Copy Markdown
Collaborator

I think you're right that there are a couple of values that violate the invariant that every accepted value also passes a test.

but I think this does too much. (tables from LLM):

Acceptance bug on the current default branch

Actual output from the test Normalized actual output Accepted snapshot after reloading and normalization
" \n b" "\n b" "\nb"
" a\n \n b" " a\n\n b" "a\n\nb"

Acceptance succeeds, but rerunning fails because the last two columns differ.

Comparison that newly passes with #949

Actual output from the test Normalized actual output today Normalized actual output with #949 Reference snapshot after normalization
" \n x" "\n x" "\nx" "\nx"

i.e. if the final column there is the reference snapshot, we would want a failure.


I'd need to spend more time thinking whether this is possible to do both: I agree the invariant seems more important; but also we can't reverse a change once it's allowed; as the trail of deprecations demonstrates

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.

2 participants