Skip to content

mark tokio::test as test - #16768

Closed
PSeitz wants to merge 1 commit into
rust-lang:masterfrom
PSeitz:master
Closed

PSeitz wants to merge 1 commit into
rust-lang:masterfrom
PSeitz:master

Conversation

@PSeitz

@PSeitz PSeitz commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

this is to support the excludeTests option to also handle #[tokio::test]

this is to support the `excludeTests` option to also handle `#[tokio::test]`
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2024
Comment on lines +205 to +210
}) || self.iter().any(|it| {
it.path()
.segments()
.iter()
.zip(["tokio", "test"].iter())
.all(|it| it.0.as_str() == Some(it.1))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's the wrong way to fix this, we need to go up the expansion hierarchy to find the necessary test attribute when categorizing the references

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought that's too slow as mentioned here: #16441 (comment)

Although I would prefer to have a full filtered result which takes a little bit longer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, that was about general traversal which we do, we are missing traversing up the macro expansions still though. Seems that was overlooked in the PR. That won't make much of a difference I guess given most finds shouldn't be in macro calls anyways.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought it's related to tokio::test, but it's not:
#16768 (comment)

test_func();
}

#[tokio::test]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#[tokio::test] is already mark as test, cause it will be expand to #[::core::prelude::v1::test]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought it's not working because I saw a lot of references in tokio::test, but the problem with detection seems more general.

e.g. a vec![] is enough to break the current filter

struct Point {
    x: f64,
    y: f64,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        assert_eq!(2 + 2, 4);
        let vec = vec![Point { x: 1.0, y: 2.0 }];
        assert_eq!(vec.len(), 1);
    }
}

@PSeitz

PSeitz commented Mar 8, 2024

Copy link
Copy Markdown
Contributor Author

@Veykril @Young-Flash
I'm not familiar enough with rust-analyzer.

sema.to_def(&it) what does this do? The it_works function should appear with the func.is_test(sema.db) on the example below, so my theory would be that this look up fails, or the ancestors traversal does not work like expected.

fn is_name_ref_in_test(sema: &Semantics<'_, RootDatabase>, name_ref: &ast::NameRef) -> bool {
    name_ref.syntax().ancestors().any(|node| match ast::Fn::cast(node) {
        Some(it) => sema.to_def(&it).map_or(false, |func| func.is_test(sema.db)),
        None => false,
    })
}
struct Point {
    x: f64,
    y: f64,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        assert_eq!(2 + 2, 4);
        let vec = vec![Point { x: 1.0, y: 2.0 }];
        assert_eq!(vec.len(), 1);
    }
}

@Veykril

Veykril commented Mar 8, 2024

Copy link
Copy Markdown
Member

The problem is that the name_ref in is_name_ref_in_test is in a macro expansion, so when ascending the tree we will end it at the top of the expansion, but instead we'd need to switch to the caller of the macro and ascend that tree (and so on for nested expansions).

Using

pub fn ancestors_with_macros_skip_attr_item(
should work

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024
@Veykril Veykril closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants