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

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

Open
FirelightFlagboy opened this issue Oct 16, 2023 · 9 comments

Comments

@FirelightFlagboy
Copy link

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:

  • src/lib.rs with the following content:

    pub fn join(left: &str, right: &str) -> String {
        format!("{left}:{right}")
    }
    
    #[cfg(test)]
    mod tests {
        use super::*;
    
        use renamed_rstest::rstest;
    
        #[rstest]
        #[case("2", "2", "22")]
        fn it_works(#[case] left: &str, #[case] right: &str, #[case] expected: &str) {
            assert_eq!(join(left, right), expected);
        }
    }
  • Cargo.toml with the following content:

    [package]
    name = "rstest-renamed-bug"
    version = "0.1.0"
    edition = "2021"
    
    [dependencies]
    
    [dev-dependencies]
    renamed_rstest = { version = "0.18.2", package = "rstest", default-features = false }

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

@FirelightFlagboy FirelightFlagboy changed the title Allow to rename the crate Consider allowing to rename the crate by passing an attribute crate = ... to the proc-macro Oct 16, 2023
@la10736
Copy link
Owner

la10736 commented Oct 17, 2023

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:

fn handling_magic_conversion_code(fixture: Cow<Expr>, arg_type: &Type) -> Expr {

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

la10736 commented Oct 17, 2023

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

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

la10736 commented Oct 17, 2023

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

FirelightFlagboy commented Oct 17, 2023

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

la10736 commented Oct 17, 2023

Ok.... I got it. I don't love it but is pragmatic. The downside here is that you should write it in all tests 😢
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 😢

@la10736
Copy link
Owner

la10736 commented Nov 13, 2023

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

@FirelightFlagboy
Copy link
Author

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

la10736 commented Nov 17, 2023

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.

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