Multiple assert_snapshot within a allow_duplicates doesn't work well

[ilyagr]

What happened?

I think there might be several problems here

Reproduction steps

1. Clone jj-vcs/jj@35440ce
2. Observe that cargo nextest run passes
3. Run cargo insta test --workspace --test-runner nextest --force-update-snapshots
4. Try cargo nextest run again

Expected results:

tests pass

Actual result:

Compilation fails

error: bare CR not allowed in string, use `\r` instead
   --> cli/src/git_util.rs:820:74
    |
820 |         assert_snapshot!(update(Duration::from_millis(1), 0.10), @"␛[?25l␍ 10% [█▊                ]␛[K...
    |                                                                          ^
    |
help: escape the character
    |
820 |         assert_snapshot!(update(Duration::from_millis(1), 0.10), @"␛[?25l\r 10% [█▊                ]␛[K");
    |                                                                          ++

error: could not compile `jj-cli` (lib) due to 1 previous error

Furthermore, these \r-s are not the only problem. If that one diff is reverted (\r replaced with ␍again) in jj-vcs/jj#5441, there are more compilation errors, see https://github.com/jj-vcs/jj/actions/runs/12934546007/job/36075816189?pr=5441 . For example,

error: expected one of `.`, `;`, `?`, `}`, or an operator, found `""`
   --> cli/tests/test_git_fetch.rs:334:10
    |
334 |     ");""""
    |          ^^ expected one of `.`, `;`, `?`, `}`, or an operator

Insta Version

1.42.0, same behavior with 1.41.1

rustc Version

rustc 1.86.0-nightly (48a426eca 2025-01-12)

What did you expect?

Tests to pass.

[max-sixty]

Thanks, this seems important.

Do you know whether this replicates without nextest? Has it been an issue for a while or are there some new test outputs in jj?

OK, I looked at the PR — I think the problem i that we're deciding how many hashes to use depending on whether there are linebreaks, but failing to consider a bare \r, which is possibly required. Does that sound correct?

[ilyagr]

I just started investigating, I think there are at least two problems (as seen by the two error messages).

Running insta without nextest gives the same result.

I randomly tried running insta on jj's commit dddba46 from 6 months ago, and there were no problems.

I tried a commit from earlier today, and got the same error. I might try bisecting to find when the commit that insta can't handle happened, it's probably when the relevant tests were last updated.

[max-sixty]

(I edited my message above a minute after sending)

No need to bisect — I know why it's doing this. We previously always used ###; now we decide how many to add (which can be "none".) The new approach is better since having ### in the snapshot used to cause errors, but the implementation likely misses a couple of characters.

[max-sixty]

#713 goes some way to fixing it; possibly not all of it. If you have an idle moment, give it a try; otherwise I can look later

[max-sixty]

I don't think #713 is complete fyi, will come back to this later (or welcome any contribution if you have an idea, but no obligation)

[ilyagr]

No, #713 does not resolve either problem. I didn't look into why yet.

[ilyagr]

I did narrow down the error to the following files in the jj repo.

M cli/src/git_util.rs
M cli/tests/test_git_fetch.rs
M cli/tests/test_git_init.rs
M cli/tests/test_git_push.rs

So, you can run the following, slightly faster command, to observe a few different errors.

cargo insta test  --workspace --test-runner nextest \
  --force-update-snapshots -- git_util test_git_fetch test_git_init test_git_push

[ilyagr]

I think there are at least two problems:

• Something about quoting \r.
• Some bad interaction between allow_duplicates! and --force-update-snapshots. When both are used, insta seems to make up snapshot output sometimes, seemingly copying it from the previous snapshot.

[max-sixty]

OK, I think the new version of #713 fixes the first; will be offline for a bit but can look more later

[ilyagr]

Yes, I think that new version of #713 helped. 🎉

For the second problem, @martinvonz pointed out that it's probably related to jj-vcs/jj#5314 and having more than one insta_snapshot! inside allow_duplicates!. So, there's probably a workaround.

[max-sixty]

OK great! I'll change the title if that's OK!

[yuja]

There are two problems:

1. single-line snapshot isn't deduplicated
2. the last assertion in the allow_duplicates! block is always chosen

#[test]
fn test_dup() {
    for _ in 0..2 {
        insta::allow_duplicates! {
            insta::assert_snapshot!("foo", @"");
        }
    }
}

#[test]
fn test_bad_location() {
    for _ in 0..2 {
        insta::allow_duplicates! {
            insta::assert_snapshot!("1", @"
            1a
            1b
            ");
            insta::assert_snapshot!("2", @"
            2a
            2b
            ");
        }
    }
}

I'll send fixes shortly.

[ilyagr]

Thanks, I guess I'll leave it to you then. I have some tests in the commit linked from this issue above, if they are helpful. Though, I think some fixes to functional testing infra might be needed to make the result of running cargo insta test on them determenistic.