Provide clear_* and clear_and_expect_* mock methods to enable clearing the expectations array for automock#676
Provide clear_* and clear_and_expect_* mock methods to enable clearing the expectations array for automock#676nobodie wants to merge 1 commit intoasomers:masterfrom
Conversation
…g the expectations array for automock. Partially addresses issue asomers#283
|
@asomers any chance you've gotten to review this? This would be a really useful addition to the library. |
asomers
left a comment
There was a problem hiding this comment.
Thanks for the PR. This looks like a good feature, and I intend to merge it. Sorry that I've been so tardy on reviewing it.
In addition to my inline comments, I have a few general ones:
- You should add a test for clearing the expectations on a static method
- You should add tests for clearing the expectations on a generic method. Does it clear all expectations, or only for a specific combination of generic arguments?
clear_and_expectneeds tests for ref and refmut methods. Unless, as I suggested inline, you simply delete it.- You should add a test that clearing an expectation with a
.times()requirement does not cause a subsequent checkpoint to fail, if the mocked method was never called. - Please add documentation. I think this feature deserves a section in the crate-level documentation in mockall/src/lib.rs, just below the section on Checkpoints.
And have a happy MLK day.
| - Provide expectations clearing for automock. | ||
| Ex: Useful to configure a general mock behavior through a helper function, | ||
| and then specialize a single method by clearing it's initial behavior. | ||
| Partially addresses issue [#283](https://github.com/asomers/mockall/issues/283) | ||
|
|
There was a problem hiding this comment.
| - Provide expectations clearing for automock. | |
| Ex: Useful to configure a general mock behavior through a helper function, | |
| and then specialize a single method by clearing it's initial behavior. | |
| Partially addresses issue [#283](https://github.com/asomers/mockall/issues/283) | |
| ## [ Unreleased ] - ReleaseDate | |
| ### Added | |
| - Provide expectations clearing for a mock object's expectations. Useful to configure general mock behavior through a helper function, and then specialize a single method by clearing it's initial behavior. | |
| ([#676](https://github.com/asomers/mockall/pull/676)) | |
| #[test] | ||
| fn calling_foo_without_expectation_after_clear() { | ||
| let mut mock = MockSimpleTrait::new(); | ||
| mock.expect_foo() | ||
| .returning(|x| x + 1); | ||
| assert_eq!(5, mock.foo(4)); | ||
|
|
||
| mock.clear_foo(); | ||
| let result = panic::catch_unwind(|| { | ||
| mock.foo(4) | ||
| }); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Rust already provides us with a great way to assert that something panics. Let's use it.
| #[test] | |
| fn calling_foo_without_expectation_after_clear() { | |
| let mut mock = MockSimpleTrait::new(); | |
| mock.expect_foo() | |
| .returning(|x| x + 1); | |
| assert_eq!(5, mock.foo(4)); | |
| mock.clear_foo(); | |
| let result = panic::catch_unwind(|| { | |
| mock.foo(4) | |
| }); | |
| assert!(result.is_err()); | |
| } | |
| #[test] | |
| #[should_panic(expected = "@nobodie should fill in this string")] | |
| fn calling_foo_without_expectation_after_clear() { | |
| let mut mock = MockSimpleTrait::new(); | |
| mock.expect_foo() | |
| .returning(|x| x + 1); | |
| assert_eq!(5, mock.foo(4)); | |
| mock.clear_foo(); | |
| mock.foo(4) | |
| } |
| assert_eq!(6, mock.foo(4)); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Just like in automock_trait, you should use #[should_panic] here.
| pub fn clear(&self, modname: &Ident) | ||
| -> impl ToTokens |
There was a problem hiding this comment.
| pub fn clear(&self, modname: &Ident) | |
| -> impl ToTokens | |
| pub fn clear(&self, modname: &Ident) -> impl ToTokens |
| } else { | ||
| quote!() | ||
| }; | ||
| let docstr = format!("Clear the [`Expectation`]({}/{}/struct.Expectation.html) array, before creating one for mocking the `{}` method", |
There was a problem hiding this comment.
| let docstr = format!("Clear the [`Expectation`]({}/{}/struct.Expectation.html) array, before creating one for mocking the `{}` method", | |
| let docstr = format!("Clear the existing [`Expectation`]({}/{}/struct.Expectation.html) array, and add a new expectation for mocking the `{}` method", |
| .doc(false) | ||
| .format(); | ||
| let name = self.name(); | ||
| let clear_and_expect_intent = format_ident!("clear_and_expect_{}", name); |
There was a problem hiding this comment.
| let clear_and_expect_intent = format_ident!("clear_and_expect_{}", name); | |
| let clear_and_expect_ident = format_ident!("clear_and_expect_{}", name); |
| #must_use | ||
| #[doc = #docstr] | ||
| #(#attrs)* | ||
| #vis fn #clear_and_expect_intent #ig(&mut self) |
There was a problem hiding this comment.
| #vis fn #clear_and_expect_intent #ig(&mut self) | |
| #vis fn #clear_and_expect_ident #ig(&mut self) |
| .expect() | ||
| } | ||
|
|
||
| #v fn clear #ig (&mut self) |
There was a problem hiding this comment.
This looks like it will clear expectations for all combinations of generic arguments. Is that intended? I would think that it would be more useful to clear them for only one specific set of generic arguments.
| /// * `modname`: Name of the parent struct's private module | ||
| // Supplying modname is an unfortunately hack. Ideally MockFunction | ||
| // wouldn't need to know that. | ||
| pub fn clear(&self, modname: &Ident) |
There was a problem hiding this comment.
This method contains much duplicated code from the expect method just above, and from the clear_and_expect method just below. That's a shame. But all three methods are called from exactly the same places. What about combining them into a single method that generates code for three mock methods? That could save some SLOC.
| .map(|meth| | ||
| meth.clear(modname) | ||
| ).collect::<Vec<_>>(); |
There was a problem hiding this comment.
| .map(|meth| | |
| meth.clear(modname) | |
| ).collect::<Vec<_>>(); | |
| .map(|meth| meth.clear(modname)).collect::<Vec<_>>(); |
Partially addresses issue #283