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

Implement Clone for MockStore #46

Merged
merged 3 commits into from
Feb 23, 2022
Merged

Implement Clone for MockStore #46

merged 3 commits into from
Feb 23, 2022

Conversation

orbwalk
Copy link
Contributor

@orbwalk orbwalk commented Feb 14, 2022

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
Copy link
Owner

nrxus commented Feb 15, 2022

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
Copy link
Contributor Author

orbwalk commented Feb 16, 2022

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
Copy link
Contributor Author

orbwalk commented Feb 17, 2022

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.

@orbwalk orbwalk closed this Feb 17, 2022
@nrxus
Copy link
Owner

nrxus commented Feb 18, 2022

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 nrxus reopened this Feb 18, 2022
@nrxus
Copy link
Owner

nrxus commented Feb 18, 2022

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
Copy link
Contributor Author

orbwalk commented Feb 18, 2022

Sure, I'll get on that!

@orbwalk
Copy link
Contributor Author

orbwalk commented Feb 23, 2022

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

Copy link
Owner

@nrxus nrxus left a comment

Choose a reason for hiding this comment

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

Thank you!

@nrxus nrxus merged commit 85693fd into nrxus:master Feb 23, 2022
@nrxus
Copy link
Owner

nrxus commented Feb 23, 2022

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

@orbwalk
Copy link
Contributor Author

orbwalk commented Feb 24, 2022

Awesome, thanks!

@orbwalk orbwalk deleted the clone_store branch March 4, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants