rstest does not ensure the execution order of cases

[ChAoSUnItY]

As titled, rstest seems unable to ensure definite execution orders of cases, this would be a serious case when other testing framework comes in, such as insta, which would compares current snapshot named with current testing function with previous one.

When there are more than 1 cases, it would possibly messed up test case functions' name (order number) and caused further unexpected issues.

[la10736]

Can you write a simple example of how this become an issue? The test names are deterministic but the execution not due cargo test implementation that spawn a test for each case. Il sab 4 mar 2023, 10:34 ChAoS_UnItY ***@***.***> ha scritto:

…

As titled, rstest seems unable to ensure definite execution orders of cases, this would be a serious case when other testing framework comes in, such as insta <https://crates.io/crates/insta>, which would compares current snapshot named with current testing function with previous one. When there are more than 1 cases, it would possibly messed up test case functions' name (order number) and caused further unexpected issues. — Reply to this email directly, view it on GitHub <#183>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5Y34IBUHW4NK422ICXVRTW2MEDNANCNFSM6AAAAAAVPOJD3Q> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ChAoSUnItY]

Ok, let's consider we have the following code for testing:

use insta::assert_yaml_snapshot;
use rstest::rstest;

#[rstest]
#[case(Some(()))]
#[case(None)]
fn test_option_value(#[case] value: Option<()>) {
    assert_yaml_snapshot!(value);
}

Running this test, I would expect that there'll be 2 snapshot files, which are test_option_value.new and test_option_value-2.new, and the content will follow the definition of cases: test_option_value.new would contain test result of #[case(Some(()))], and test_option_value-2.new would contain test result of #[case(None)].

But after running tests for multiple times, it sometimes appears that their result contents are swapped (mainly due to execution order which effects test case name), and this would causes test to fail due to content mismatched.

[la10736]

Follow code expose the issue almost every time. Seams that insta extract the wrong function's name: all snapshot have the same name even if the cargo test show that are all different.

I'll go deaply later

use insta::assert_yaml_snapshot;
use rstest::rstest;

#[rstest]
fn test_option_value(#[values(1,2,3,4,5,6,7,8,9,10)] value: u32) {
    assert_yaml_snapshot!(value);
}

[la10736]

Ok, the issue seams in insta: the _function_name!() macro doesn't capture the correct name.

Follow code that not use rstest fails in the same way:

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

Maybe is better replace _function_name with something that use thread::current().name().unwrap().to_string() like in

rstest/rstest_test/src/utils.rs

Line 349 in 23b5311

pub fn testname() -> String {

to be more coherent with cargo test implementation.

[alexpovel]

One solution I've found to this (in the context of this issue, maybe counts more as a workaround...) is to use named snapshots, where the docs give an example like:

#[test]
fn test_something() {
    assert_snapshot!("first_snapshot", "first value");
    assert_snapshot!("second_snapshot", "second value");
}

The first argument to the family of snapshot macros can be a string.

So if you're explicitly specifying a snapshot name, I've found insta no longer relies on any outside ordering, and snapshot files no longer get mixed up. However, with rstest and its case or values, you then run into the issue of generating those snapshot names dynamically.

That gets old quickly, as you'll have to assemble a string from function signature arguments by hand for each test.

Note this assumes the tuple of all current arguments to the test function uniquely identifies the test. If you have a duplicate test, this no holder holds true, but should be very easy to fix and rarely happen in real life: just remove the duplicate test. And even if it duplicates remain present, insta would just replace a snapshot file with one of the same contents and should remain happy (didn't test this). Only in the case of identical tests producing different results (aka snapshots) would that not work, but that would also trip up any other form of unit testing I'm aware of.

---

So this is where I came up with a macro. It's overengineered and probably poorly implemented (new to Rust). It generates a struct from the function signature, makes that available and implements Display, such that the snapshot name can simply be generated from that as struct.to_string(). It also sets insta's info, such that in review, snapshot contents are displayed properly automatically. For this, it also needs Serialize from serde.

The macro reads:

pub fn sanitize_for_filename_use(filename: &str) -> String {
    const REPLACEMENT: char = '_';
    filename
        .replace(
            [
                ' ', ':', '<', '>', '\"', '/', '\\', '|', '?', '*', '\n', '\r',
            ],
            &REPLACEMENT.to_string(),
        )
        // Collapse consecutive underscores into one
        .split(REPLACEMENT)
        .filter(|&s| !s.is_empty())
        .collect::<Vec<_>>()
        .join(&REPLACEMENT.to_string())
}

#[macro_export]
macro_rules! instrament {
    ($(#[$attr:meta])* fn $name:ident ( $( $(#[$arg_attr:meta])* $arg:ident : $type:ty),* ) $body:expr ) => {
        ::paste::paste! {
            #[derive(::serde::Serialize)]
            struct [<$name:camel>]<'a> {
                $( $arg: &'a $type, )*
            }

            impl<'a> ::std::fmt::Display for [<$name:camel>]<'a> {
                fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
                    let items: Vec<String> = vec![
                        stringify!($name).to_owned() ,
                        $(
                            format!("{:#?}", self.$arg).to_owned() ,
                        )*
                    ];

                    let name = $crate::sanitize_for_filename_use(&items.join("-"));
                    write!(f, "{}", name)
                }
            }

            $(#[$attr])*
            fn $name ( $( $(#[$arg_attr])* $arg : $type),* ) {
                let function_data = [<$name:camel>] { $($arg: &$arg),* };
                let mut settings = ::insta::Settings::clone_current();
                settings.set_info(&function_data);

                settings.bind(|| {
                    #[allow(clippy::redundant_closure_call)]
                    $body(&function_data);
                });
            }
        }
    };
}

and a test example reads:

use itertools::Itertools;

fn power_set<C, T>(collection: C) -> Vec<Vec<T>>
where
    C: IntoIterator<Item = T>,
    T: Clone,
{
    let vec = collection.into_iter().collect_vec();

    // https://en.wikipedia.org/wiki/Power_set#Properties
    let mut result = Vec::with_capacity(2usize.checked_pow(vec.len() as u32).expect("Overflow"));

    for i in 0..=vec.len() {
        result.extend(vec.iter().cloned().combinations(i));
    }

    result
}

#[cfg(test)]
mod tests {
    use super::power_set;
    use instrament::instrament;
    use rstest::rstest;

    instrament! {
        #[rstest]
        fn test_power_set(
            #[values(vec![], vec![1], vec![1, 2], vec![1, 2, 3])]
            collection: Vec<i32>
        ) (|data: &TestPowerSet| {
            let result = power_set(collection.clone());
            insta::assert_yaml_snapshot!(data.to_string(), result);
        })
    }
}

in which the instrament! { ... } block expands to (only a fraction of which is from my macro):

// Recursive expansion of instrament! macro
// =========================================

#[derive(::serde::Serialize)]
struct TestPowerSet<'a> {
    collection: &'a Vec<i32>,
}
impl<'a> ::std::fmt::Display for TestPowerSet<'a> {
    fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
        let items: Vec<String> = (<[_]>::into_vec(
            #[rustc_box]
            $crate::boxed::Box::new([
                ("test_power_set".to_owned()),
                ({
                    let res = $crate::fmt::format(::core::fmt::Arguments::new_v1(
                        &[""],
                        &[::core::fmt::ArgumentV1::new(
                            &(self.collection),
                            ::core::fmt::Display::fmt,
                        )],
                    ));
                    res
                }
                .to_owned()),
            ]),
        ));
        let name = $crate::sanitize_for_filename_use(&items.join("-"));
        f.write_fmt(::core::fmt::Arguments::new_v1(
            &[""],
            &[::core::fmt::ArgumentV1::new(
                &(name),
                ::core::fmt::Display::fmt,
            )],
        ))
    }
}
#[rstest]
fn test_power_set(#[values(vec![],vec![1],vec![1,2],vec![1,2,3])] collection: Vec<i32>) {
    let function_data = TestPowerSet {
        collection: &collection,
    };
    let mut settings = ::insta::Settings::clone_current();
    settings.set_info(&function_data);
    settings.bind(|| {
        #[allow(clippy::redundant_closure_call)]
        (|data: &TestPowerSet| {
            let result = power_set(collection.clone());
            {
                {
                    let value = $crate::_macro_support::serialize_value(
                        &result,
                        $crate::_macro_support::SerializationFormat::Yaml,
                        $crate::_macro_support::SnapshotLocation::File,
                    );
                    {
                        $crate::_macro_support::assert_snapshot(
                            (Some((data.to_string()))).into(),
                            &value,
                            "/some/path/to/project",
                            {
                                fn f() {}

                                fn type_name_of_val<T>(_: T) -> &'static str {
                                    std::any::type_name::<T>()
                                }
                                let mut name =
                                    type_name_of_val(f).strip_suffix("::f").unwrap_or("");
                                while let Some(rest) = name.strip_suffix("::{{closure}}") {
                                    name = rest;
                                }
                                name
                            },
                            "module::path",
                            "",
                            0 as u32,
                            ("result"),
                        )
                        .unwrap()
                    };
                };
            };
        })(&function_data);
    });
}

The info part of insta together with a derived Serialize then allows the test cases to look like this in review:

So far, to address the core issue of this thread, I have observed no instability (insta/rstest combination reordering test cases/snapshots, triggering false positives).

One caveat is that, while snapshot names will be sanitized for filename use for both Linux and Windows, on a case-insensitive file system you cannot have test cases that, when cast to a filename via data.to_string(), compare equally under case insensitivity. For example, if your entire function signature is a single &str, and you use two inputs "Hello" and "hello", snapshots will once again be confused. This shouldn't occur on sane, aka case-sensitive file systems.

Another disadvantage is that intellisense shits the bed (as it does for other macros as well though), and the closure syntax (|data: &TestPowerSet| { ... test code .... } is very awkward.

[la10736]

THX @alexpovel to share your solution here. You're right, is not possible to use default named snapshot because sadly insta assume that assert_*_snapshot!() is always in the test body.

Anyway you can use something like

use insta::assert_yaml_snapshot;
use rstest::rstest;

#[fixture]
pub fn testname() -> String {
    thread::current().name().unwrap().to_string()
}

#[rstest]
fn test_option_value(testname: String, #[values(1,2,3,4,5,6,7,8,9,10)] value: u32) {
    assert_yaml_snapshot!(testname, value);
}

The testname fixture give you the same name that cargo test shows. Maybe is better to sanitize it but I'm not sure

[viperML]

I just ran into this problem by combining rstest with insta. My solution is to input the values as a tuple of (name, case) instead, to get stable names:

So, instead of:

#[rstest]
fn test_option_value(
    #[values(
        1,
        2
    )]
    value: u32
) {
    assert_yaml_snapshot!(value);
}

This:

#[rstest]
fn test_option_value(
    #[values(
        ("foo", 1),
        ("bar", 2)
    )]
    value: (&str, u32)
) {
    assert_yaml_snapshot!(value.0, value.1);
}