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

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

Open
la10736 opened this issue Mar 6, 2023 · 10 comments
Labels
bug Something isn't working

Comments

@la10736
Copy link

la10736 commented Mar 6, 2023

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 la10736/rstest#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 la10736 added the bug Something isn't working label Mar 6, 2023
@mitsuhiko
Copy link
Owner

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 #105

@la10736
Copy link
Author

la10736 commented Mar 9, 2023

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
Copy link
Author

la10736 commented Mar 9, 2023

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

@mitsuhiko
Copy link
Owner

mitsuhiko commented Mar 9, 2023

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.

@DanielJoyce
Copy link

Bumping into this too.

@mitsuhiko
Copy link
Owner

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

@DanielJoyce
Copy link

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
Copy link

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

@DanielJoyce
Copy link

This may just need to be a documentation thing.

@mitsuhiko
Copy link
Owner

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);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants