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

regression: tokio::test in 1.38 eats my tests #6610

Closed
zeenix opened this issue Jun 3, 2024 · 6 comments · Fixed by becheran/ntest#28
Closed

regression: tokio::test in 1.38 eats my tests #6610

zeenix opened this issue Jun 3, 2024 · 6 comments · Fixed by becheran/ntest#28
Labels
A-tokio Area: The main tokio crate A-tokio-macros Area: The tokio-macros crate C-bug Category: This is a bug.

Comments

@zeenix
Copy link

zeenix commented Jun 3, 2024

Version

1.38

Platform

Linux zeeframework 6.8.10-300.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Fri May 17 21:20:54 UTC 2024 x86_64 GNU/Linux

Description

On upgrading from 1.37 to 1.38, my tests are suddenly just getting ignored. If I don't set RUSTFLAGS=-Dwarnings env, the tests are just not run at all (which is why the compiler is complaining about the functions being called from the tests, are unused).

@zeenix zeenix added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jun 3, 2024
@zeenix zeenix changed the title regression: tokio::test attribute macro eats my tests regression: tokio::test in 1.38 eats my tests Jun 3, 2024
@mox692
Copy link
Member

mox692 commented Jun 3, 2024

After 1.38, #[tokio::test] macro now embeds #[test] attribute most inner than the other macros. (See also #6497)

On the other hand, the timeout macro you are using doesn't seem to recognize the inner #[test] attribute:

// This won't be triggered.
#[timeout]
#[test]
fn greet() {...}

So, it seems that the combination of the tokio::test's change and the current behavior of the timeout macro has made your test broken..

// Before, your `greet` test will be expanded like
#[test]
#[timeout]
fn greet() {...}

// After 1.38, your `greet` test will be expanded like
#[timeout]
#[test]
fn greet() {...}

@mox692 mox692 added the A-tokio-macros Area: The tokio-macros crate label Jun 3, 2024
@Darksonn
Copy link
Contributor

Darksonn commented Jun 3, 2024

That's unfortunate. There's a good chance we'll have to revert this, as it is a breaking change.

@becheran Ignoring users of #[tokio::test], are you able to clarify whether the current behavior of #[timeout] #[test] is a bug in ntest?

@becheran
Copy link

becheran commented Jun 6, 2024

@Darksonn thanks for the hint. Hm... yea, the #[timeout] part in ntest seems broken now. But to be honest I don't really know how I can possibly fix this in the ntest lib. Is the #[tokio::test] macro now always putting every macro below it above the #[test] macro? If that is the case, does #[should_panic] work with #[tokio::test] or is this also broken? If it does, then I will have a look if I can do something similar in ntest.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 6, 2024

Yes, right now it always puts #[test] at the bottom. The motivation behind putting #[test] at the end is so that if you are using multiple test macros, then they are able to "see" each other and avoid adding multiple copies of #[test].

As for #[should_panic] and #[test], those attributes work in either order, so yes it still works with #[tokio::test]. My understanding is that if #[should_panic] comes first, then it just adds another copy of itself after all other attributes so that it comes after the #[test] macro. And the #[test] macro will remove #[should_panic] attributes that come later.

Looking over your #[timeout] macro, it looks like you are completely ignoring input.attrs. You should attach all of the attributes to the test function instead. Anything that comes after #[timeout] is completely ignored.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 6, 2024

In the case of this example from your docs:

#[test]
#[timeout(10)]
#[should_panic]
fn timeout() {
    loop {};
}

First, the #[test] macro is expanded. It removes the #[should_panic] attribute and generates a constant:

#[cfg(test)]
#[rustc_test_marker = "my_test"]
pub const timeout: test::TestDescAndFn = /* let's ignore this part */;

#[timeout(10)]
fn timeout() {
    loop {};
}

And then the #[timeout] macro gets to run. So the reason it works with #[should_panic] after #[timeout] is that by the time #[timeout] expands, there is no longer a #[should_panic] attribute after it.


If we consider this example:

#[should_panic]
#[test]
#[timeout(10)]
fn timeout() {
    loop {};
}

Then the #[should_panic] attribute runs first and expands to this:

#[test]
#[timeout(10)]
#[should_panic]
fn timeout() {
    loop {};
}

Which in turn expands as above.


Finally, this example does not work:

#[timeout(10)]
#[test]
#[should_panic]
fn timeout() {
    loop {};
}

Because here #[timeout] is expanded first, and it expands to this:

fn timeout() {
    fn ntest_callback() {
        loop {};
    }
    let ntest_timeout_now = std::time::Instant::now();
    let (sender, receiver) = std::sync::mpsc::channel();
    std::thread::spawn(move || {
        if let std::result::Result::Ok(()) = sender.send(ntest_callback()) {}
    });
    match receiver.recv_timeout(std::time::Duration::from_millis(10u64)) {
        std::result::Result::Ok(t) => return t,
        Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {
            ::std::rt::panic_fmt(
                format_args!(
                    "timeout: the function call took {0} ms. Max time {1} ms",
                    ntest_timeout_now.elapsed().as_millis(),
                    10u64,
                ),
            );
        }
        Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {
            ::std::rt::begin_panic("explicit panic")
        }
    }
}

The #[timeout] attribute does not produce any attributes in the output, so no further expansion happens.

With #[timeout] fixed, the output should look like this:

#[test]
#[should_panic]
fn timeout() {
    fn ntest_callback() {
        loop {};
    }
    ...
}

muzarski added a commit to muzarski/scylla-rust-driver that referenced this issue 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 issue 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 issue 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 issue 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
@mox692
Copy link
Member

mox692 commented Jun 21, 2024

It seems that the ntest(ntest_timeout) with a patch applied has been released, so I'll close this issue.

Please open a new issue if you encounter similar problems.

@mox692 mox692 closed this as completed Jun 21, 2024
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Also bumps the itertools + base64 versions, since they are trivial upgrades
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
rukai added a commit to rukai/scylla-rust-driver that referenced this issue Jun 24, 2024
This is fine since tokio-rs/tokio#6610 is
resolved via becheran/ntest#28 which has now
been released in ntest 0.9.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate A-tokio-macros Area: The tokio-macros crate C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants