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.

Sign up for GitHub

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

Jump to bottom

fix: improve compatibility among test proc macros #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: improve compatibility among test proc macros #143

wants to merge 1 commit into 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.

@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 kezhuw mentioned this pull request Apr 18, 2024
Merged
@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 kezhuw mentioned this pull request Apr 19, 2024
Merged
kezhuw added a commit to kezhuw/test-log that referenced this pull request Apr 19, 2024
@kezhuw
fix: append #[::core::prelude::v1::test] only if it does not exist
da8927c 
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.
@kezhuw kezhuw mentioned this pull request Apr 19, 2024
Open
@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.

kezhuw
kezhuw commented May 24, 2024
View reviewed changes
crates/test-case-macros/src/lib.rs
@@ -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

@kezhuw kezhuw force-pushed the test-proc-macros-cooperation branch from 7472801 to 4c248c5 Compare November 19, 2024 13:41
@kezhuw
fix: improve compatibility among test proc macros
ffa1f6f 
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, frondeus#146.
@kezhuw kezhuw force-pushed the test-proc-macros-cooperation branch from 4c248c5 to ffa1f6f Compare November 20, 2024 07:55
@kezhuw kezhuw mentioned this pull request Jan 15, 2025
Open
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 16, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
28e20ea 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
@kezhuw kezhuw mentioned this pull request Jan 16, 2025
Merged
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 16, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
e3045e4 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 17, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
9937853 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
la10736 pushed a commit to la10736/rstest that referenced this pull request Jan 17, 2025
@kezhuw @la10736
Append generated test macro so next test macros are aware of it
e0b735e 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
87b2fc2 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

It is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after rust-lang/rust#82419
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
61d7917 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

It is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after la10736/rstest#291
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
dc7620d 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

This is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after la10736/rstest#291
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
@kezhuw kezhuw mentioned this pull request Jan 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test-env-log used with test-case creates test duplicates
2 participants
@kezhuw @Darksonn