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

defmt-test: Modify attributes in place and handle #[cfg] #383

Merged
merged 2 commits into from
Feb 10, 2021
Merged

defmt-test: Modify attributes in place and handle #[cfg] #383

merged 2 commits into from
Feb 10, 2021

Conversation

jonas-schievink
Copy link
Contributor

Closes (?) #381 (but doesn't add #[ignore] support)

This changes the defmt-test macro to retain the ItemFns and selectively delete defmt-test-specific attributes from them. The result is that the code is now simpler and arbitrary Rust attributes can be attached to tests. Doing so with #[ignore] gives the expected "unused attribute" warning.

@jonas-schievink jonas-schievink added the pr waits on: review Pull Request is waiting on review label Feb 10, 2021
@jonas-schievink jonas-schievink changed the title Modify attributes in place and handle #[cfg] defmt-test: Modify attributes in place and handle #[cfg] Feb 10, 2021
format!(
"({}/{}) running `{}`...",
i + 1,
tests.len(),
Copy link
Member

@japaric japaric Feb 10, 2021

Choose a reason for hiding this comment

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

the number of tests that will run is not known in this context because cfg are evaluated after expansion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I changed it so it gets computed at runtime (or in a const) in adce0f3

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

The overall approach looks good to me but the log messages that report which test is run next should also be cfg-ed

@japaric
Copy link
Member

japaric commented Feb 10, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 10, 2021

Build succeeded:

@bors bors bot merged commit 1081084 into knurling-rs:main Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr waits on: review Pull Request is waiting on review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants