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

fix: improve compatibility among test proc macros #143

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kezhuw
Copy link

@kezhuw kezhuw commented Apr 17, 2024

This pr proposes a generic mechanism among different test proc macros to avoid to generate multiple [::core::prelude::v1::test] on test method.

proc_macro_attribute function is fed with tokens after its attribute and no tokens before it.

Give the above, this pr proposes test proc macros to append newly generated macros after existing ones. This way, proc macros processed later can read all macros including generated and handwritten and make further decisions. Specifically, proc macros can append #[::core::prelude::v1::test] only if it does not exist.

Macros that transform test method signature can append #[::core::prelude::v1::test] directly without checking its existence once they generate valid signature for test method.

Closes #101.

This pr proposes a generic mechanism among different test proc macros to
avoid to generate multiple `[::core::prelude::v1::test]` on test method.

`proc_macro_attribute` function is fed with tokens after its attribute
and no tokens before it.

Give the above, this pr proposes test proc macros to append newly
generated macros after existing ones. This way, proc macros processed
later can read all macros including generated and handwritten and make
further decisions. Specifically, proc macros can append
`#[::core::prelude::v1::test]` only if it does not exist.

Macros that transform test method signature can append
`#[::core::prelude::v1::test]` directly without checking its existence
once they generate valid signature for test method.

Closes frondeus#101.
@kezhuw
Copy link
Author

kezhuw commented Apr 17, 2024

This comes from my experience in kezhuw/stuck#51.

https://github.com/kezhuw/stuck/blob/629ebefaf00de809011ff93a762e7ecb26e256ce/src/task/session.rs#L609-L616

Currently, above tests are run twice each as both test_case and stuck::test generate #[::core::prelude::v1::test]. Same symptom as #101.

I worked out a solution not only for test_case and stuck::test, but also for test-log, tokio::test and etc. I think it need a cooperation among crates.

One can take kezhuw/stuck#53 as an example for how it solve this. It depends on patch from this pr. cargo test session_cancellation can show the difference with switch [patch.crates-io] in Cargo.toml.

@frondeus @d-e-s-o @taiki-e @Darksonn Sorry for ping you, but would you mind take a look at this and check whether we have a chance to solve this in general ?

@Darksonn
Copy link

So, to sum up, your proposal is that test macros should add #[::core::prelude::v1::test] at the end and only if the function doesn't already have the attribute?

@kezhuw
Copy link
Author

kezhuw commented Apr 17, 2024

Yeh, that is it. And I think test_case and tokio::test could free from check existence. test_log::test should check the existence and append #[::core::prelude::v1::test] only if it does not already have.

@kezhuw
Copy link
Author

kezhuw commented Apr 18, 2024

@tamird expressed similar in d-e-s-o/test-log#35

in favor of parsing other attributes on the test function and inserting #[test] only if nothing else has already done it.

I think it is also encouraged for decoration test macros to not generate any form of #[test] as what traced_test did. I expressed this in kezhuw/stuck#53 (comment).

To be simple, test proc macros should choose either of two.

  1. Don't generate any form of #[test].
  2. Use #[::core::prelude::v1::test] to avoid name collision. And add it to the end of proc macros only if it does not exist.

Hopefully, we can solve this without breaking changes.

@kezhuw kezhuw force-pushed the test-proc-macros-cooperation branch 2 times, most recently from c2573f8 to 2099742 Compare April 19, 2024 05:21
@kezhuw
Copy link
Author

kezhuw commented Apr 19, 2024

Added a fixup commit to error duplicated test attributes which is similar to tokio::test.

kezhuw added a commit to kezhuw/test-log that referenced this pull request Apr 19, 2024
This way if preceding test macros add `#[::core::prelude::v1::test]` by
appending, then we can avoid duplicated test runs.

See also frondeus/test-case#101, frondeus/test-case#143

Closes d-e-s-o#35.
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
@kezhuw
Copy link
Author

kezhuw commented May 24, 2024

Hi @frondeus, would you mind take a look at this ?

It solves #101. I have synced this with tokio counterpart tokio-rs/tokio#6497.

@@ -110,7 +110,7 @@ fn expand_additional_test_case_macros(item: &mut ItemFn) -> syn::Result<Vec<(Tes
}

for i in attrs_to_remove.into_iter().rev() {
item.attrs.swap_remove(i);
item.attrs.remove(i);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This preserve order or attributes.

See rust-lang/rust#82419

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

Successfully merging this pull request may close these issues.

test-env-log used with test-case creates test duplicates
2 participants