la10736 / rstest

Fixture-based test framework for Rust
Apache License 2.0
1.21k stars 43 forks source link

Consider allowing to rename the crate by passing an attribute `crate = ...` to the proc-macro #221

Closed FirelightFlagboy closed 6 months ago

FirelightFlagboy commented 1 year ago

Currently the crate rstest cannot be renamed.

That would be nice if it could have something like the proc-macro tokio::test where you can rename the crate by providing the attribute crate: https://docs.rs/tokio-macros/latest/tokio_macros/attr.test.html#rename-package

Reproduction steps

I've create a simple rust project with cargo new --lib rstest-renamed-bug and edited the following files:

How does that expand ?

If we look at the expanded code using cargo-expand:

cargo +nightly expand --lib --tests

We have the following rust output:

// ...
#[cfg(test)]
mod tests {
    use super::*;
    use renamed_rstest::rstest;
    // ... 
    #[cfg(test)]
    mod it_works {
        use super::*;
        extern crate test;
        #[cfg(test)]
        #[rustc_test_marker = "tests::it_works::case_1"]
        pub const case_1: test::TestDescAndFn = test::TestDescAndFn {
            desc: test::TestDesc {
                name: test::StaticTestName("tests::it_works::case_1"),
                ignore: false,
                ignore_message: ::core::option::Option::None,
                source_file: "src/lib.rs",
                start_line: 15usize,
                start_col: 8usize,
                end_line: 15usize,
                end_col: 16usize,
                compile_fail: false,
                no_run: false,
                should_panic: test::ShouldPanic::No,
                test_type: test::TestType::UnitTest,
            },
            testfn: test::StaticTestFn(|| test::assert_test_result(case_1())),
        };
        fn case_1() {
            let left = {
                use rstest::magic_conversion::*;
                (&&&Magic::<&str>(std::marker::PhantomData)).magic_conversion("2")
            };
            let right = {
                use rstest::magic_conversion::*;
                (&&&Magic::<&str>(std::marker::PhantomData)).magic_conversion("2")
            };
            let expected = {
                use rstest::magic_conversion::*;
                (&&&Magic::<&str>(std::marker::PhantomData)).magic_conversion("22")
            };
            it_works(left, right, expected)
        }
}
// ...

The problem is caused by the use rstest::magic_conversion::*; lines

la10736 commented 1 year ago

Nice catch!!! Thx for reporting it!

This issue is just related to magic_conversion that is applied just to literal str.

The bug is here: I assume that you have imported rstest as rstest: https://github.com/la10736/rstest/blob/b32e182e96e1927491f1bf2d76b6d3ded939d957/rstest_macros/src/render/inject.rs#L103

I don't know how I can fix it but I can investigate is there is a way to identify the new crate name on compile time and use it in procedural macro.

la10736 commented 1 year ago

Seams https://docs.rs/proc-macro-crate/latest/proc_macro_crate does the job... but is not really clean and some edge-case are not covered.

The base Idea is to parse the Cargo.toml and extract the new name.... maybe there's something better.

FirelightFlagboy commented 1 year ago

Having the attribute crate would be enough for me since we have a proc-macro that wrap our tests.

But for other people that don't, that would be nice if the new name of the crate could be inferred with proc-macro-crate as you said.

la10736 commented 1 year ago

I'm not sure if I understand your point correctly... in procedural macro I cannot use $crate to identify the macro crate. Why you say "Having the attribute crate would be enough for me since we have a proc-macro that wrap our tests"? How do you think I can implement a fix that is suitable for your case?

FirelightFlagboy commented 1 year ago

By

Having the attribute crate would be enough for me since we have a proc-macro that wrap our tests

I mean something like:

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

    use renamed_rstest::rstest;

    #[rstest(crate = "renamed_rstest")]
    #[case("2", "2", "22")]
    fn it_works(#[case] left: &str, #[case] right: &str, #[case] expected: &str) {
        assert_eq!(join(left, right), expected);
    }
}

Where the proc-macro rstest could take an attribute crate that is "set" to the actual rstest crate name.

tokio does that for the proc-macro test:

use tokio as tokio1;

#[tokio1::test(crate = "tokio1")]
async fn my_test() {
    println!("Hello world");
}
la10736 commented 1 year ago

Ok.... I got it. I don't love it but is pragmatic. The downside here is that you should write it in all tests :cry: I'll implement the following logic:

  1. Add crate option to rstest : this will not work with the (deprecated) compact form
  2. Add a feature to implement the rstest name discovering that use proc-macro-crate (disable by default)
  3. use rstest as last fallback.

The real issue here is.... I don't know when I'll can do it :cry:

la10736 commented 1 year ago

Take a look to substrate/primitives/api/proc-macro/src/utils.rs where thy solve the issue

FirelightFlagboy commented 1 year ago

Take a look to substrate/primitives/api/proc-macro/src/utils.rs where thy solve the issue

I'm not sure to understand what you mean. I can't find a file named substrate/primitives/api/proc-macro/src/utils.rs, maybe you mean rstest_macros/src/utils.rs ?

la10736 commented 1 year ago

Sorry.. it was just a note for me... in https://github.com/paritytech/polkadot-sdk/blob/master/substrate/primitives/api/proc-macro/src/utils.rs they handling the same problem and solve it without introducing the string name needs.

la10736 commented 6 months ago

Ok, I've implemented it.

Now you can import and use it with any name:

my_great_name = {package = "rstest", version = "0.20.0" }

and in your code use

use my_great_name::{rstest, fixture}

I'll publish this version ASAP

FirelightFlagboy commented 6 months ago

Oh, nice I'll try that this week :)