Skip to content

Commit e55ce99

Browse files
max-sixtyclaude
andauthored
Fix backward compatibility for legacy inline snapshot format (#830)
## Summary This fixes a regression introduced in 1.44.0 where existing inline snapshots using the legacy multiline format would fail to match. **The problem:** PR #563 changed how inline snapshots handle leading newlines. This caused snapshots like: ```rust insta::assert_snapshot!(value, @r" Unconflicted Mode(FILE) 0839b2e9412b "); ``` to fail when the actual value is `"Unconflicted Mode(FILE) 0839b2e9412b"`. The legacy format stored single-line content in a multiline raw string with source code indentation. After `from_inline_literal` processing, this becomes `" Unconflicted...\n "` — the leading spaces from code indentation weren't being stripped in the legacy comparison path. **The fix:** In `matches_legacy`, detect the legacy single-line-in-multiline pattern: ```rust let is_legacy_single_line_in_multiline = sc.contents.contains('\n') && sc.contents.trim_end().lines().count() <= 1; ``` When this pattern is detected, apply `trim_start()` to strip the source code indentation artifacts. This ensures: - ✅ Legacy snapshots continue to pass (with a warning to update) - ✅ Modern single-line snapshots with intentional leading spaces are preserved - ✅ True multiline snapshots are unaffected ## Test plan - [x] Added 8 comprehensive functional tests covering: - Single-line content in multiline format (the jj bug) - Raw string multiline variant - Force update reformatting works - True multiline content unaffected - Multiline with relative indentation preserved - Intentional leading spaces correctly fail (no false positives) - Trailing whitespace line edge case - Empty snapshot edge case - [x] All 72 functional tests pass - [x] Full test suite passes Reported-by: jj-vcs/jj See: #819 (comment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
1 parent d44dd42 commit e55ce99

File tree

3 files changed

+409
-1
lines changed

3 files changed

+409
-1
lines changed
Lines changed: 383 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,383 @@
1+
use insta::assert_snapshot;
2+
use std::process::Stdio;
3+
4+
use crate::TestFiles;
5+
6+
// Tests for backwards compatibility with older snapshot formats.
7+
//
8+
// These tests verify that upgrading insta doesn't cause existing valid
9+
// snapshots to fail. Legacy formats should still match via matches_legacy.
10+
//
11+
// See: https://github.com/mitsuhiko/insta/pull/819#issuecomment-3583709431
12+
13+
/// Test that single-line content stored in multiline format still passes.
14+
///
15+
/// This reproduces the issue reported at:
16+
/// https://github.com/jj-vcs/jj/commit/2f0132a765518a8df705fd00e10dcc05862c3799
17+
#[test]
18+
fn test_no_reformat_single_line_in_multiline_format() {
19+
let test_project = TestFiles::new()
20+
.add_cargo_toml("no_reformat_multiline")
21+
.add_file(
22+
"src/lib.rs",
23+
// This is the old format - single-line content in multiline format
24+
// (as seen in jj before the upgrade)
25+
r#####"
26+
#[test]
27+
fn test_single_line_in_multiline() {
28+
insta::assert_snapshot!(get_status(), @r"
29+
Unconflicted Mode(FILE) 0839b2e9412b ctime=0:0 mtime=0:0 size=0 flags=0 file1.txt
30+
");
31+
}
32+
33+
fn get_status() -> &'static str {
34+
"Unconflicted Mode(FILE) 0839b2e9412b ctime=0:0 mtime=0:0 size=0 flags=0 file1.txt"
35+
}
36+
"#####
37+
.to_string(),
38+
)
39+
.create_project();
40+
41+
// Run tests with --check - should pass (legacy format still matches)
42+
// If this fails, it means insta incorrectly rejects the legacy format
43+
let output = test_project
44+
.insta_cmd()
45+
.args(["test", "--check", "--", "--nocapture"])
46+
.stderr(Stdio::piped())
47+
.stdout(Stdio::piped())
48+
.output()
49+
.unwrap();
50+
51+
let stderr = String::from_utf8_lossy(&output.stderr);
52+
let stdout = String::from_utf8_lossy(&output.stdout);
53+
54+
assert!(
55+
output.status.success(),
56+
"Test should pass with --check (legacy format matches). Stderr: {stderr}"
57+
);
58+
59+
// Should show legacy format warning
60+
let combined = format!("{stderr}\n{stdout}");
61+
assert!(
62+
combined.contains("existing value is in a legacy format"),
63+
"Should show legacy format warning. Output: {combined}"
64+
);
65+
66+
// The file should NOT be modified (tests don't modify files)
67+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
68+
}
69+
70+
/// Similar test with the UU file example from jj
71+
#[test]
72+
fn test_no_reformat_raw_string_multiline() {
73+
let test_project = TestFiles::new()
74+
.add_cargo_toml("no_reformat_raw_string")
75+
.add_file(
76+
"src/lib.rs",
77+
r#####"
78+
#[test]
79+
fn test_uu_file() {
80+
insta::assert_snapshot!(get_output(), @r#"
81+
UU file
82+
"#);
83+
}
84+
85+
fn get_output() -> &'static str {
86+
"UU file"
87+
}
88+
"#####
89+
.to_string(),
90+
)
91+
.create_project();
92+
93+
// Run tests with --check - should pass (legacy format still matches)
94+
let output = test_project
95+
.insta_cmd()
96+
.args(["test", "--check", "--", "--nocapture"])
97+
.stderr(Stdio::piped())
98+
.output()
99+
.unwrap();
100+
101+
assert!(
102+
output.status.success(),
103+
"Test should pass with --check (legacy format matches). Stderr: {}",
104+
String::from_utf8_lossy(&output.stderr)
105+
);
106+
107+
// The file should NOT be modified (tests don't modify files)
108+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
109+
}
110+
111+
/// Test that --force-update-snapshots DOES reformat to canonical form
112+
#[test]
113+
fn test_force_update_does_reformat() {
114+
let test_project = TestFiles::new()
115+
.add_cargo_toml("force_update_reformats")
116+
.add_file(
117+
"src/lib.rs",
118+
r#####"
119+
#[test]
120+
fn test_single_line_in_multiline() {
121+
insta::assert_snapshot!(get_status(), @r"
122+
single line content
123+
");
124+
}
125+
126+
fn get_status() -> &'static str {
127+
"single line content"
128+
}
129+
"#####
130+
.to_string(),
131+
)
132+
.create_project();
133+
134+
// Run tests WITH --force-update-snapshots
135+
let output = test_project
136+
.insta_cmd()
137+
.args(["test", "--force-update-snapshots", "--", "--nocapture"])
138+
.stderr(Stdio::piped())
139+
.output()
140+
.unwrap();
141+
142+
assert!(
143+
output.status.success(),
144+
"Test should pass: {}",
145+
String::from_utf8_lossy(&output.stderr)
146+
);
147+
148+
// With --force-update-snapshots, the file SHOULD be reformatted to canonical form
149+
assert_snapshot!(test_project.diff("src/lib.rs"), @r#"
150+
--- Original: src/lib.rs
151+
+++ Updated: src/lib.rs
152+
@@ -1,9 +1,7 @@
153+
154+
#[test]
155+
fn test_single_line_in_multiline() {
156+
- insta::assert_snapshot!(get_status(), @r"
157+
- single line content
158+
- ");
159+
+ insta::assert_snapshot!(get_status(), @"single line content");
160+
}
161+
162+
fn get_status() -> &'static str {
163+
"#);
164+
}
165+
166+
/// Test that actual multiline content in legacy format works correctly.
167+
/// normalize_inline handles multiline correctly, so matches_latest succeeds
168+
/// and there's no legacy fallback needed (no warning shown).
169+
#[test]
170+
fn test_no_reformat_true_multiline_legacy() {
171+
let test_project = TestFiles::new()
172+
.add_cargo_toml("true_multiline_legacy")
173+
.add_file(
174+
"src/lib.rs",
175+
r#####"
176+
#[test]
177+
fn test_multiline() {
178+
insta::assert_snapshot!(get_output(), @r"
179+
line1
180+
line2
181+
");
182+
}
183+
184+
fn get_output() -> &'static str {
185+
"line1\nline2"
186+
}
187+
"#####
188+
.to_string(),
189+
)
190+
.create_project();
191+
192+
// Run tests with --check - should pass without needing any changes
193+
let output = test_project
194+
.insta_cmd()
195+
.args(["test", "--check", "--", "--nocapture"])
196+
.stderr(Stdio::piped())
197+
.stdout(Stdio::piped())
198+
.output()
199+
.unwrap();
200+
201+
assert!(
202+
output.status.success(),
203+
"Test should pass with --check. Stderr: {}",
204+
String::from_utf8_lossy(&output.stderr)
205+
);
206+
207+
// Note: No legacy warning expected here because normalize_inline correctly
208+
// handles true multiline content. The legacy warning only appears when
209+
// matches_latest fails but matches_legacy passes.
210+
211+
// The file should NOT be modified (tests don't modify files)
212+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
213+
}
214+
215+
/// Test multiline content with relative indentation is preserved.
216+
/// The first line has extra indentation that should be kept.
217+
#[test]
218+
fn test_multiline_with_relative_indentation() {
219+
let test_project = TestFiles::new()
220+
.add_cargo_toml("relative_indent")
221+
.add_file(
222+
"src/lib.rs",
223+
r#####"
224+
#[test]
225+
fn test_indented_code() {
226+
insta::assert_snapshot!(get_output(), @r"
227+
if condition:
228+
return value
229+
");
230+
}
231+
232+
fn get_output() -> &'static str {
233+
"if condition:\n return value"
234+
}
235+
"#####
236+
.to_string(),
237+
)
238+
.create_project();
239+
240+
// Run tests with --check - should pass
241+
let output = test_project
242+
.insta_cmd()
243+
.args(["test", "--check", "--", "--nocapture"])
244+
.stderr(Stdio::piped())
245+
.stdout(Stdio::piped())
246+
.output()
247+
.unwrap();
248+
249+
assert!(
250+
output.status.success(),
251+
"Test should pass - relative indentation preserved. Stderr: {}",
252+
String::from_utf8_lossy(&output.stderr)
253+
);
254+
255+
// The file should NOT be modified (tests don't modify files)
256+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
257+
}
258+
259+
/// Test that intentional leading whitespace in snapshots doesn't get stripped.
260+
/// This is an edge case where someone INTENTIONALLY has leading spaces in their snapshot.
261+
/// The test should FAIL because the actual value doesn't have those spaces.
262+
#[test]
263+
fn test_intentional_leading_spaces_should_fail() {
264+
let test_project = TestFiles::new()
265+
.add_cargo_toml("intentional_spaces")
266+
.add_file(
267+
"src/lib.rs",
268+
r#####"
269+
#[test]
270+
fn test_with_leading_spaces() {
271+
// The snapshot has intentional leading spaces, but the actual value doesn't
272+
insta::assert_snapshot!(get_content(), @" content with leading spaces");
273+
}
274+
275+
fn get_content() -> &'static str {
276+
"content with leading spaces"
277+
}
278+
"#####
279+
.to_string(),
280+
)
281+
.create_project();
282+
283+
// Run tests with --check - should FAIL because values don't match
284+
let output = test_project
285+
.insta_cmd()
286+
.args(["test", "--check", "--", "--nocapture"])
287+
.stderr(Stdio::piped())
288+
.output()
289+
.unwrap();
290+
291+
// This should FAIL - the intentional leading spaces should NOT be stripped
292+
assert!(
293+
!output.status.success(),
294+
"Test should FAIL when snapshot has intentional leading spaces that don't match actual. \
295+
If this passes, it means matches_legacy is too permissive."
296+
);
297+
}
298+
299+
/// Test that content with trailing whitespace line works correctly.
300+
/// Even though the heuristic triggers (has \n, 1 line after trim), the content
301+
/// has no leading whitespace so trim_start() is a no-op.
302+
#[test]
303+
fn test_trailing_whitespace_line() {
304+
let test_project = TestFiles::new()
305+
.add_cargo_toml("trailing_ws")
306+
.add_file(
307+
"src/lib.rs",
308+
r#####"
309+
#[test]
310+
fn test_trailing() {
311+
insta::assert_snapshot!(get_content(), @r"
312+
content
313+
");
314+
}
315+
316+
fn get_content() -> &'static str {
317+
"content"
318+
}
319+
"#####
320+
.to_string(),
321+
)
322+
.create_project();
323+
324+
// Run tests with --check - should pass
325+
let output = test_project
326+
.insta_cmd()
327+
.args(["test", "--check", "--", "--nocapture"])
328+
.stderr(Stdio::piped())
329+
.stdout(Stdio::piped())
330+
.output()
331+
.unwrap();
332+
333+
assert!(
334+
output.status.success(),
335+
"Content with trailing whitespace line should pass. Stderr: {}",
336+
String::from_utf8_lossy(&output.stderr)
337+
);
338+
339+
// The file should NOT be modified
340+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
341+
}
342+
343+
/// Test that empty snapshots in multiline format work correctly.
344+
/// This is an edge case where the snapshot is just whitespace/newlines.
345+
#[test]
346+
fn test_empty_snapshot_in_multiline_format() {
347+
let test_project = TestFiles::new()
348+
.add_cargo_toml("empty_multiline")
349+
.add_file(
350+
"src/lib.rs",
351+
r#####"
352+
#[test]
353+
fn test_empty() {
354+
insta::assert_snapshot!(get_empty(), @r"
355+
");
356+
}
357+
358+
fn get_empty() -> &'static str {
359+
""
360+
}
361+
"#####
362+
.to_string(),
363+
)
364+
.create_project();
365+
366+
// Run tests with --check - should pass
367+
let output = test_project
368+
.insta_cmd()
369+
.args(["test", "--check", "--", "--nocapture"])
370+
.stderr(Stdio::piped())
371+
.stdout(Stdio::piped())
372+
.output()
373+
.unwrap();
374+
375+
assert!(
376+
output.status.success(),
377+
"Empty snapshot should pass. Stderr: {}",
378+
String::from_utf8_lossy(&output.stderr)
379+
);
380+
381+
// The file should NOT be modified (tests don't modify files)
382+
assert_snapshot!(test_project.diff("src/lib.rs"), @"");
383+
}

cargo-insta/tests/functional/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ use itertools::Itertools;
6868
use similar::udiff::unified_diff;
6969
use tempfile::TempDir;
7070

71+
mod back_compat;
7172
mod binary;
7273
mod delete_pending;
7374
mod glob_filter;

0 commit comments

Comments
 (0)