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

Can't figure out how to cleanly reformat test cases using rustfmt / fmt #281

Open
JohnstonJ opened this issue Oct 1, 2024 · 4 comments
Open

Comments

@JohnstonJ
Copy link

TL;DR: I want my test case inputs into rstest to be auto-reformatted with rustfmt, and this is proving surprisingly hard to figure out a way to do this without introducing other downsides like the possibility of forgetting to run a test case.

I have some more complicated test case inputs, and I'm struggling to figure out how to use rstest so that they are automatically formatted by rustfmt. Is it even possible in a reasonable way? If not, I wonder if there may be some solutions possible to make the situation better.

Currently, it seems that anything inside of #[case::panic(42, "Foo")] is not reformatted by rustfmt, and this can get quite messy if we are passing in a lot of parameters and/or complex structs.

The best workaround I've come up with is to put each test case into its own separate variable, and reference the variables. Here is an example:

Click to view example
#[derive(Debug)]
struct InfoCheckSimilarTestCase<'a> {
    expected: Info,
    comparison: Info,
    err: Option<&'a str>,
}

static INFO_CHECK_SIMILAR_MATCHES: LazyLock<InfoCheckSimilarTestCase> =
    LazyLock::new(|| InfoCheckSimilarTestCase {
        expected: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        comparison: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        err: None,
    });

static INFO_CHECK_SIMILAR_FRAME_RATE_MISMATCH: LazyLock<InfoCheckSimilarTestCase> =
    LazyLock::new(|| InfoCheckSimilarTestCase {
        expected: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        comparison: Info {
            file_size: 720_000,
            video_frame_rate: Ratio::<u32>::from(25),
            video_duration: Ratio::<u128>::new(5, 25),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        err: Some("Video frame rate 25 does not match 30000/1001"),
    });

static INFO_CHECK_SIMILAR_FRAME_SIZE_MISMATCH: LazyLock<InfoCheckSimilarTestCase> =
    LazyLock::new(|| InfoCheckSimilarTestCase {
        expected: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        comparison: Info {
            file_size: 1_200_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        err: Some("Video frame size 240000 does not match 120000"),
    });

static INFO_CHECK_SIMILAR_AUDIO_STREAM_COUNT_MISMATCH: LazyLock<InfoCheckSimilarTestCase> =
    LazyLock::new(|| InfoCheckSimilarTestCase {
        expected: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        comparison: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 1,
            audio_sample_rate: Some(32_000),
        },
        err: Some("Audio stereo stream count 1 does not match 2"),
    });

static INFO_CHECK_SIMILAR_AUDIO_SAMPLE_RATE_MISMATCH: LazyLock<InfoCheckSimilarTestCase> =
    LazyLock::new(|| InfoCheckSimilarTestCase {
        expected: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(32_000),
        },
        comparison: Info {
            file_size: 600_000,
            video_frame_rate: Ratio::<u32>::new(30_000, 1_001),
            video_duration: Ratio::<u128>::new(1_001 * 5, 30_000),
            audio_stereo_stream_count: 2,
            audio_sample_rate: Some(48_000),
        },
        err: Some("Audio sample rate 48000 does not match 32000"),
    });

#[googletest::test]
#[rstest]
#[case::matches(&*INFO_CHECK_SIMILAR_MATCHES)]
#[case::frame_rate_mismatch(&*INFO_CHECK_SIMILAR_FRAME_RATE_MISMATCH)]
#[case::frame_size_mismatch(&*INFO_CHECK_SIMILAR_FRAME_SIZE_MISMATCH)]
#[case::audio_stream_count_mismatch(&*INFO_CHECK_SIMILAR_AUDIO_STREAM_COUNT_MISMATCH)]
#[case::audio_sample_rate_mismatch(&*INFO_CHECK_SIMILAR_AUDIO_SAMPLE_RATE_MISMATCH)]
fn test_info_check_similar(#[case] tc: &InfoCheckSimilarTestCase) {
    let expected = UnvalidatedInfo::from(tc.expected).validate().unwrap();
    let comparison = UnvalidatedInfo::from(tc.comparison).validate().unwrap();
    match tc.err {
        Some(e) => expect_that!(expected.check_similar(&comparison), err(displays_as(eq(e)))),
        None => expect_that!(expected.check_similar(&comparison), ok(eq(&()))),
    };
}

While this does restore the ability to format the complex test cases with rustfmt because at this point they are simple static variables, this workaround has several downsides:

  • Most critically: if someone creates a new test case variable but forgets to add the corresponding #[case(...)], then the test case is not run!
  • The name of the test case is copy/pasted into three different places, providing opportunities to make a mistake that results in a test case being misnamed somewhere.
  • In general, it's a little more verbose than it ought to be: (a) LazyLock syntax is repeated over & over, (b) duplication of test case name increases verbosity.

Ideally, there would be a way to have #[rstest] consume a list of test cases from someplace else? But I haven't been able to figure out how to do that. The matrix / list input testing functionality still seems to demand a hard-coded list, which still leaves opportunity for somebody to forget to add new test case variables to the rstest matrix.

Key things I'm looking for:

  • Each test case shows up as a subtest in my IDE's test explorer. No simple "for" loops over the test cases - this is why I'm using rstest instead.
  • The most complex parts of each test case are auto-formatted e.g. via rustfmt, so that I don't spend hours trying to get the formatting consistent.
  • Test case inputs are coded in Rust. For example, solutions that involve defining test cases in YAML or some other format that is very different from Rust, and then deserializing with serde, aren't particularly appealing to me. I'd like IDE auto-completion support when filling in each test case, for example.

Past experiences in other languages:

  • In Go, I meet these requirements by simply looping over an array of test case structs inside my test, and then using the subtest functionality of Go to start a new subtest in each loop iteration.
  • In Python, I use pytest's parametrize functionality. But unlike rstest's case syntax, Python's black reformatter has no problem with reformatting code inside a decorator. So I put the test case structures directly inside of the decorator, similar to the rstest examples in the documentation.

In comparison, this is proving surprisingly hard for me to figure out how best to do this in Rust...

@JohnstonJ
Copy link
Author

Here's an updated workaround I came up with. It solves the problem of somebody forgetting to add a #[case] attribute by moving all the test cases into a single HashMap, and then adding a test case that asserts that every map entry has a corresponding test in the test binary.

Updated workaround
use core::str;
use std::{
    collections::{HashMap, HashSet},
    ops,
    path::PathBuf,
    sync::LazyLock,
};

use derive_more::{Deref, From};
use googletest::prelude::*;
use regex::Regex;

/// Directory containing test-related data files.
pub(crate) fn test_resource(path: &str) -> PathBuf {
    [env!("CARGO_MANIFEST_DIR"), "resources/test", path].iter().collect()
}

/// Map of test case name to test case data.  See the [`test_case_map`] macro for an example of how
/// to use this.
#[derive(Debug, Deref, From)]
pub(crate) struct TestCaseMap<'a, T>(HashMap<&'a str, T>);

impl<'a, T> TestCaseMap<'a, T> {
    /// Return test case given the test function name generated by rstest
    ///
    /// The parameter needs to be the return value of the [`stdext::function_name`] macro.  The
    /// macro must be called from within a `#[case]` attribute.  Do not call it from within your
    /// test function itself.
    ///
    /// Example input: `my_crate_name::my_module::tests::test_frob::case_1_success_if_foo` which
    /// will return `success`.
    pub(crate) fn get_test_case(&self, test_function_name: &'a str) -> &T {
        let case_name = &GET_TEST_CASE_RE.captures(test_function_name).unwrap()[1];
        assert!(self.contains_key(case_name), "Test case {case_name} is not in test table");
        &self[case_name]
    }
}

