-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add support for async fn in proptest! #185
Add support for async fn in proptest! #185
Conversation
Ah unfortunately this obviously wouldn't work on 1.33, potentially could be feature gated but that seems undesirable. |
35b8236
to
598e8a4
Compare
Is there any update on this? It seems essential to use proptest in async code. |
598e8a4
to
9c70f6f
Compare
Adds cases for asynchronous functions with the proptest macros and a test case using tokio. Should work for both tokio::test and async_std::test attributes. Support for async closures not added as they are not available in the latest stable rust, but they are not supported by the test attributes currently regardless. Additionally as async/await syntax requires rust 1.39 sets minimum CI version to 1.39.
9c70f6f
to
70a6881
Compare
I think there were some problems with a dependency, but now the PR should hopefully pass CI. I wonder if this should be gated behind a feature flag to continue to allow people with pre-1.39 rust versions to use this crate? I could make that change if necessary. |
XAMPPRocky/remove_dir_all#26 looks like this is the reason it can't compile, I can bump rustc to 1.40, which should work, but it seems like quite a big departure for the project. edit: alternatively I can just not include the test? |
tempfile requires remove_dir_all which no longer supports rustc < 1.40.
IIUC this PR doesn't actually bump the MSRV for users of |
In other words - all of the non-test changes are in macros, so this new functionality is 'opt-in' for downstream users of |
(#![proptest_config($config:expr)] | ||
$( | ||
$(#[$meta:meta])* | ||
async fn $test_name:ident($($parm:pat in $strategy:expr),+ $(,)?) $body:block | ||
)*) => { | ||
$( | ||
$(#[$meta])* | ||
async fn $test_name() { | ||
let mut config = $config.clone(); | ||
config.test_name = Some( | ||
concat!(module_path!(), "::", stringify!($test_name))); | ||
$crate::proptest_helper!(@_BODY config ($($parm in $strategy),+) [] $body); | ||
} | ||
)* | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be possible to do this without the duplication by matching an optional async
keyword via $( async )?
, then outputting it in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind that's not a thing
proptest! { | ||
#[tokio::test] | ||
async fn test_something_async(a in 0u32..42u32, b in 1u32..10u32) { | ||
prop_assume!(a != 41 || b != 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add an await
call into the body this will fail to compile, even something like async { }.await
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From expanding the macro it looks like this is not actually adding the async
keyword in front of the function, but I'm not sure why. Annoying that I didn't spot this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I spent a bit of time trying to figure out how to fix, but I'm not familiar with that code so couldn't puzzle it out. seems like it should be straightforward, but macros are always harder than you think they'll be :)
I'm curious to know if there are any updates on this? Thanks a lot for the crate and for this PR! |
I've been looking at this PR again and I'm not sure it's possible to do without writing a lot of new capabilities for the runner to be able to handle async tests; I think my original idea was a bit naive and not really viable. I think if someone wanted to implement this feature the best way would be to add an Possibly this PR is just worth closing? |
To me it is fairly one deal breaker of proptest to not having async. You can obviously write in each test an ugly block async but it is inconvenient and breaks partially RLS ... |
I'm not sure that adding async support to the runner would be the ideal solution. With tokio at least, users might want to start a test with the time paused or a different runtime flavour. Async support could be added somewhat easily by adding something like the following to (@_ASYNC_BODY $config:ident ($($parm:pat in $strategy:expr),+) $(tokio_test_args ($($tokio_test_arg:tt)*))? [$($mod:tt)*] $body:expr) => {{
#[tokio::main$(($($tokio_test_arg)*))?]
async fn test_fn() -> Result<(), $crate::test_runner::TestCaseError> {
let _: () = $body;
Ok(())
}
$config.source_file = Some(file!());
let mut runner = $crate::test_runner::TestRunner::new($config);
let names = $crate::proptest_helper!(@_WRAPSTR ($($parm),*));
match runner.run(
&$crate::strategy::Strategy::prop_map(
$crate::proptest_helper!(@_WRAP ($($strategy)*)),
|values| $crate::sugar::NamedArguments(names, values),
),
$($mod)* |
$crate::sugar::NamedArguments(
_,
$crate::proptest_helper!(@_WRAPPAT ($($parm),*)))
| {
test_fn()
},
)
{
Ok(_) => (),
Err(e) => panic!("{}\n{}", e, runner),
}
}}; This should work for For the record: The reason just keeping the |
For anyone else waiting on this to be merged, below package seems to work to combine tokio with proptest: |
This got closed when i switched branch from |
Adds cases for asynchronous functions with the proptest macros and a
test case using async-std.
Should work for both tokio::test and async_std::test attributes.
Support for async closures not added as they are not available
in the latest stable rust (I think?), but they are not supported
by the test attributes currently regardless.
As this PR adds an extra dev dependency I'm willing to look at alternative
ways to test this change as the test is more for syntax than for functionality.
Should resolve #179