-
Notifications
You must be signed in to change notification settings - Fork 118
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
Implemented a test
attribute macro
#209
base: master
Are you sure you want to change the base?
Conversation
Nice that you copied from I'll admit I don't know the benefits of making the macro a separate feature. Maybe @Noah-Kennedy will chime in whether it is called for or not. If there were no benefit, I would prefer to see the features stream-lined. Don't change that, let's see what Noah things. He's been working with But back to the code. Can you apply the macro to the test in src/io/noop.rs so we can see it in action with this PR? If it works, then either all the tests that used tokio_uring::start could be changed in the same PR or not. They could be changed at a later time too. But at least one to show that it works as intended. |
@FrankReh the advantage of the feature is to avoid requiring the proc macro crate to be built unless a user actually needs it. |
Personally I'm of the opinion that this isn't worth doing unless we actually want to have and maintain our own |
Is defining that one macro really noticeable? Then there are probably a lot of other functions and structs we are defining that could be split into their own crates. And don't we want to use the test macro in the crate itself to make the test code less boiler-platy? And I thought I saw somewhere recently that you gave the thumbs up for such a test macro for tokio-uring? |
Procedural macros have to be in their own crate which must be built ahead of wherever they are used. This means that the proc macro crate and any dependencies must all be built before the compiler can start building the downstream crates which use the macros. This basically pushes back a bit when the user crates can get built, and potentially adds some expensive dependencies like syn. |
This is easy enough since pretty much all the code can be re-used. This is also what the main tokio project does so it wouldn't be pretty much any added burden. (Basically both macros do the same thing, just |
|
… with test attribute
I added I conciously left out niche features like passing a |
I think this is @Noah-Kennedy 's call now. And I would have preferred a PR that just was used on the no_op test, so that the git commit would show more easily where this new feature comes from - the use of the new feature is a bit of noise when trying to understand the underpinnings first. But I'll also leave that to Noah too. I still like it. Removing the boilerplate from the tests makes the tests much more readable and should help people understand tests more quickly which can only be a positive. |
What kept this from being merged? I just took a stab at it myself and was about to open a pull request when I saw someone already completed this... |
@Noah-Kennedy never answered so this PR kind of just faded away |
Imho, it would still be great to merge this. It reduces the amount of code to write new tests significantly and, thus, makes adding new features easier. |
Closes #181
This implements a naive
test
macro, which simply wraps the body of the function in::tokio_uring::start
.Opening as a draft because I'm not sure about the design and whether we'd want to also provide a convenience
main
macro.The current implementation is heavily inspired by the one found in tokio but severely simplified.