/// A lazy-initialized map of test cases.  See the [`test_case_map`] macro for an example of how to
/// use this.
pub(crate) type LazyTestCases<'a, T> = LazyLock<TestCaseMap<'a, T>>;

/// Lazy-initializes a map of [`rstest`] test cases for a given function.  Intended for use as a
/// static variable.
///
/// The advantage of using this over passing structures directly to `#[case]` is that the structures
/// will be auto-formatted with rustfmt.
///
/// # Examples
///
/// ```ignore
/// use rstest::rstest;
/// use stdext::function_name;
///
/// #[derive(Debug)]
/// struct FunctionATestCase {
///     foo: i32,
///     bar: i32,
/// }
///
/// static FUNCTION_A_TEST_CASES: LazyTestCases<FunctionATestCase> = test_case_map!(
///     "first_case",
///     FunctionATestCase { foo: 1, bar: 1 },
///     "second_case",
///     FunctionATestCase { foo: 2, bar: 2 }
/// );
///
/// #[rstest]
/// #[case::first_case(function_name!())]
/// #[case::second_case(function_name!())]
/// fn test_function_a(#[case] test_function_name: &str) {
///     let tc = FUNCTION_A_TEST_CASES.get_test_case(test_function_name);
///
///     assert!(tc.foo > 0);
/// }
///
/// // If you add an entry to FUNCTION_A_TEST_CASES but forget to add a #[case] for it, this will
/// // catch the mistake.
/// test_all_test_cases_ran!(
///     ("test_function_a", &FUNCTION_A_TEST_CASES)
/// );
/// ```
macro_rules! test_case_map {
    ($($test_case_name:expr, $test_case_data:expr),*) => {
        ::std::sync::LazyLock::new(|| ::std::collections::HashMap::<&str, _>::from([
            $(($test_case_name, $test_case_data)),*
        ]).into())
    };
}

pub(crate) use test_case_map;

/// Asserts that all [`rstest`] test cases in the given tables of test cases are actual test cases
/// in this test binary.
///
/// If you are using the [`test_case_map`] macro and [`TestCaseMap`] type, then you should also
/// call this macro to make sure that every entry in a [`TestCaseMap`] has a corresponding `case`
/// statement in [`rstest::rstest`].
///
/// # Examples
///
/// ```ignore
/// test_all_test_cases_ran!(
///     ("test_function_a", &FUNCTION_A_TEST_CASES),
///     ("test_function_b", &FUNCTION_B_TEST_CASES),
/// );
/// ```
macro_rules! test_all_test_cases_ran {
    ($($test_function_test_cases:expr),*) => {
        #[test]
        fn test_all_test_cases_ran() {
            let mut _expected_test_cases = ::std::collections::HashMap::<&str, Vec<&str>>::new();
            $({
                let (test_function_name, test_case_map) = $test_function_test_cases;
                _expected_test_cases
                    .insert(test_function_name, test_case_map.keys().map(|k| *k).collect());
            })*
            $crate::testutil::assert_all_test_cases_ran(&_expected_test_cases, module_path!());
        }
    };
}

pub(crate) use test_all_test_cases_ran;

static GET_TEST_CASE_RE: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r".+::case_\d*_([^:]+)$").unwrap());

static GET_CRATE_NAME_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^[^:]+").unwrap());

static PARSE_TEST_LIST_RE: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"^(.+)::case_\d*_(.+): test$").unwrap());

/// Asserts that all [`rstest`] test cases in the given tables of test cases are actual test cases
/// in this test binary.
///
/// The function is not generally to be used directly.  Use the [`test_all_test_cases_ran`] macro
/// instead.
pub(crate) fn assert_all_test_cases_ran(
    expected_test_cases: &HashMap<&str, Vec<&str>>,
    test_module_path: &str,
) {
    // final result for each set are paths like:
    // my_crate_name::my_module::tests::test_frob::success_if_foo

    // test cases built from the user's tast table HashMap
    let expected_test_cases = HashSet::<String>::from_iter(expected_test_cases.iter().flat_map(
        |(test_function_name, test_cases_for_function)| {
            test_cases_for_function.iter().map(move |test_case_name| {
                format!("{test_module_path}::{test_function_name}::{test_case_name}")
            })
        },
    ));

    let crate_name = GET_CRATE_NAME_RE.find(test_module_path).unwrap().as_str();

    // run the test executable to list all tests
    // the output of this command looks like:
    // my_module::tests::test_frob::case_02_success_if_foo: test
    let list_all_tests_output = std::process::Command::new(std::env::current_exe().unwrap())
        .args(["--list", "--format", "terse"])
        .output()
        .unwrap();
    assert_that!(
        list_all_tests_output.status.success(),
        eq(true),
        "Failed to run test EXE to list tests"
    );
    let actual_test_cases = HashSet::<String>::from_iter(
        str::from_utf8(&list_all_tests_output.stdout).unwrap().lines().flat_map(
            |test_output_line| {
                let parsed = PARSE_TEST_LIST_RE.captures(test_output_line)?;
                Some(format!("{crate_name}::{}::{}", &parsed[1], &parsed[2]))
            },
        ),
    );

    let unrun_test_cases: Vec<_> = expected_test_cases.difference(&actual_test_cases).collect();
    assert_that!(
        unrun_test_cases,
        empty(),
        "These test cases are not referenced by the test function with a #[case] attribute."
    );
}

The test_case_map macro has an example that shows how to use it.

I don't particularly love this solution, but I haven't been able to think of a better way. It makes some assumptions about the output of the test binary when using --list, it has to invoke the test binary in the first place, it makes some assumptions about the naming convention of the rstest-generated code, and the function_name! macro that was used makes some assumptions about how Rust returns this string.

Potentially rstest could generate more code that reduces/eliminates the need for some of these assumptions, but then again it would also be adding new public APIs for what amounts to a rustfmt workaround, so not sure if it's worth it.

For example, rstest could provide an automatic "test case name" fixture so I don't have to call function_name!. It could also provide a fixture for the name of the test itself. (Something that seems to be a common request but hasn't yet been implemented by Rust's testing infrastructure - no good solutions to get at runtime. But rstest, being a macro, can easily discover the test name during parsing.) It could also emit a static variable that contains the list of all test cases for the test (names of generated test functions).

@la10736
Copy link
Owner

la10736 commented Oct 3, 2024

For function name you can take a look to #177 as example. I would like to implement a #[context] attribute but I don't know if I can find some time to do it

@la10736
Copy link
Owner

la10736 commented Oct 3, 2024

Just to note that everybody can write and publish crate with fixture implementations these crate should not be included in rstest but just use it.

@JohnstonJ
Copy link
Author

Yeah.... using the thread name feels like an even worse undocumented hack though. I tried out that approach as well, but I wasn't comfortable with it because I wasn't sure how reliable it would be. I noticed there has been some instability in the thread name in the past, like if the number of test threads is changed to be single-threaded.

It seems a lot of people are asking for a stable API to get the test name, but the requests are several years old, so who knows if/when it will happen. In the meantime, it seems like a useful thing that rstest could provide in a much more stable/supported way.

I would like to implement a #[context] attribute

If by this, you mean allowing the injection of a parameter to the test that has context information about the test (e.g. test name, other things) - i.e. similar to Go's testing.T type, which provides a name() function, then I think it's a great idea!

Just to note that everybody can write and publish crate with fixture implementations these crate should not be included in rstest but just use it.

For the function name macro I referenced, I think it is impossible to put it in a fixture. It would just return the name of the fixture function instead of the test case name. I looked at the code generated by rstest, and it seems that each test case results in a separate generated function which calls the original user-provided function. The call to this function name macro has to happen inside the generated test case function in order to come up with the test case name.

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

No branches or pull requests

2 participants