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-macros: Add before_each and after_each attributes. #696

Merged
merged 6 commits into from
Oct 3, 2022

Conversation

hdoordt
Copy link
Contributor

@hdoordt hdoordt commented Sep 13, 2022

Add before_each and after_each attributes to defmt-test-macros. These attributes define functions that are called before and after each test case respectively.

I use them myself to alter a pin state, signaling my power profiler whether an actual test is running, but this feature can be used in many other situations.

I have some doubt about (not completely, but mostly) duplicated code, but I'm not sure adding macros would make it any better. Any suggestions on cleaning that up are very welcome!

And of course, thanks to the Knurling team and all contributors for setting up a great set of crates!

@hdoordt hdoordt changed the title Add before_each and after_each attributes. defmt-test-macros: Add before_each and after_each attributes. Sep 13, 2022
@hdoordt
Copy link
Contributor Author

hdoordt commented Sep 14, 2022

I have no idea why my changelog entry is considered insufficient. Any help?

Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

Looks really good! I didn't try it out yet, and only read through it, but great work! I have some small nitpicks.

firmware/defmt-test/macros/src/lib.rs Outdated Show resolved Hide resolved
firmware/defmt-test/macros/src/lib.rs Outdated Show resolved Hide resolved
firmware/defmt-test/macros/src/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
error: this type must match `#[init]`s return type
Copy link
Member

Choose a reason for hiding this comment

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

Can we add which type it should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a great improvement, thanks. Do you have a suggestion on how to convert state to String here?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@hdoordt hdoordt Sep 20, 2022

Choose a reason for hiding this comment

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

Welp, I have to say I'm not convinced that there's no prettier way of printing the type name, but hopefully the last commit does what we want. It seems that type_name won't help here, as there's no way of passing the type parameter to it. We need to obtain the type name from syn::Type, to which I haven't found a nice solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Urhengulas How do you feel about the current version?

Copy link
Member

Choose a reason for hiding this comment

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

I like it!

@Urhengulas
Copy link
Member

Urhengulas commented Sep 16, 2022

@hdoordt said:

I have no idea why my changelog entry is considered insufficient. Any help?

The problem is that the CI expects you to change ./CHANGELOG.md, but you are, correctly, changing ./firmware/defmt-test/CHANGELOG.md.

@hdoordt hdoordt force-pushed the before_each branch 2 times, most recently from 9deea62 to c04017f Compare September 18, 2022 18:10
@hdoordt hdoordt force-pushed the before_each branch 2 times, most recently from d919ce0 to 1ef19d6 Compare September 20, 2022 17:34
@Urhengulas
Copy link
Member

Sorry for the delay. I was busy and didn't get around to take a second look at the PR. Thank you for your work!

bors r+

@hdoordt
Copy link
Contributor Author

hdoordt commented Oct 3, 2022

@Urhengulas thanks for having a look!

@bors
Copy link
Contributor

bors bot commented Oct 3, 2022

Build succeeded:

@bors bors bot merged commit f379ff8 into knurling-rs:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants