By-reference fixtures

[narpfel]

I have an annoying problem regarding invariant lifetimes in test case inputs.

A simplified example:

#![allow(dead_code)]

use std::cell::Cell;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum E<'a> {
    A(bool),
    B(&'a Cell<E<'a>>),
}

fn make_e_from_bool<'a>(_bump: &'a (), b: bool) -> E<'a> {
    E::A(b)
}

fn new_bump() {}

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

    use super::*;

    #[rstest]
    #[case(true, E::A(true))]
    fn it_works(#[case] b: bool, #[case] expected: E) {
        let bump = &new_bump();
        let actual = make_e_from_bool(bump, b);
        assert_eq!(actual, expected);
    }
}

E contains a reference to a Cell containing an E, making it invariant in the lifetime parameter 'a. This means that the automatically derived PartialEq instance requires that both E objects have the same lifetime parameter. However, actual’s lifetime is tied to the lifetime of bump via make_e_from_bool, while expected can have any lifetime. (In my real code, bump is a bumpalo::Bump, so I can’t leak it if I don’t want memory leaks. Another solution that I don’t really want to use is to implement PartialEq for E by hand, because that’s error-prone when E is changed.)

If it was possible to take bump by reference from a fixture, this would not be a problem, because the signature of it_works could specify the correct lifetimes for bump and expected:

[...]

    #[fixture]
    fn bump() {}

    #[rstest]
    #[case(true, E::A(true))]
    fn it_works<'a>(bump: &'a (), #[case] b: bool, #[case] expected: E<'a>) {
        let actual = make_e_from_bool(bump, b);
        assert_eq!(actual, expected);
    }

[...]

I have an (untested proof-of-concept) implementation of this here that checks if parameters to test functions are references and adds & in render_exec_call if they are. Is this a feature you’d like to have in rstest? The generated code looks like this:

        fn case_1() {
            let bump = bump::default();
            let b = true;
            let expected = E::A(true);
            it_works(&bump, b, expected)
            //       ^ only change is this `&`
        }

[la10736]

Yes, it'll be very nice to have this feature. Freak free to open a PR.

[la10736]

While I was fixing #230 I noted that the following syntax will work

use std::cell::Cell;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum E<'a> {
    A(bool),
    B(&'a Cell<E<'a>>),
}

fn make_e_from_bool<'a>(b: bool) -> E<'a> {
    E::A(b)
}

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

    use super::*;

    #[rstest]
    #[case(true, E::A(true))]
    fn it_works<'a>(#[case] b: bool, #[case] expected: E<'a>) {
        let actual = make_e_from_bool(b);
        assert_eq!(actual, expected);
    }
}

Is this code fine for you?

[narpfel]

Unfortunately not. This is my real code:

    #[rstest]
    #[case::bool("true", Value::Bool(true))]
    fn test_eval<'a>(bump: &'a Bump, #[case] src: &'static str, #[case] expected: Value<'a>) {
        pretty_assertions::assert_eq!(eval_str(bump, src).unwrap(), expected);
    }

bump is actually used by the code under test and can’t be removed.

[la10736]

And what about to use #[once] here? Is it an issue to have a just an Bump instance?

With the unreleased version follow code works

use std::cell::Cell;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum E<'a> {
    A(bool),
    B(&'a Cell<E<'a>>),
}

fn make_e_from_bool<'a>(_bump: &'a (), b: bool) -> E<'a> {
    E::A(b)
}

fn new_bump() {}

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

    use super::*;

    #[fixture]
    #[once]
    fn bump() -> () {}

    #[rstest]
    #[case(true, E::A(true))]
    fn it_works<'a>(bump: &'a (), #[case] b: bool, #[case] expected: E<'a>) {
        let actual = make_e_from_bool(&bump, b);
        assert_eq!(actual, expected);
    }
}

[narpfel]

A #[once] fixture would be possible, but I’d like to avoid it if possible because then the tests would leak memory, and that’s annoying when testing with tools that flag memory leaks such as miri or valgrind.

[la10736]

That's new for me... why static reference is marked as memory leak in valgrind? Ok, verbose set up will report it as an not released static reference but should not be an issue.

Anyway I cannot accept a PR about this just because is a breaking change when #[once] fixture is used.

Beside that my next feature will be a #[map] attribute that should be help to fix this case too: follow code should will work

use std::cell::Cell;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum E<'a> {
    A(bool),
    B(&'a Cell<E<'a>>),
}

fn make_e_from_bool<'a>(_bump: &'a (), b: bool) -> E<'a> {
    E::A(b)
}

fn new_bump() {}

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

    use super::*;

    #[fixture]
    fn bump() -> () {}

    #[rstest]
    #[case(true, E::A(true))]
    fn it_works<'a>(#[map(|b| &b)] bump: &'a (), #[case] b: bool, #[case] expected: E<'a>) {
        let actual = make_e_from_bool(&bump, b);
        assert_eq!(actual, expected);
    }
}

[narpfel]

I think that even with a #[once] fixture it will not work, because bumpalo::Bump is !Sync and has to be wrapped in a Mutex. And to lock it, you create a MutexGuard, which derefs to the Bump. But... the reference’s lifetime is bounded by the MutexGuard’s lifetime, so we’re back to exactly the same problem as in the initial code.

Anyway I cannot accept a PR about this just because is a breaking change when #[once] fixture is used.

I think there’s a non-breaking way to implement this by annotating fixture parameters with an attribute to enable pass-by-reference, like this:

#[fixture]
fn f() -> u32 { 42 }

#[rstest]
fn test(#[by_reference] f: &u32) {
    assert_eq!(*f, 42);
}

Beside that my next feature will be a #[map] attribute that should be help to fix this case

How would that work? It looks like the closure returns a reference to a local variable?

[la10736]

I think that even with a #[once] fixture it will not work, because bumpalo::Bump is !Sync and has to be wrapped in a Mutex. And to lock it, you create a MutexGuard, which derefs to the Bump. But... the reference’s lifetime is bounded by the MutexGuard’s lifetime, so we’re back to exactly the same problem as in the initial code.

Right!

Anyway I cannot accept a PR about this just because is a breaking change when #[once] fixture is used.

I think there’s a non-breaking way to implement this by annotating fixture parameters with an attribute to enable pass-by-reference, like this:

#[fixture]
fn f() -> u32 { 42 }

#[rstest]
fn test(#[by_reference] f: &u32) {
    assert_eq!(*f, 42);
}

Beside that my next feature will be a #[map] attribute that should be help to fix this case

How would that work? It looks like the closure returns a reference to a local variable?

Yes, that's feasible. Maybe the right syntax can be just #[ref] and should work for every kind of input.

[narpfel]

Maybe the right syntax can be just #[ref]

I don’t think #[ref] can be an attribute because ref is a keyword. #[r#ref] would be possible, but at that point I think #[by_ref] is better.

and should work for every kind of input.

You mean also for #[case], #[values], and #[files]? That would need some more work in this code

rstest/rstest_macros/src/parse/rstest.rs

Lines 134 to 140 in 06a2117

	let composed_tuple!(fixtures, case_args, cases, value_list, files) = merge_errors!(
	extract_fixtures(item_fn),
	extract_case_args(item_fn),
	extract_cases(item_fn),
	extract_value_list(item_fn),
	extract_files(item_fn)
	)?;

because (as I understand it), this would parse e. g. fn f(#[case] #[by_reference] x: u32) as both a test case input and a fixture when both parsers recognise #[by_reference].

For now I’d like to get this feature working (and robust) for #[fixture] first and then maybe look into supporting the other input types.

[la10736]

Ok #[by_ref] it's a good compromise. I've an idea of how to implement it in a generic way. My idea is to catch #[by_ref] and set it as argument attribute and just use & we I use this argument on calling test function.

Example :

#[fixture]
fn f() -> u32 { 42 }

#[rstest]
#[case(42)]
fn test(#[by_ref] f: &u32, #[case] #[by_ref] c: &u32, #[values(42, 142)] #[by_ref] v: &u32) {
    assert_eq!(f, c);
    assert_eq!(*c, *v%100);
}

it'll be expanded in something like follow:

mod f {
    fn default() -> u32 {
        42
    }
}

fn test(f: &u32, c: &u32, v: &u32) {
    assert_eq!(f, c);
    assert_eq!(*c, *v%100);
}

mod test {
    use super::*;
    mod case_1 {
        use super::*;
        #[test]
        fn v_1_42() {
            let f = f::default();
            let c = 42;
            let v = 42;
            test(&f, &c, &v)
        }

        #[test]
        fn v_2_142() {
            let f = f::default();
            let c = 42;
            let v = 142;
            test(&f, &c, &v)
        }
    }
}

[la10736]

@narpfel I've implemented it in https://github.com/la10736/rstest/tree/by_ref
I didn't published it yet because I need to do some refactor till merge it. You can try it ... let me know if it's ok

[narpfel]

That branch works for my use case. Thanks!