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 mocking foreign-defined traits #164

Closed
cramertj opened this issue May 24, 2019 · 6 comments · Fixed by #182
Closed

Support mocking foreign-defined traits #164

cramertj opened this issue May 24, 2019 · 6 comments · Fixed by #182
Labels
effort-13 feature Brand new functionality ✨

Comments

@cramertj
Copy link

Seems pretty easy-- probably just an extra annotation to indicate that the trait definition itself should be omitted from the generated code, and only the mocks should be kept.

@bash
Copy link
Member

bash commented May 25, 2019

Mocking foreign traits and by extension directly depending on a trait defined in an external crate (excluding crates in the same workspace) seems like a clear violation of the Dependency Inversion Principle, which states that one should depend upon abstractions and not details.

To quote Robert C. Martin (Clean Architecture, Frameworks are Details):

Don’t let frameworks into your core code. Instead, integrate them into components that plug in to your core code, following the Dependency Rule.

I believe this statement should also be applied to libraries in general.

@bash bash added the wontfix This will not be worked on label May 25, 2019
@cramertj
Copy link
Author

Huh? Why would it be bad to mock e.g. io::Read? Abstracting over that interface certainly wouldn't be bad practice.

@bash
Copy link
Member

bash commented May 27, 2019

You're absolutely right, I forgot about traits defined in std which are an exception anyways since "marrying" yourself to the std is considered an acceptable thing.

This was also mentioned on the Reddit thread by Matthias247:

If there is already a stable trait in another crate that defines the behavior - or especially if it's a stable std trait (like Read), then it should be fine to reuse this in a program with wrapping it in another layer. And often we have to create mocks for those. So being able to mock traits defined elsewhere would certainly be helpful.

@bash
Copy link
Member

bash commented May 27, 2019

Another point that was mentioned in the Reddit thread is being able to keep the "production" code free of testing annotations.

I think it's even helpful without talking about dependencies: I actually would prefer not to add any mocking related annotations directly to traits in my library, since that now moves testing considerations into my src, and maybe raises questions for other people what that thing is about.
Matthias247

I agree with the sentiment of this, however I don't like the fact that trait definitions would need to be maintained in two separate locations.

@bash bash added feature Brand new functionality ✨ and removed wontfix This will not be worked on labels May 27, 2019
@austinglaser
Copy link

I'll also throw this out there -- the "components that plug into your core code" need testing as well. And in many cases there's no feasible way to do that without a mock over the external interface they're wrapping.

@bash
Copy link
Member

bash commented Jun 24, 2019

I think something like serde's remote option would be a good idea:

#[mockable(remote = "io::Write")]
trait Write {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize>;

    fn flush(&mut self) -> io::Result<()>;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-13 feature Brand new functionality ✨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants