Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inline snapshot handling within allow_duplicates! block #722

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Feb 12, 2025

#712

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! {
            // shouldn't be updated twice
            insta::assert_snapshot!("foo", @"");
        }
    }
}
#[test]
fn test_bad_location() {
    for _ in 0..2 {
        insta::allow_duplicates! {
            // each snapshot should be updated accordingly
            insta::assert_snapshot!("1", @"
            1a
            1b
            ");
            insta::assert_snapshot!("2", @"
            2a
            2b
            ");
        }
    }
}

@ilyagr
Copy link

ilyagr commented Feb 12, 2025

Passes all of my tests, nice!

yuja added 2 commits February 13, 2025 12:51
This is a follow up for 7b5a7f8 "Fix a patcher panic (mitsuhiko#341)." Since snapshots
are identified by line number, multiple snapshots located at the same line
should be omitted.
Before, the last assertion within the allow_duplicates! { .. } block was always
chosen. This patch makes visitor parse block-like macro as a plain code block
and visit the inner statements recursively.

Maybe scan_nested_macros() could be removed or cleaned up, but it's still called
from visit_attribute(). I have no idea what's supposed to be done here.

Fixes mitsuhiko#712
@yuja yuja force-pushed the push-lyrrpzmpsmxn branch from 89dcd53 to b218ed7 Compare February 13, 2025 03:52
@max-sixty
Copy link
Collaborator

This looks very good, thank you @yuja !

@max-sixty max-sixty merged commit 04a98c3 into mitsuhiko:master Feb 13, 2025
15 checks passed
@yuja yuja deleted the push-lyrrpzmpsmxn branch February 13, 2025 04:26
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.

3 participants