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

Derived/Auto traits on mocks #17

Closed
nrxus opened this issue Jan 8, 2020 · 8 comments
Closed

Derived/Auto traits on mocks #17

nrxus opened this issue Jan 8, 2020 · 8 comments

Comments

@nrxus
Copy link
Owner

nrxus commented Jan 8, 2020

What traits should the mocks support deriving?

aa18c66 added support for Sync/Send/Debug.

What about Eq, PartialEq, Ord, PartialOrd, Clone, Copy, Hash, Default?

@nrxus
Copy link
Owner Author

nrxus commented Jan 8, 2020

I feel fairly strongly that Hash, Ord/PartialOrd, and Copy are out. Those are traits that apply to data and mocking data is... weird. My philosophy of mocking is to mock behavior which wouldn't have a concept or Ord/Hash/Copy in my opinion.

I feel similarly but less strongly about Eq and PartialEq.

Clone is more gray. It also feel like it is meant for data but.... you could clone behavior (i.e., clone your HTTP client). But perhaps for those cases they would not be auto-derived?

Default... 🤷‍♂️

@sazzer
Copy link

sazzer commented Apr 8, 2020

I've found cases where I need to have structs that I can Clone. E.g. to use in Actix I need to either put my services into Arc<> wrappers, or have one clone per thread that Actix is running.

@sazzer
Copy link

sazzer commented Apr 8, 2020

It's worth pointing out that there is a workaround, albeit a bit naff, for Clone. Namely, do it by hand:

#[cfg(test)]
use faux;

#[cfg_attr(test, faux::create)]
#[cfg_attr(not(test), derive(Clone))]
pub struct UserService {
    db: Database,
}

#[cfg_attr(test, faux::methods)]
impl UserService {
  pub async fn new(db: Database) -> Self {
    Self { db: db }
  }
}

#[cfg(test)]
#[cfg_attr(test, faux::methods)]
impl Clone for UserService {
  fn clone(&self) -> Self {
    Self { db: self.db.clone() }
  }
}

@nrxus
Copy link
Owner Author

nrxus commented Apr 10, 2020

That makes sense. My concern with allowing auto-deriving for Clone is that I don't know what a mocked clone should do.

let service = UserService::faux();
when!(service.fetch).safe_then(/* do something*/)
let cloned_service = service.clone();

Ideally, I think, cloned_service would still have the mocked function, but that is unfortunately impossible since the mock closure does not necessarily implement Clone.

I could clear all the stored mocks but I fear that might be confusing.
I could panic! if a mock is called on a mocked instance, this sounds unideal but at least it's loud. With your current workaround, it wold essentially panic when cloning a mocked instance unless the clone method is mocked. So perhaps that's what the auto-derived version should do?

I might need to juggle this in my brain a bit and decide an okay path. Thanks for bringing this up though, and please let me know if you have any preferences or ideas (:

@najamelan
Copy link

najamelan commented May 4, 2020

A downside is that it won't work even if you don't need to use the traits in testing. What I mean is if you don't want to clone explicitly like in your example above, but the library code needs these traits to be implemented. It sure is a problem if the closure is does not implement Clone. The others can maybe just be passed through to the inner object? Maybe require the closure to be clone if the inner object has a clone derive?

#[cfg_attr(test, faux::create)]
#[ derive( Debug, Clone, PartialEq, Eq ) ]
//
pub struct WireFormat
{
	bytes: Bytes
}

This won't compile, but changing it the other way around won't compile either because attribute macros have to be before derive macros.

@nrxus
Copy link
Owner Author

nrxus commented Jul 20, 2020

I think a step forward, at least to allow things to compile without load-bearing impls, will be to have the mock implement clone, if the original struct does as well. Then have the mock panic if someone actually attempts to clone it. Not perfect but good enough.

I can always change the behavior (i.e., make it not panic) without it being a breaking change.

@nrxus
Copy link
Owner Author

nrxus commented Jul 22, 2020

this will be addressed in the next release: 0a0577e

I am holding releasing for a bit since I am updating the minimum stable rust version to 1.45.0

@nrxus
Copy link
Owner Author

nrxus commented Apr 3, 2021

I have also implemented Default in: 53ce22d

Closing. If there is need for other derivable traits those can be their own issues.

@nrxus nrxus closed this as completed Apr 3, 2021
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

No branches or pull requests

3 participants