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

Support returning future #400

Closed
archfz opened this issue Aug 8, 2022 · 14 comments
Closed

Support returning future #400

archfz opened this issue Aug 8, 2022 · 14 comments
Labels
question Usage question

Comments

@archfz
Copy link

archfz commented Aug 8, 2022

I am mocking a method that is async. The mocked method is called in context of a timeout (from tokio). I would like to add a delay to the mocked return value. Something like this:

port.expect_read()
   .times(1)
   .returning(async move |mut buf: &mut[u8]| {
       sleep(Duration::from_millis(61)).await;
       buf.write(&[0x00])
   });

I get the following error:

= note:     expected enum `std::result::Result<usize, std::io::Error>`
       found opaque type `impl futures::Future<Output = std::result::Result<usize, std::io::Error>>`

Maybe there is another way of doing this, but I haven't found anything useful. Especially in the mockall documentation. The upper described way was my natural first instinct to implement this.

@asomers
Copy link
Owner

asomers commented Aug 8, 2022

If you defined the method as "async fn", then the generated code expects your expectation function to provide an immediate value, and it will add the future::ready part. If however you need to return a future that isn't ready yet, then you'll have to define the method as a normal function, like this:

pub fn read(&self) -> impl Future<Output = io::Result<()>> {...}

See https://docs.rs/mockall/latest/mockall/#impl-future

@asomers asomers added the question Usage question label Aug 8, 2022
@archfz
Copy link
Author

archfz commented Aug 8, 2022

Ok I am confused. Am I expected to change the tested code return type just to be able to have mocked delays? Isn't the whole purpose of async keyword to not do this?

@archfz
Copy link
Author

archfz commented Aug 8, 2022

Could we maybe have two separate methods?

  • returning_async(...) // accepts only mocking async functions
  • returning() // accepts both async and non async, but always resolves immediately

This would make it more clear what is happening and how to gain back control. It would also not require hopefully adapting fundamental code for tests.

@asomers
Copy link
Owner

asomers commented Aug 8, 2022

Yes, you will need to change the function signature to accomplish what you want. No, we can't just add a different returning method, because it's an issue in the code generation that affects more than just that method. No, you aren't the only person who's ever been confused by this. But the existing behavior is actually convenient for most users, and making the behavior selectable would probably require adding some kind of attribute to the function to control Mockall's code generation. That, too, would be a form of changing the function signature.
If you really don't want to touch the original function signature, then you can use mock! instead of #[automock]. That won't affect the original code at all.

@archfz
Copy link
Author

archfz commented Aug 8, 2022

Maybe having another mock macro #[automock_async] which would then always require sending async for async methods?

@archfz
Copy link
Author

archfz commented Aug 8, 2022

Or maybe having a second argument on returning that could control the delay on futures, that might still be confusing for non async functions, but it would still be better than altering tested code for tests.

@daniel-savu
Copy link

In case it helps anyone, I've also run into this issue an was able to work around it by wrapping a call to the async fn into a sync fn that returns a BoxFuture: interlay/interbtc-clients@efd7071

@archfz
Copy link
Author

archfz commented Aug 9, 2022

@savudani8 Your solution uses the proposed alternative of using mock!. I would prefer to use #[automock] as there is way less boilerplate and duplication.

@archfz
Copy link
Author

archfz commented Aug 9, 2022

   pub fn read(&self) -> impl Future<Output = io::Result<()>> {...}

@asomers I've attempted to use this solution but since what I am #[automock]-ing is a trait I wasn't able to just change the code simply to what you've sent, due to issue https://doc.rust-lang.org/error-index.html#E0562

So basically looking at @savudani8`s solution I arrived at:

// From simply having:
#[async_trait]
pub trait Trait {
   async fn read(&mut self, buf: &mut [u8]) -> Result<usize>;
}
// To having:
pub trait Trait {
   fn read<'a>(&'a mut self, buf: &'a mut [u8]) -> BoxFuture<'a, Result<usize>>;
}

In the process I have:

  1. lost the ability to use async, and thus the async_trait package
  2. was forced to introduce lifetime due the argument being a reference
  3. introduced further dependencies on the futures crate

Now the above compiles and runs, but there is an issue with mockall

21 |     fn read<'a>(&'a mut self, buf: &'a mut [u8]) -> BoxFuture<'a, Result<usize>>;
   |                 ------------  --- ...is used here...
   |                 |
   |                 this data with lifetime `'a`...
   |
note: ...and is required to live as long as `'static` here
  --> src/utils/internal_serial_port.rs:19:18
   |
19 | #[cfg_attr(test, automock)]
   |                  ^^^^^^^^
   = note: this error originates in the attribute macro `automock` (in Nightly builds, run with -Z macro-backtrace for more info)

So in a sense I am forced to use mock! if I understand this correctly, to be able to specify 'static lifetime?

At this stage I would argue that mockall supporting #[async_trait] with #[automock] is incomplete, since if one wants simulate delays they would need to fallback on mock!. EDIT: Or even #[automock] supporting delays in the above case, since I don't even use #[async_trait] anymore.

@archfz
Copy link
Author

archfz commented Aug 9, 2022

So I got until this point.

mock! {
//...
fn read<'a, 'b, 'c>(&'a mut self, buf: &'b mut [u8])
    -> Pin<Box<dyn Future<Output = Result<usize>> + Send + 'c>>
        where 'a: 'c, 'b: 'c;
//...
}

// ...
port.expect_read()
    .times(1)
    .returning(|mut buf: &mut[u8]| Box::pin(async {
        buf.write(&bytes)
     }));

The issue is that this doesn't work:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/cash_registers/datecs_cash_register/datecs_cash_register_test.rs:41:44
   |
41 |               .returning(|mut buf: &mut[u8]| Box::pin(async {
   |  ____________________________________________^
42 | |                 buf.write(&bytes)
43 | |             }));
   | |______________^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined here...

Is this really impossible? If I understand correctly 'c needs to be static, 'c and 'b can be 'static, but 'a cannot be since that is the instance of the mock trait, and there is a relationship required that 'a lives as much as 'c

@ok300
Copy link

ok300 commented Aug 22, 2022

So basically looking at @savudani8`s solution I arrived at:

// From simply having:
#[async_trait]
pub trait Trait {
   async fn read(&mut self, buf: &mut [u8]) -> Result<usize>;
}
// To having:
pub trait Trait {
   fn read<'a>(&'a mut self, buf: &'a mut [u8]) -> BoxFuture<'a, Result<usize>>;
}

Have you tried

#[cfg_attr(test, automock)]
#[async_trait]
pub trait Trait {
   async fn read(&mut self, buf: &mut [u8]) -> Result<usize>;
}

I'm also mocking async calls and after fiddling with the order of derives, the specific way automock is imported, etc I got a similar structure to work for me.

#[cfg_attr(test, automock)]
#[async_trait]
pub trait NostrPowProviderTrait {
    async fn automine(&self, target_difficulty: u8, message: String) -> anyhow::Result<MinedNote>;
}

and in tests

#[tokio::test]
async fn automine_when_criteria_fit() {
    // ...
    p.expect_automine().times(1).returning(|_diff, _msg| create_random_mined_note());
} 

@archfz
Copy link
Author

archfz commented Aug 22, 2022

@ok300 I think you are missing the point. Automock will mock the method in a way that you cannot control a delay on the returned promise. It has 0 delay. I am trying to return in the mocked method call an actual future so I can delay it.

@asomers
Copy link
Owner

asomers commented Aug 22, 2022

@archfz "supporting #[async_trait] with #[automock] is incomplete" is truly only in the sense that #[automock] is an inherently incomplete solution. There's lots of stuff that it can't do. Mocking structs with multiple trait impls, for instance. It's handy when it works, but mock! is the more general solution.
As for your lifetime problems, returning's closure must be 'static, but in your case you are capturing bytes by reference. You need to use a move closure instead: move |mut buf: &mut[u8]| Box::pin(async { buf.write(&bytes) })).

@asomers
Copy link
Owner

asomers commented Dec 21, 2023

Closing this because the original question has been answered. BTW, see #189 for the enhancement request that would cleanly solve the problem.

@asomers asomers closed this as completed Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Usage question
Projects
None yet
Development

No branches or pull requests

4 participants