insta icon indicating copy to clipboard operation
insta copied to clipboard

Wrong function name in snapshot when insta::assert_*_snapshot is in a other function

Open la10736 opened this issue 2 years ago • 10 comments

What happened?

Let this code example:

use insta::assert_yaml_snapshot;

mod test_option_value {
    use super::*;
    fn assert(v: u32) {
        assert_yaml_snapshot!(v);
    }

    #[test]
    fn value_01_1() {
        assert(1);
    }

    #[test]
    fn value_02_2() {
        assert(2);
    }

    #[test]
    fn value_03_3() {
        assert(3);
    }

    #[test]
    fn value_04_4() {
        assert(4);
    }

    #[test]
    fn value_05_5() {
        assert(5);
    }

    #[test]
    fn value_06_6() {
        assert(6);
    }

    #[test]
    fn value_07_7() {
        assert(7);
    }

    #[test]
    fn value_08_8() {
        assert(8);
    }

    #[test]
    fn value_09_9() {
        assert(9);
    }

    #[test]
    fn value_10_10() {
        assert(10);
    }
}

In this case all files will have the same name. This is due to _function_name macro implementation that assume it should be used in only in the #[test] function.

I know that is not a good practice call a function that do the assert but this issue is raised up when somebody tried to integrate insta with rstest https://github.com/la10736/rstest/issues/183

What about of using something like thread::current().name().unwrap().to_string() to get the current name? that's the one cargo test use to log the test and so should be always the correct one.

Reproduction steps

use insta::assert_yaml_snapshot;

mod test_option_value { use super::*; fn assert(v: u32) { assert_yaml_snapshot!(v); }

#[test]
fn value_01_1() {
    assert(1);
}

#[test]
fn value_02_2() {
    assert(2);
}

#[test]
fn value_03_3() {
    assert(3);
}

#[test]
fn value_04_4() {
    assert(4);
}

#[test]
fn value_05_5() {
    assert(5);
}

#[test]
fn value_06_6() {
    assert(6);
}

#[test]
fn value_07_7() {
    assert(7);
}

#[test]
fn value_08_8() {
    assert(8);
}

#[test]
fn value_09_9() {
    assert(9);
}

#[test]
fn value_10_10() {
    assert(10);
}

}

2. run test that use the previous created snapshots


### Insta Version

_No response_

### rustc Version

_No response_

### What did you expect?

That generate the shapshot file that take the function names like  `value_08_8` and not `test_insta__test_option_value__assert.new`

la10736 avatar Mar 06 '23 13:03 la10736

Unfortunately the thread name does not work. That's what insta did originally but sadly it breaks in some scenarios and I had to change this. While one of the major blockers was resolved, the max length limit still applies on linux and you can trivially run into it. For some history see https://github.com/mitsuhiko/insta/issues/105

mitsuhiko avatar Mar 08 '23 22:03 mitsuhiko

Ok, I can see your point. But I cannot understand the issue related to the max length. Now the fix for single thread test is fixed on stable also and follow code works non my linux

mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
    mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
        mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
            mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
                mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
                    mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
                        mod aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {
                            #[test]
                            fn feature() {
                                let path = std::thread::current().name().unwrap().to_owned();
                                let name = path.rsplit_once("::").unwrap().1;
                                assert_eq!("feature", name)
                            }
                        }
                    }
                }
            }
        }
    }
}
mdamico@miklap:~/dev_random/thread_name_test$ cargo test --release -- --test-threads=1
    Finished release [optimized] target(s) in 0.00s
     Running unittests src/main.rs (target/release/deps/thread_name_test-0ec132d327548f97)

running 1 test
test aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::feature ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

la10736 avatar Mar 09 '23 08:03 la10736

Anyway insta autoname have some limits and (maybe in order to take some simplicity to take just a folder for fixture files) it can simply fall in some issue when you have the same test name in different modules (contract tests falls frequently in this case). In this case is better to use the macro variants that take the filename also.

I think that to make work the integration between rstest and insta is a good goal (they are two of the most widely used test framework in rust).... so I'll provide some documentation of how to use it.

Thx

la10736 avatar Mar 09 '23 08:03 la10736

I'm not sure under which circumstances one runs into the thread name limit. It's technically 16 or 24 characters (TASK_COMM_LEN) but I assume that Rust might now have a workaround for it? I can revisit this again, but in general I quite like now that the name is based on the name of the function. If rstest could expose the executed function name somehow, then a feature could be added to insta to pick up the test name from rstest.

mitsuhiko avatar Mar 09 '23 08:03 mitsuhiko

Bumping into this too.

DanielJoyce avatar Mar 27 '24 23:03 DanielJoyce

@DanielJoyce can you show an example of how this issue affects you?

mitsuhiko avatar Mar 28 '24 10:03 mitsuhiko

Sure,

The issue seems to be using insta with rstest and the test case feature to parametrize the tests. The tests and snapshots would pass on my dev box, but failed in CI as the test case execution order seemed to be swapped.

Explicitly naming the tests fixed the issue

local dev is using rust 1.71, CI, I don't know the rust verision.

Test is something like this

#[rstest]
#[case("/path/to/some/file.yml")]
#[case("/path/to/some/file2.yml")]
fn do_something_with_yaml(#[case] file: string) {
    /// Set up a file reader to read from the file
   ....
    /// Both of these structs impl Serialize, one is for reading a file, the other is the canonical datastructure 
    /// used by the library. I just want to snapshot a few things so unintended struct changes cause build failures.
    let some_data_structure = serde_yaml::from_reader(file_reader).unwrap();
    let some_other_structure = some_data_structure.try_into().unwrap();
    /// This would fail in CI, where the snapshot file checked in on dev  would be compared with the wrong test
    assert_yaml_snapshot!(some_other_structure);
    /// This works, explicitly naming the snapshot
    assert_yaml_snapshot!(file, some_other_structure);
}

I don't think insta quite captures enough data to distinguish these two cases reliably, especially when different rust compiler versions are involved, which can lead to field/function reordering.

DanielJoyce avatar Mar 31 '24 17:03 DanielJoyce

the way rstest expands test cases basically breaks the invariants that insta auto-name relies on.

DanielJoyce avatar Mar 31 '24 17:03 DanielJoyce

This may just need to be a documentation thing.

DanielJoyce avatar Mar 31 '24 17:03 DanielJoyce

This is documented but I guess not well enough. The docs have a snippet for how to make this work best with rstest: https://insta.rs/docs/patterns/

TLDR:

macro_rules! set_snapshot_suffix {
    ($($expr:expr),*) => {
        let mut settings = insta::Settings::clone_current();
        settings.set_snapshot_suffix(format!($($expr,)*));
        let _guard = settings.bind_to_scope();
    }
}

#[rstest]
#[case(0, 2)]
#[case(2, 4)]
fn test_it(#[case] a: usize, #[case] b: usize) {
    set_snapshot_suffix!("{}-{}", a, b);
    insta::assert_debug_snapshot!(a * b);
}

mitsuhiko avatar Apr 02 '24 11:04 mitsuhiko