nrxus / faux

Struct mocking library for Rust
https://nrxus.github.io/faux/
MIT License
411 stars 14 forks source link

Implement Clone for MockStore #46

Closed orbwalk closed 2 years ago

orbwalk commented 2 years ago

In one of my projects I need to clone a struct that might be mocked. Doing this in a test currently results in a panic. This PR implements Clone on the MockStore, which allows us to remove the panic on a clone of MaybeFaux::Faux. I have tested it locally and it works in my codebase, but if these changes have any undesired behaviour in certain conditions please let me know and I might be able to fix it. Alternatively, I could wrap the Mutex in an Arc, which would make it cheaper to clone the store.

nrxus commented 2 years ago

Thank you for the PR!

Why exactly are you cloning the mock? I need to understand a little bit more about the context of your use case to know what the cloning of a mock should do.

With this PR the weird thing is that cloning a mock results in both mocks sharing the same stubs which could cause weird action at a distance errors:

let mut foo = Foo::faux();
when!(foo.bar).once().then_return(3);

let cloned = foo.clone();

let x = cloned.bar();
let y = foo.bar(); // <~~~ explodes because the call to `cloned.bar()` used up the "once" mock. 

When I think of .clone() I think of a deep clone unless otherwise clear that it isn't (i.e., cloning an Rc or Arc). In this case however it is only a shallow clone because the mocks are still shared but it doesn't seem obvious. We can't clone the mocks without enforcing a Clone requirement on the stub closures which would add a limitation to mocking that is only there for the few cases you may want to clone a mock.

Another alternative would be that cloning a mock creates a new one without any stubs but then that clone would be useless since it would panic as soon as anyone tried to call any methods on it since it has no stubs... 🤔

Anyway, maybe this is the best faux can do since i agree the hardcoded panic isn't great I would just like to know the use case a little more since I have yet to want to clone a mock myself.

orbwalk commented 2 years ago

Thanks for the quick reply!

I'll try to give a short explanation of my use case. For a project I am working on I have a API struct that roughly looks like this:

struct Api {
  pool: Arc<Pool>,
  sessions: Arc<SessionStore>,
  user_id: Option<UserId>,
}

This struct is somewhat cheap to clone. Function on this API are called through warp, which is why I need to clone the struct. The routes look something like this:

fn with_api(api: &Api) -> impl warp::Filter {
  let api = api.clone();
  warp::any().and_then(move || {
    let api = api.clone();
    async move { Ok(api) }
  })
}

fn routes(api: &Api) -> impl warp::Filter {
  let foo = warp::path("foo")
    .and(with_api(api))
    .and_then(|api| async { api.foo().await });
  let bar = warp::path("bar")
    .and(with_api(api))
    .and_then(|api| async { api.bar().await });

  foo.or(bar)
}

In my test I'd like to mock Api and keep everything else as is, which is why I need to be able to clone it.

Now there are some ways around having to clone my Api in the first place, but none of them seemed great. I could for example have the Api be a simple wrapper that just calls some InnerApi with only associated functions, and mock that instead. But that doesn't seem great as I'd basically have to have a lot of duplicate functions. Another option would be to somehow have an instance that always exists so i can borrow it for 'static and pass &Api around.

This made me think that it would be easier to make mock stores clonable. But if it introduces undesired behaviour in other usecases of faux I don't mind closing this PR and exploring alternative options.

orbwalk commented 2 years ago

Actually, I've found a cleaner way to do it that doesn't require cloning anymore. Closing this PR as it doesn't seem like there's any other need for it other than my own. If that changes I'm happy to re-open and finish it! Thanks for the help.

nrxus commented 2 years ago

I've been thinking about this and I actually think the least surprising behavior in the happy case is what you have here so I am happy to merge it I'll just add some documentation before the next release explaining the behavior.

Until then a workaround if you still need to clone is to implement Clone manually rather than through derive and then mock it as you would any other method.

nrxus commented 2 years ago

Actually would you mind wrapping the Mutex in an Arc?

Right now there is a weird behavior where two clones share a mock but only if the method was mocked prior to the cloning. Stubbing a not-yet-stubbed method would be completely independent in the clone which I think would be confusing. They should either all be shared or all be independent and since we cannot make them all be independent then they should all be shared by having an Arc around the Mutex. A test for it would also be great but I can always add that later so no biggie on that.

orbwalk commented 2 years ago

Sure, I'll get on that!

orbwalk commented 2 years ago

I wrapped the Mutex in an Arc and fixed the test. If you'd like to see any additional changes, let me know!

nrxus commented 2 years ago

I'll add some docs and get a release out in the next few days. Thank you for the PR!

orbwalk commented 2 years ago

Awesome, thanks!