mockito icon indicating copy to clipboard operation
mockito copied to clipboard

verify(someMethod).called(0) fails with unintuitive error.

Open jonahwilliams opened this issue 8 years ago • 6 comments

I initially thought that verify(something).called(0) would be equivalent to verifyNever. Instead the first fails with a no matching calls error. I am not sure if this is a bug or not, but if called doesn't support values <= 0 then it should throw a better error, perhaps pointing a user to verifyNever.

Minimal example:

import 'package:test/test.dart';
import 'package:mockito/mockito.dart';

void main() {
  group('verify calls', () {
    test('verifyNever passes', () {
      var cat = new MockCat();

      verifyNever(cat.countLives());
    });

    test('verify .called(0) fails', () {
      var cat = new MockCat();

      verify(cat.countLives()).called(0);
    });
  });
}


class Cat {
  int countLives() => 9;
}

class MockCat extends Mock implements Cat {}

jonahwilliams avatar Jul 28 '17 21:07 jonahwilliams

I'd say that's a bug.

srawlins avatar Jul 28 '17 21:07 srawlins

Ack, unfortunately, verify(...).called(0);would require a breaking change ☹️ .

When you call verify or verifyNever, we don't require any further methods to be called on the resulting object (a _VerifyCall). This is common with verifyNever(...);, but not very common with verify(...);, which basically (as you have learned the hard way), expects that at least one invocation has been logged. Users may depend on this behavior today.

In further detail: let's say you call verify(something).called(0);. While executing the verify(...) bit, we wish we could see that in the future, called() will be called, but we can't see the future. Also, when verify sees that no invocations have been logged, it calls package:test's fail method, which immediately throws. There's no way to reach that called() call, without changing the current behavior.

Now, I'm all for changing the behavior 😃 , as I don't think many people rely on verify(...);, but I'll discuss with a few folks on the versioning, etc. In the mean time, I'll clean up the messaging, as you suggest.

srawlins avatar Jul 31 '17 14:07 srawlins

@jonahwilliams how about the following breaking change for Mockito 3:

  • Calling verify(a); does not verify anything about the call count of a.
  • Calling verify(a); followed, at some point, by a call to when or verifyX throws an error, stating that a previous verify call was made without a subsequent call to called(). (But if verify(a); is the last statement in a test, it just silently does nothing; I don't think the test framework provides us with any APIs to register hooks at the end of a test. [1])
  • The argument to called can become optional, so that verify(a).called() just sort of, verifies that a was called more than 0 times. This provides the same functionality as the old verify(a); (which was never documented), but I think is easier to read for maintainability; someone unfamiliar would not be expected to guess what verify(a); does, but could make an educated guess at what verify(a).called(); does.

[1] The single downside to this change that I can see is that verify(a); becomes just a silent no-op, which might be surprising. We could be more heavy-handed and register a expectAsync0 when verify is called, and then call the expected callback in called(), but this seems heavy handed...

srawlins avatar Apr 23 '18 16:04 srawlins

If verify(a) does nothing by itself, why not combine it with the expectation? You could arrange the naming so that it corresponds with the current state. For example,

verify(a, called: 1) or verify.called(a)/verify.never(a)

That said, I think the proposed changes are perfectly good. We don't throw an error if a user writes a test case without an expect, so I wouldn't expect verify to have it's own error prevention here.

jonahwilliams avatar Apr 23 '18 17:04 jonahwilliams

@srawlins Is this going to happen for Mockito 3? If not, do you either want to:

  • Milestone it for Mockito 4
  • Close the issue as not planned

matanlurey avatar Jul 08 '18 19:07 matanlurey

I found a case where verify(someMethod) is called without .called(n):

var verification = verify(someMethod);
expect(verification.captured[0], equals('hey'));
expect(verification.captured[1], startsWith('hi ho'));

So I don't think we can make the changes I proposed, except I'd like to still make the argument to called() be optional:

the argument to called can become optional, so that verify(a).called() just sort of, verifies that a was called more than 0 times.

srawlins avatar Jul 09 '18 03:07 srawlins