Check if actual calls exceed the expected number on mock object drop #472

vikz95 · 2023-03-21T17:54:13Z

This pull request will allow to check again if the actual number of function calls is greater than the expected during the mock object dropout.
So if the check executed at every call panics in a sub task, this additional check at object dropout will cause another panic and will make the test fail.

I was not able to run cargo test because I got this error:

error[E0601]: `main` function not found in crate `ffi`
  --> mockall\examples\ffi.rs:35:2
   |
35 | }
   |  ^ consider adding a `main` function to `mockall\examples\ffi.rs`

asomers

This needs a CHANGELOG and a test case too.

asomers · 2023-03-21T18:12:48Z

mockall/src/lib.rs

@@ -1605,7 +1605,8 @@ impl Times {
    /// Has this expectation already been called the minimum required number of


You should update doc comment too, because you've changed the function's behavior.

asomers · 2023-03-21T18:13:24Z

mockall_derive/src/mock_function.rs

@@ -416,9 +416,9 @@ pub(crate) struct MockFunction {
    /// Types used for Predicates.  Will be almost the same as args, but every
    /// type will be a non-reference type.
    predty: Vec<Type>,
-    /// Does the function return a non-'static reference? 
+    /// Does the function return a non-'static reference?


Please do not mix formatting changes and content changes in the same commit.

asomers · 2023-03-21T18:18:42Z

mockall_derive/src/mock_function.rs

-                               desc,
-                               self.times.count(),
-                               self.times.minimum());
+                        if (self.times.count() < self.times.minimum()) {


This is very confusing because you're checking times.count() here, and also implicitly up above in is_satisfied. It makes the code hard to read. You should only do that check on one level.

Could you elaborate on this point?

Now is_satisfied checks both expected boundaries of the actual number of calls and returns a boolean.
From a boolean I'm not able to determine what error message to print in the panic, this is why the second check.

Instead of a boolean I could return an Enum with values: SATISFIED_LOWER_BOUND, SATISFIED_UPPER_BOUND, SATISFIED, NOT_SATISFIED.
Then I could differentiate the cases. But I don't know if this is what you had in mind...

You could do something like that, returning an enum. Or you could inline the is_satisfied logic into the proc macro. Either could work. I just don't want that logic to be repeated in two places.

asomers · 2023-03-21T18:21:14Z

I was not able to run cargo test because I got this error:

You seem to be using Windows, and as you can see that example is empty on Windows. Would you like to submit a separate PR to fix it?

vikz95 · 2023-03-21T20:39:31Z

Created a new pull request for the test failure on Windows: #473

I'll fix the other issues tomorrow.

vikz95 · 2023-03-22T21:13:39Z

mockall_derive/src/mock_function.rs

-                               self.times.count(),
-                               self.times.minimum());
+                    if !::std::thread::panicking() {
+                        match self.times.is_satisfied(); {


will remove ; in next commit

Why cargo build doesn't catch there errors?
How should I compile?

Well, if you just did cargo build, that would build the proc macros and the small amount of code in the mockall crate, but it wouldn't evaluate any of the proc macros, anywhere. You should do cargo test before pushing.

vikz95 · 2023-03-22T21:23:23Z

If the implementation with enums is ok, then I will create a test where the panics are raised in a subthread and then assure that checkpoint() makes the test fail.

I also need to figure why now cargo test causes so many errors, maybe it's due to the new enum:

error[E0433]: failed to resolve: use of undeclared type `ExpectedCalls`                                                                                                                
 --> mockall\tests\automock_attrs.rs:7:1
  |
7 | #[automock]
  | ^^^^^^^^^^^ use of undeclared type `ExpectedCalls`
  |

Update: should have used: ::mockall::ExpectedCalls

asomers · 2023-03-22T21:30:31Z

If the implementation with enums is ok, then I will create a test where the panics are raised in a subthread and then assure that checkpoint() makes the test fail.

No need to get all fancy with threads. Just use catch_unwind.

I also need to figure why now cargo test causes so many errors, maybe it's due to the new enum:

error[E0433]: failed to resolve: use of undeclared type `ExpectedCalls`                                                                                                                
 --> mockall\tests\automock_attrs.rs:7:1
  |
7 | #[automock]
  | ^^^^^^^^^^^ use of undeclared type `ExpectedCalls`
  |

Because you forgot to import that type in the generated code. Best to refer to it by its absolute path, ::mockall::ExpectedCalls

asomers · 2023-03-22T21:46:56Z

mockall_derive/src/mock_function.rs

+                                    self.times.minimum());
+                            },
+                            ::mockall::ExpectedCalls::SatisfiedOnlyLowerBound => {
+                                panic!("Exceeded number of expected calls panic propagation");


What's this about panic propagation? A better message would be:

Suggested change

panic!("Exceeded number of expected calls panic propagation");

panic!("{}: Expectation({}) called {} time(s) which is more than the permitted {}",

#funcname,

desc,

self.times.count(),

self.times.maximum() - 1);

This message is already printed by the panic in the call() method.
So in a subthread scenario there will be 2 equal panic messages.
I wanted to differentiate the second one.

Or did you expected to remove completely the panic in the call() method? And have only one in drop()?

Will it be? The second message will only be printed if for some reason the user is ignoring panics. In that case, he'll need all the info in the second one.

when calls happen in subthread and are more than expected fitst there is the panic from the check in call() method.
but it's ignored by main thread.
then there is the panic from the check in drop() method that makes the test fail.

but even if the messages are the same and are printed 2 times in this scenario it's not a problem, so I changed the message as suggested.

asomers · 2023-03-22T21:48:06Z

mockall/src/lib.rs

+    Satisfied,
+    SatisfiedOnlyLowerBound,
+    SatisfiedOnlyUpperBound,


I find this nomenclature confusing, because it focuses on which bounds are satisfied, rather than which aren't, which is more interesting. I suggest

Suggested change

Satisfied,

SatisfiedOnlyLowerBound,

SatisfiedOnlyUpperBound,

Satisfied,

TooFew,

TooMany,

Fine, will update

vikz95 · 2023-03-22T21:51:40Z

Now that the tests failures are fixed I need to write the 3 test cases:

actual calls < than expected
actual calls = expected
actual calls > expected

No need to get all fancy with threads. Just use catch_unwind.

But if I don't use threads then in case 3 the call() method will panic, and not the drop().

asomers · 2023-03-22T22:26:12Z

Now that the tests failures are fixed I need to write the 3 test cases:
1. actual calls < than expected

2. actual calls = expected

3. actual calls > expected
No need to get all fancy with threads. Just use catch_unwind.

But if I don't use threads then in case 3 the call() method will panic, and not the drop().

That's what catch_unwind is for. It will catch the panic and allow the test to continue.

vikz95 · 2023-03-25T15:10:15Z

cargo doc is failing:

error: Unsupported in this context
  --> mockall/src/examples.rs:42:5
   |
42 |     /// An implementation of a trait on a mocked struct
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not document `mockall`

I don't know if it's due to my changes, and what is the issue.

Update: I see that the issue is also on master, so it's unrelated.

vikz95 · 2023-03-25T15:13:47Z

In the end I choose to use threads in the tests to reproduce the issue that I encountered, and because I don't know how to use catch_unwind() to ignore the panic in call() and fail on the panic in drop() when calls > expectation

I think having tests that check expectations with calls that happen in subthreads is useful, this pull request aims to fill a gap exactly for this scenario.

asomers · 2023-03-25T15:38:17Z

You should learn how to use catch_unwind. It's more direct than using threads. I'll ensure that the master branch works.

vikz95 · 2023-03-25T16:03:16Z

Ok, I used catch_unwind and tests are definetely simpler.

asomers · 2023-03-25T21:46:48Z

Ok, I fixed master. The problem was a change in the new syn-2.0.9 crate. Could you please rebase your PR now?

# Conflicts: # CHANGELOG.md

asomers · 2023-03-26T16:07:25Z

CHANGELOG.md

+- Before dropping the mock object an additional check will verify if the
+  actual number of times a method was called is above the expectation.
+  This is needed because if the original check, that happens at every call,
+  panics in a subthread, then the caller thread will ignore the panic and 
+  the test will succeed while should have failed. 


Or to put it much more simply:

Suggested change

- Before dropping the mock object an additional check will verify if the

actual number of times a method was called is above the expectation.

This is needed because if the original check, that happens at every call,

panics in a subthread, then the caller thread will ignore the panic and

the test will succeed while should have failed.

- The `checkpoint` method now works correctly even after a panic due to too many

method calls.

asomers · 2023-03-26T16:23:48Z

mockall/src/lib.rs

+    /// Has this expectation already been called a number of times above the minimum,
+    /// below the maximum, both cases, or neither?


Let me suggest some more natural English:

Suggested change

/// Has this expectation already been called a number of times above the minimum,

/// below the maximum, both cases, or neither?

/// Has this expectation already been called the expected number of times?

/// If not, was it too many or too few?

asomers · 2023-03-26T16:33:18Z

mockall/tests/check_expectations_on_object_drop.rs

@@ -0,0 +1,54 @@
+// vim: tw=80


You don't need a new test program for this. The reason that Mockall's tests are split into so many separate test programs is so that most of them will still compile successfully even if one does not. And when investigating a compile failure, it helps to narrow the search for the problem.
For this PR, you aren't changing mockall's syntax at all. So I suggest that you delete this file and delete the expectation_satisfied and too_few_calls methods, which are redundant with tests elsewhere. Then, move too_many_calls into tests/mock_struct.rs, in the checkpoint module.

asomers · 2023-03-26T17:22:02Z

Thanks for your contribution @vikz95 .

vikz95 · 2023-03-26T17:29:03Z

Thank you for the support and guidance.
Do you already know when a new version with the changes will be released?

asomers · 2023-03-26T17:34:16Z

I'm planning to make a patch release soon. But not a feature release, because I'm still not comfortable with the implementation of the concretize feature. I still haven't found a non-hacky way to do it.

This is usually not an issue, since the program should already have paniced as soon as it attempted the first method call beyond the expected limits. However, it can still be useful if the method call is made in a context where panics are ignored, but checkpoint() is called in a context where they are not.

Check if actual calls exceed the expected number on mock object drop

e4fa7e6

vikz95 requested a review from asomers as a code owner March 21, 2023 17:54

asomers requested changes Mar 21, 2023

View reviewed changes

vikz95 added 2 commits March 22, 2023 20:53

Merge branch 'asomers:master' into extend_panic_on_object_drop

c72ebd3

Addressed comments

76e7dd0

vikz95 commented Mar 22, 2023

View reviewed changes

Fixed test failures

3ae2074

asomers requested changes Mar 22, 2023

View reviewed changes

addressed comments and added tests

0658079

use catch_unwind instead of threads

d258ad0

Merge branch 'master' into extend_panic_on_object_drop

f8d21b4

# Conflicts: # CHANGELOG.md

asomers requested changes Mar 26, 2023

View reviewed changes

addressed comments

a53e258

asomers approved these changes Mar 26, 2023

View reviewed changes

asomers merged commit f4144a8 into asomers:master Mar 26, 2023

asomers mentioned this pull request May 12, 2023

checkpoint() fails to validate if function is called 0 times or never #463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if actual calls exceed the expected number on mock object drop #472

Check if actual calls exceed the expected number on mock object drop #472

vikz95 commented Mar 21, 2023

asomers left a comment

asomers Mar 21, 2023

asomers Mar 21, 2023

asomers Mar 21, 2023

vikz95 Mar 21, 2023

asomers Mar 21, 2023

asomers commented Mar 21, 2023

vikz95 commented Mar 21, 2023

vikz95 Mar 22, 2023

asomers Mar 22, 2023

vikz95 commented Mar 22, 2023 •

edited

Loading

asomers commented Mar 22, 2023

asomers Mar 22, 2023

vikz95 Mar 22, 2023

asomers Mar 22, 2023

vikz95 Mar 25, 2023

asomers Mar 22, 2023

vikz95 Mar 22, 2023

vikz95 commented Mar 22, 2023 •

edited

Loading

asomers commented Mar 22, 2023

vikz95 commented Mar 25, 2023 •

edited

Loading

vikz95 commented Mar 25, 2023 •

edited

Loading

asomers commented Mar 25, 2023

vikz95 commented Mar 25, 2023

asomers commented Mar 25, 2023

asomers Mar 26, 2023

asomers Mar 26, 2023

asomers Mar 26, 2023

asomers commented Mar 26, 2023

vikz95 commented Mar 26, 2023

asomers commented Mar 26, 2023

		@@ -1605,7 +1605,8 @@ impl Times {
		/// Has this expectation already been called the minimum required number of

-                                panic!("Exceeded number of expected calls panic propagation");
+                                panic!("{}: Expectation({}) called {} time(s) which is more than the permitted {}",
+                                    #funcname,
+                                    desc,
+                                    self.times.count(),
+                                    self.times.maximum() - 1);

		/// Has this expectation already been called a number of times above the minimum,
		/// below the maximum, both cases, or neither?

Check if actual calls exceed the expected number on mock object drop #472

Check if actual calls exceed the expected number on mock object drop #472

Conversation

vikz95 commented Mar 21, 2023

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Mar 21, 2023

vikz95 commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikz95 commented Mar 22, 2023 • edited Loading

asomers commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikz95 commented Mar 22, 2023 • edited Loading

asomers commented Mar 22, 2023

vikz95 commented Mar 25, 2023 • edited Loading

vikz95 commented Mar 25, 2023 • edited Loading

asomers commented Mar 25, 2023

vikz95 commented Mar 25, 2023

asomers commented Mar 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Mar 26, 2023

vikz95 commented Mar 26, 2023

asomers commented Mar 26, 2023

vikz95 commented Mar 22, 2023 •

edited

Loading

vikz95 commented Mar 22, 2023 •

edited

Loading

vikz95 commented Mar 25, 2023 •

edited

Loading

vikz95 commented Mar 25, 2023 •

edited

Loading