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: append #[::core::prelude::v1::test] only if it does not exist #46

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kezhuw
Copy link

@kezhuw kezhuw commented 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 #35.

This pr depends on frondeus/test-case#143 and tokio-rs/tokio#6497.

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

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal! I am fine with the approach. Left a few nitpicks.

This pr depends on frondeus/test-case#143 and tokio-rs/tokio#6497.

Can we please wait until these are merged? I'd rather not include any temporary patches.

macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
@kezhuw
Copy link
Author

kezhuw commented Apr 20, 2024

@d-e-s-o Thank you for reviewing. I am good with the merge timing.

I have pushed with a fixup commit to solve your concerns. Also, I checked that there is no duplicated test runs for new test cases. cargo test with_existing runs 2 tests, cargo test with_append runs 7 tests including ones generated by test_case.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jan 13, 2025

It seems as if the tokio change has made it in and test-case can't get its act together. @kezhuw I think we use test-case only for exercising attribute parsing with special-sauce syntax. If you know of a different crate supporting similar non-standard syntax, then we can just switch existing tests away from test-case.

@kezhuw
Copy link
Author

kezhuw commented Jan 15, 2025

I think we use test-case only for exercising attribute parsing with special-sauce syntax.

Do you mean tests like this ?

#[test_log::test(test_case::test_case(-2, -4; "my test name"))]
fn with_inner_test_attribute_and_test_args_and_name(x: i8, y: i8) {
  assert_eq!(x, -2);
  assert_eq!(y, -4);
}

This pr does not depend on this. I can drop some tests so that we don't need to patch test-case crate.

@kezhuw
Copy link
Author

kezhuw commented Jan 15, 2025

I can drop some tests so that we don't need to patch test-case crate.

Those tests should pass once frondeus/test-case#143 published. Or I could #[ignore] and doc them.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jan 15, 2025

Right, I understand it's just a development thing and all that, but my point is that we don't want to loose the coverage nor do we want to depend on some unpublished snapshot (or add ignore broken tests, for that matter).

So the easiest way out that I can think of would be to just use some other proc macro that supports more or less arbitrary syntax in test-case's stead. That would then unblock this PR. That it would (presumably) break test-case interaction is something I can live with, given that at this point that crate seems to be abandoned and we cannot freeze the entire eco system because of that.

@kezhuw
Copy link
Author

kezhuw commented Jan 16, 2025

we don't want to loose the coverage

For test coverage, the following two methods are identical in test-log side except that it will fail without patch to test-case. Besides, tokio::test also accept attributes.

#[tokio::test]
#[test_log::test]
async fn with_append_test_attribute_and_async() {
  assert_eq!(async { 42 }.await, 42)
}

#[test_case::test_case(-2, -4)]
#[test_case::test_case(-2, -5)]
#[test_log::test]
fn with_append_test_attribute_and_test_args(x: i8, _y: i8) {
  assert_eq!(x, -2);
}

So the easiest way out that I can think of would be to just use some other proc macro that supports more or less arbitrary syntax in test-case's stead.

Let me figure out.

kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 16, 2025
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 16, 2025
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
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
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
`#[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
`#[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
`#[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.
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_log::test(test_case::test_case)] doesn't work with multiple cases
2 participants