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

macros: append generated test attribute so others are aware of it #6497

Merged
merged 11 commits into from
May 5, 2024

Conversation

kezhuw
Copy link
Contributor

@kezhuw kezhuw commented Apr 19, 2024

Motivation

Improve compatibility among multiple test proc macros.

Solution

  1. Add #[::core::prelude::v1::test] to the end of test proc macros. This way, proc macros process later can choose to not generate #[::core::prelude::v1::test] to avoid duplicated test runs.
  2. Forbid #[::core::prelude::v1::test] to avoid duplicated test runs as tokio::test is transforming an invalid test signature to one accepted by #[test]. This is consistent with what we currently treat #[test].

Links

  1. fix: improve compatibility among test proc macros frondeus/test-case#143
  2. Is proc-macro-attribute application order defined? rust-lang/reference#692
  3. Attributes are reordered before they are passed to a proc-macro rust-lang/rust#67839

This way other test macros can choose to not generate
`#[::core::prelude::v1::test]` to avoid duplicated test runs.

This commit also rejects existing `#[::core::prelude::v1::test]` just
like how and why it rejects existing `#[test]`.
@@ -24,7 +24,7 @@ proc-macro = true
[dependencies]
proc-macro2 = "1.0.60"
quote = "1"
syn = { version = "2.0", features = ["full"] }
syn = { version = "2.0", features = ["full", "extra-traits"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you need this feature for? I prefer to avoid adding these features as they hurt compilation times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

impl PartialEq for Attribute. I guess I could duplicate code. Let me figure out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. if it is possible, then try to perform the equality check manually. Thanks!

Comment on lines 97 to 102
error: second test attribute is supplied, try to order it after `tokio::test`
--> $DIR/macros_invalid_input.rs:49:1
|
49 | #[::core::prelude::v1::test::test]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

What? The #[tokio::test] macro is first, so what do you mean by "after"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I supposed this to be generated by other preceding test macros, e.g. #[test_log::test]. I guess I should reshape it or keep it origin. Any preference ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "second test attribute is supplied, consider removing or ordering it after tokio::test" ?

Copy link
Contributor

@Darksonn Darksonn Apr 19, 2024

Choose a reason for hiding this comment

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

My challenge with the wording here is that the code that triggers this error has #[tokio::test] first, and then afterwards, it has a #[::core::prelude::v1::test]. So suggesting that you move #[::core::prelude::v1::test] after #[tokio::test] doesn't make since it's already after #[tokio::test]. Perhaps you could say "before" or "above" instead of "after"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, it is strange. It should be something similar to following.

❯ grep -n -B 2 -A 2 "with_append_test_attribute_and_test_args_and_panic_async" tests/mod.rs
84-#[test_log::test]
85-#[tokio::test]
86:async fn with_append_test_attribute_and_test_args_and_panic_async(x: i8, _y: i8) {
87-  assert_eq!(x, 0);
88-}
❯
❯ cargo test with_append_test_attribute_and_test_args_and_panic_async -- --nocapture
error: second test attribute is supplied, consider removing it or ordering it after `tokio::test`
  --> tests/mod.rs:84:1
   |
84 | #[test_log::test]
   | ^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the attribute macro `test_log::test` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `test-log` (test "mod") due to 2 previous errors

Moving it "before" or "above" will result in duplicated test runs.

I guess we could:

  1. Introduce dev dependency test-log. This ways the words should be more intuitively. Is it worth ?
  2. Use the short version "second test attribute is supplied".
  3. Other suggestions.

Hi @Darksonn, thank you for reviewing. I have updated this with a fixup commit targeting to your two concerns. Regarding the error words, I am open to any sentence, simply not good at 😮‍💨 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just say "consider removing or changing the order of your test attributes".

@mox692 mox692 added the A-tokio-macros Area: The tokio-macros crate label Apr 20, 2024
@kezhuw
Copy link
Contributor Author

kezhuw commented May 5, 2024

Hi @Darksonn, thank you for your reviewing, I have solved all review comments. Is there anything else left for this to move forward ? Should I do some squash/rebase work ?

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Hi again, thanks for the ping. Sorry about the delay.

syn::Meta::Path(path) => path,
_ => return false,
};
let segments = ["core", "prelude", "v1", "test"];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's also available as a few other paths such as ::std::prelude::v1::test and ::core::prelude::rust_2021::test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be too paranoid for us ?

Given that Rust has three editions since v1 and new coming in future, together with combinations from "std", "core" and leading colon, this is a pretty large list.

Given the fact that test is there since day one, I would recommend not doing this. I checked test macros listed below and none use variants other than the two. I don't think test macros should generate other variants, if they do, let them fix it to avoid duplicated test runs but not us.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can match ::[std|core]::prelude::*:test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Additionally, leading colons are optional now as review comments below.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. If you can also add some of the variants to the tests, then I think this is ready to merge.

Comment on lines 48 to 50
#[tokio::test]
#[::core::prelude::v1::test]
async fn test_has_generated_second_test_attr() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about core without the leading colons?

tokio-macros/src/entry.rs Show resolved Hide resolved
@kezhuw kezhuw requested a review from Darksonn May 5, 2024 15:45
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

@Darksonn Darksonn enabled auto-merge (squash) May 5, 2024 16:19
@Darksonn Darksonn merged commit 6fcd9c0 into tokio-rs:master May 5, 2024
77 checks passed
muzarski added a commit to muzarski/scylla-rust-driver that referenced this pull request Jun 12, 2024
We decided to bump minimum required version of tokio to 1.34.

Currently, the newest tokio version is 1.38, but some of the integration
tests are eaten when testing with this specific verstion of tokio.
Which is why, as of now, we decided not to support this version.

The issue with version 1.38 is related
to #[tokio::test] and #[ntest::timeout] attributes.
Refs:
- tokio-rs/tokio#6610
- becheran/ntest#28
- tokio-rs/tokio#6497
muzarski added a commit to muzarski/scylla-rust-driver that referenced this pull request Jun 12, 2024
We decided to bump minimum required version of tokio to 1.34.

Currently, the newest tokio version is 1.38, but some of the integration
tests are eaten when testing with this specific verstion of tokio.
Which is why, as of now, we decided not to support this version.

The issue with version 1.38 is related
to #[tokio::test] and #[ntest::timeout] attributes.
Refs:
- tokio-rs/tokio#6610
- becheran/ntest#28
- tokio-rs/tokio#6497
muzarski added a commit to muzarski/scylla-rust-driver that referenced this pull request Jun 12, 2024
We decided to bump minimum required version of tokio to 1.34.

Currently, the newest tokio version is 1.38, but some of the integration
tests are eaten when testing with this specific verstion of tokio.
Which is why, as of now, we decided not to support this version.

The issue with version 1.38 is related
to #[tokio::test] and #[ntest::timeout] attributes.
Refs:
- tokio-rs/tokio#6610
- becheran/ntest#28
- tokio-rs/tokio#6497
muzarski added a commit to muzarski/scylla-rust-driver that referenced this pull request Jun 12, 2024
We decided to bump minimum required version of tokio to 1.34.

Currently, the newest tokio version is 1.38, but some of the integration
tests are eaten when testing with this specific verstion of tokio.
Which is why, as of now, we decided not to support this version.

The issue with version 1.38 is related
to #[tokio::test] and #[ntest::timeout] attributes.
Refs:
- tokio-rs/tokio#6610
- becheran/ntest#28
- tokio-rs/tokio#6497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants