New "expect" DSL promoting safer prefixed verifications, fixes #198

[einholen]

With existing postfix verifications, it is easy to overlook a missing verification when method signatures are long, so was called postfixes can be hard to spot - and unfortunately, in a lot of cases it means that a test that should be failing starts to pass.

New DSL helps combat this situation by sort of going back to the roots of vanilla Mockito, where "verify" word is defined before the verified method signature. Macros help us solve the inconvenience of having to wrap the mock variable with parentheses.

I wanted to reuse as much code as possible, and in order to do that I needed to restructure IdiomaticMockitoBase a bit: I split it into IdiomaticStubbing + PostfixVerifications traits, so a proper idiomatic base can now be composed of IdiomaticStubbing + PostfixVerifications/PrefixExpectations

However, better naming still wanted for IdiomaticMockito2 🙂

A few examples comparing was called DSL with expect a call DSL:

foo.bar(*) was called                        <=>  expect a call to foo.bar(*)
foo.bar(*) wasCalled once                    <=>  expect one call to foo.bar(*)
foo.bar(*) wasCalled 3.times                 <=>  expect exactly 3.calls to foo.bar(*)
foo.bar(*) wasCalled 1.times.after(3.millis) <=>  expect (1.calls after 3.millis) to foo.bar(*)

foo.bar(*) wasCalled onlyHere                <=>  expect only call to foo.bar(*)

foo.bar(*) wasNever called                   <=>  expect no calls to foo.bar(*)

foo wasNever called                          <=>  expect no calls to foo
foo wasNever calledAgain                     <=>  expect noMore calls to foo
foo wasNever calledAgain(ignoringStubs)      <=>  expect noMore calls(ignoringStubs) to foo

So far I believe the version in this PR supports everything that existing DSL can.

My macros are a bit rusty, so I'm not 100% confident that all the pattern match cases are correct - but all existing tests do pass.

[einholen]

from here on it's basically a copy of IdiomaticMockitoTest with updated DSL

[ultrasecreth]

Should we split the test and create a "Stubbings" one to avoid copy/pasting all the first part? Basically to also follow the split you made in the code :)

[einholen]

Good idea!
It might be hard to split them cleanly though, because some stubbing tests seem to rely on verifications too...

[ultrasecreth]

We could start with the ones that require no verification and then review the rest, or just pick one verification syntax, or even use the traditional one via the Mockito object so we don't favour a specific syntax.
Maybe that could be a different PR, for this one I think that with the README changes and a minor bump on the version.txt we should be good to go, being a new feature we have little risk of breaking exisiting clients.

[Krever]

Nice, this makes errors harder to make indeed. Some opinion below but feel free to ignore :)
I will be hesitant to use this DSL due to its verbosity. I know it's in the very spirit of scalatest, but for me, there are too many redundant words in there. E.g. expect a call to foo.bar(*) I would rather see as expect foo.bar(*) or even better, as a block:

expect {
  foo.bar(*)
  foo.baz(*)
}

At the same time, I'm not sure how to join this with more detailed expectations (like once), so treat it as food for thought at best.

[einholen]

@Krever thanks for looking at it and sharing the feedback!

I agree that it ended up a bit verbose. I originally tackled this initiative with something like expect 1.call to foo.bar in mind, but quickly realized that there's no way to make the short version fluent: the entire verify/expect expression has to have an odd number of total terms, with the actual mock at an odd position too - otherwise it won't form into a fluent DSL (expect.a(call).to(foo.bar(*)) works in infix, but expect(1.call).to(foo.bar) doesn't, and neither does expect foo.bar(*)). An even number of terms would require wrapping some arguments into parentheses, and may thus end up unintuitive to use (similar to Scalatest's ... must be (expected) vs ... must not be expected). And lastly, even if we somehow come up with a good short pattern for exact number of calls, we have atLeast, atMost, only, ignoringStubs - I wanted to make the syntax usable and readable for all predefined cases as well as a generic case with arbitrary ScalaVerificationMode.

That said, if expect a call to foo.bar(*) is long/awkward to write, a slightly shorter expect (1.call) to foo.bar(*) is also supported 🙂 But overall, as you said, I feel it would still be very much in the spirit of scalatest and existing mockito-scala syntax, so a small change to get used to, and subjectively, reads communicates intent well.

As for the expect block - I thought about it originally (and it's in the ticket), but went with @bbonanno's suggestion about something more fluent.

I'm definitely open to changing it to whatever better/shorter/less verbose DSL we can find - as long as we can keep "expect/verify" context at the front.

[ultrasecreth]

@einholen Nice contrib! I like it already :)
I left a comment regarding test duplication, also if you could add some brief explanation and details on the README.md I'd say this would be ready to be merge and be tried out in the wild.

Sorry for the delay on feedback but been quite busy lately

[einholen]

@bbonanno do you have thoughts on a better name? I'm thinking maybe it shouldn't be a base trait for now, or maybe we can hide it in the companion object and call something like trait IdiomaticMockito.WithExpect?

[ultrasecreth]

I think I'd follow the spirit of Scalatest once again and leave them separated, and maybe deprecate IdiomaticMockito in favour of IdiomaticStubbing with PostfixExpectations or whatever the user wants to use, altough for people using warnings as errors it could be quite annoying (assuming they have a lot of tests to refactor), so not 100% about deprecating it in the code yet, but definetively express in the README what the new preferred way is.

[jozic]

it feels like this deserves a major version bump, especially if IdiomaticMockito is deprecated

[ultrasecreth]

it feels like this deserves a major version bump, especially if IdiomaticMockito is deprecated

@jozic That's why I wasn't planing to deprecate it (yet), but I agree, if we do such breaking change it will require a major version bump, maybe the best is to mark the new API as Experimental until a couple of people actually use it in a project (I found out myself that while using the lib I realised of a lot of problems or improvements to be made)

@einholen Do you agree? if so, can you do the small changes in the code (remove IdiomaticMockito2 and mark the new verification syntax somehow as experimental)?

With that and a README entry I'd be happy to merge it in and start the work towards mockito-scala 2.x.x

[einholen]

Yes, totally - I wouldn't wanna do the major version bump before we have the new syntax as experimental for a while. I'll push the proposed changes and the readme update within a couple days!

[einholen]

Apologies for the delay, I was traveling last weekend, and it was hard to find time to work on the PR amidst regular work and coronavirus panic.

@bbonanno I think I addressed your comments!

• bumped version.properties to 1.13.0
• added a version upgrade note + "Expect DSL" section in the main docs
• split IdiomaticStubbingTest from identical parts of verification tests (I believe more can be done for them, like maybe extend both from an abstract trait testing all common verifications?)
• provided a generic base IdiomaticMockito.WithExpect trait as an alternative to IdiomaticMockito
• provided a base scalatest.IdiomaticMockitoBase that has IdiomaticVerifications subtype

[ultrasecreth]

@einholen Nice work, let's release it to the wild :)

Solves #198