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 runnable detection for #[tokio::test] #15157

Merged
merged 1 commit into from Jun 28, 2023
Merged

Conversation

HKalbasi
Copy link
Member

fix #15141

It is hacky, and it wouldn't work for e.g. this case:

use ::core::prelude;

#[prelude::v1::test]
fn foo() {
}

But it works for the tokio case. We should use the name resolution here somehow, and after that we should probably also get rid of the ast based test_related_attribute function.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2023
@Veykril
Copy link
Member

Veykril commented Jun 27, 2023

Why are we changing the is_test impl, can't we just bring back the use of test_related_attribute where it was removed?

We could also try a different idea. Fetch the source of the function (or whatever def), walk up the macro expansions its in and check if any of those comes from the test attribute. If yes, it's a test runnable. Like

/// Return whether this file is an include macro
pub fn is_include_macro(&self, db: &dyn db::ExpandDatabase) -> bool {
match self.macro_file() {
Some(macro_file) => {
let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id);
loc.def.is_include()
}
_ => false,
}
}

@HKalbasi
Copy link
Member Author

Why are we changing the is_test impl, can't we just bring back the use of test_related_attribute where it was removed?

I would like to get rid of the test_related_attribute function in future. It detects any proc macro with test in the start or end of name if I understand it correctly, which is bad. It looks like a pre macro expansion thing to me.

We could also try a different idea. Fetch the source of the function (or whatever def), walk up the macro expansions its in and check if any of those comes from the test attribute. If yes, it's a test runnable.

I guess we don't do macro expansion for the test attribute, do we? If we do, we can just check for #[rustc_test_maker] or emit some fictional attribute with a unique name and check for that.

@Veykril
Copy link
Member

Veykril commented Jun 28, 2023

Ah right, we are skipping the test/bench expansion as its just wasting memory.

@HKalbasi
Copy link
Member Author

Ah right, we are skipping the test/bench expansion as its just wasting memory.

Aha, that's reasonable. Is there a way to do name resolution on attributes without doing relying on macro expansion?

@HKalbasi HKalbasi force-pushed the tokio-test branch 3 times, most recently from dcc7607 to 97c48f9 Compare June 28, 2023 17:51
@HKalbasi
Copy link
Member Author

I'm going to merge this to fix the tokio issue sooner, so that we can procrastinate think about the ideal solution without pressure.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

📌 Commit 97c48f9 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit 97c48f9 with merge 5d4dd9f...

bors added a commit that referenced this pull request Jun 28, 2023
Fix runnable detection for `#[tokio::test]`

fix #15141

It is hacky, and it wouldn't work for e.g. this case:
```Rust
use ::core::prelude;

#[prelude::v1::test]
fn foo() {
}
```
But it works for the tokio case. We should use the name resolution here somehow, and after that we should probably also get rid of the ast based `test_related_attribute` function.
@bors
Copy link
Collaborator

bors commented Jun 28, 2023

💔 Test failed - checks-actions

@HKalbasi
Copy link
Member Author

@bors retry

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit 97c48f9 with merge 729f5d0...

bors added a commit that referenced this pull request Jun 28, 2023
Fix runnable detection for `#[tokio::test]`

fix #15141

It is hacky, and it wouldn't work for e.g. this case:
```Rust
use ::core::prelude;

#[prelude::v1::test]
fn foo() {
}
```
But it works for the tokio case. We should use the name resolution here somehow, and after that we should probably also get rid of the ast based `test_related_attribute` function.
@bors
Copy link
Collaborator

bors commented Jun 28, 2023

💔 Test failed - checks-actions

@HKalbasi
Copy link
Member Author

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

📌 Commit 7901538 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit 7901538 with merge ad434fc...

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing ad434fc to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Jun 28, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing ad434fc to master...

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit ad434fc into rust-lang:master Jun 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokio::test lense broken
4 participants