Display matching invocations alongside expectations #394

nitishr · 2019-10-24T19:06:36Z

As suggested in #178

floehopper · 2019-10-27T12:11:10Z

At first glance, this looks really promising. I'll try to do a full review as soon as I can, but it might not be for a couple of days. Thanks again for all your contributions - they are much appreciated! 😄

floehopper

I've now reviewed the commits up to and including the "DRY up tests" commit, i.e. I haven't looked in detail at the commits where you move invocations into cardinality. In general it looks great and I'm really grateful for all your work on this.

I've made a number of suggestions in inline comments. I've reviewed the changes commit by commit and it looks as if some of the inline comments have been hidden by GitHub, because it doesn't really respect the commits within the review process.

I also have a couple more general suggestions:

It would be nice to add unit tests for the new classes you've introduced, e.g. Invocation, RaisedException, etc. Presumably we could effectively move some of the existing unit tests into these new tests...?
It would also be nice if the #assert_invocations method you extracted in the acceptance test was less brittle i.e. did not assert the existence of other lines of the failure messages particularly details of the expected method. I can see that we need at least one test to ensure the invocations are displayed in the correct position within the other lines, but maybe that could be done separately...?

I'm sorry I haven't yet looked at the later commits, but given that I've suggested a bunch of changes, I wondered if it might be easier to get this first part of the PR merged as a separate PR and then tackle the latter commits in another PR. What do you think?

If you'd prefer, I'd be happy to address my suggestions myself, but unless I hear otherwise, I'll work on the assumption that you'll make the changes yourself.

lib/mocha/expectation.rb

test/acceptance/display_matching_invocations_alongside_expectations_test.rb

lib/mocha/expectation.rb

test/acceptance/display_matching_invocations_alongside_expectations_test.rb

lib/mocha/expectation.rb

test/acceptance/display_matching_invocations_alongside_expectations_test.rb

floehopper · 2019-11-04T12:41:30Z

Just to expand slightly on my review, I think the most pressing issues are handling the Expectaction#throws scenario and avoiding rescuing Exception.

Since an expectation can have multiple matching invocations, and an invocation can yield multiple times, we will need to remember the sequence of yields per invocation. Yields also don't correspond 1:1 with returned values or raised expectations. Therefore, remembering the results/effects of an invocation is best done by a different object.

This will allow the yields to be captured and remembered by the invocation

previously the expected arguments were being displayed here (using Expectation#method_signature), but now the actual arguments are displayed

The tests were helpful for test-driving, but not all are needed due to overlap

Less state to maintain. Array#size is O(1), so should be fine performance-wise.

expectations test extract the assert invocations method. Note that the method is a bit fragile as it assumes a certain fixture, without which the calling test would fail.

Cardinality and invocations go hand-in-hand. So, the target is to move invocations to Cardinality.

The target is to encapsulate it inside Cardinality in a tell-don't-ask style.

expected hadn't quite sounded right because it also included allowed

This helps us avoid rescuing an exception raised by code elsewhere in the Mocha code. Also, enables us to handle the missed scenario where Expectaction#throws is called, 'coz without it, we'd need a generic way to catch any thrown tag and that might be quite awkward.

floehopper · 2019-11-06T16:07:23Z

@nitishr I see you've done a bunch more work on this. Let me know if/when you'd like me to review anything. And in particular whether you're happy with the idea of only merging the first half of this PR as a first step...? I'm also happy to take it off your hands now you've done all the hard work if you're fed up with it!

nitishr

Not fed up. I've addressed all your commit-based comments except where it was minor and too painful for me to fix using fixup and rebase.

I'm now gonna try to address your two general comments:

unit tests for new classes
make assert_invocations less brittle

I don't expect to make any more changes to non-test code, so feel free to review now if you'd like or wait until I'm done with the above 2 points.

As for breaking the PR up, while I'm not opposed to it, I'm not too keen to do the (what feels to me like) git gymnastics required to split unless not splitting is actually making the review harder for you (or you don't approve of the later changes). I see that the PR could be seen as two logically distinct pieces of work, but the oppositve view seems equally fair to me.

I'll commit my test changes to the same branch/PR, and you could decide if you wanna review and merge it in one go or two (or discard the cardinality-related changes altogether).

test/acceptance/display_matching_invocations_alongside_expectations_test.rb

lib/mocha/expectation.rb

test/acceptance/display_matching_invocations_alongside_expectations_test.rb

checking that invocations follow 'satisfied expectations:' ensures the invocations are displayed in the correct position within the other lines

nitishr · 2019-11-06T22:36:50Z

OK, I think I'm now done with this PR. I've made assert_invocations less brittle. I think that assertion as well as the whole test file could be further improved, but I don't have any bright ideas atm.

As for unit tests for the new classes, I did write one for Invocation but decided to discard it because I thought it didn't do much more than to introduce an extra set of tests that're more coupled to the structure of the system than the acceptance tests. I understand that those unit tests would still be valuable to pinpoint the causes of failures more narrowly than the acceptance tests do. However, I think the risk of difficulty focusing on the cause of a failure isn't high enough to offset the cost of the unnecessary/redundant structural coupling between the implementation and unit tests.

Also, upon reflection, I'm now much more open to splitting into 2 PRs if it makes it easier for you to review or manage as I noticed there's still a number of commits for you to review. However, I'll leave it to you to decide.

floehopper · 2019-11-08T17:54:14Z

Cool. I've created a new version of the branch to which I've done the following:

Rebased against the latest master
Ran an interactive rebase executing rake test for each commit and fixing a small number of Rubocop violations and test failures
Minor tweaks/formatting of commit notes
Applied the remaining feedback to the relevant commits:
Squashed 6284acb ("don't assert existence of other lines of failure messages") into 26e4397 ("DRY up display matching invocations alongside expectations test")

I still haven't yet reviewed the later cardinality-related commits. I plan to tackle that next or at least decide whether to merge the earlier commits first.

floehopper · 2019-11-08T19:09:18Z

I still haven't yet reviewed the later cardinality-related commits. I plan to tackle that next or at least decide whether to merge the earlier commits first.

I've had an initial look at the remaining commits. I'm not yet 100% convinced that the invocations belong "inside" the cardinality, but I need to have a bit more of a think about it.

Rather than add review comments to this PR for the later commits, I've decided to get the first commits merged and then open a new PR with the remaining commits.

First bunch of commits from #394. Co-authored-by: Nitish Rathi <[email protected]>

floehopper · 2019-11-08T19:40:08Z

I've now merged the first set of commits in 00f0540.

floehopper · 2019-11-08T20:04:47Z

I've now opened a 2nd PR containing the other commits. Closing this one.

Also handles Expectaction#throws scenario. PR: #410 Second bunch of commits from #394. Co-authored-by: Nitish Rathi <[email protected]>

nitishr force-pushed the display-matching-invocations-alongside-expectations branch 5 times, most recently from feddf29 to 4dea01a Compare October 30, 2019 22:59

nitishr mentioned this pull request Oct 31, 2019

Expectation description ought to include the return value #126

Closed

This was referenced Nov 4, 2019

Consistently use assert_* methods #405

Closed

Consider passing instances of Invocation to ExpectationList methods #408

Closed

Consider reducing duplication between Invocation & UnexpectedInvocation #409

Closed

floehopper requested changes Nov 4, 2019

View reviewed changes

floehopper self-assigned this Nov 4, 2019

nitishr force-pushed the display-matching-invocations-alongside-expectations branch 2 times, most recently from 9dca773 to ff8c3a3 Compare November 5, 2019 22:11

nitishr added 5 commits November 6, 2019 05:19

display return values of invocations

7149c23

display raised exceptions of invocations

84a844c

Move yielding to invocation

24f9c11

This will allow the yields to be captured and remembered by the invocation

display yields of matching invocations

1ff477c

nitishr force-pushed the display-matching-invocations-alongside-expectations branch 2 times, most recently from e056a78 to f8e640a Compare November 6, 2019 05:46

nitishr added 2 commits November 6, 2019 07:28

display arguments of matching invocations

1976f01

previously the expected arguments were being displayed here (using Expectation#method_signature), but now the actual arguments are displayed

Reduce granularity and redundancy of tests

de777f3

The tests were helpful for test-driving, but not all are needed due to overlap

nitishr force-pushed the display-matching-invocations-alongside-expectations branch from f8e640a to 9391c7e Compare November 6, 2019 07:29

nitishr added 5 commits November 6, 2019 10:36

Replace invocation_count with invocations.size

4b133ec

Less state to maintain. Array#size is O(1), so should be fine performance-wise.

add test for yielding & returning nothing

90c57c3

DRY up display matching invocations alongside

26e4397

expectations test extract the assert invocations method. Note that the method is a bit fragile as it assumes a certain fixture, without which the calling test would fail.

Prep to move invocation size based logic to Cardinality

78cfb20

Cardinality and invocations go hand-in-hand. So, the target is to move invocations to Cardinality.

Move invocation size based logic to Cardinality

41142f7

Cardinality and invocations go hand-in-hand. So, the target is to move invocations to Cardinality.

nitishr added 14 commits November 6, 2019 10:36

Move invocations list to Cardinality

b8e07cf

The target is to encapsulate it inside Cardinality in a tell-don't-ask style.

Infer invocation count from invocations size

e7a9618

The target is to encapsulate it inside Cardinality in a tell-don't-ask style.

Encapsulate invocations inside Cardinality

2f7011a

rename methods to better describe intention

aa5ed0e

rename methods to better describe intention

1aa23bd

rename methods to better describe intention

f1922d4

expected hadn't quite sounded right because it also included allowed

interpolate rather than append simple expressions

77aa25e

use same times logic for non-zero configured & invoked

fd290ea

times is never invoked with 0

d5c7e5b

more expressive & concise conditionals & booleans

7051809

use consistent phrasing for configured & invoked times

21ac1f9

reduce test's knowledge of irrelevant details

0d57961

display thrown object

4e32c0c

nitishr force-pushed the display-matching-invocations-alongside-expectations branch from 9391c7e to 4e32c0c Compare November 6, 2019 10:36

nitishr commented Nov 6, 2019

View reviewed changes

nitishr added 2 commits November 6, 2019 21:58

don't assert existence of other lines of failure messages

6284acb

checking that invocations follow 'satisfied expectations:' ensures the invocations are displayed in the correct position within the other lines

no need for blocks when expectation doesn't yield

c061de0

floehopper added a commit that referenced this pull request Nov 8, 2019

Display matching invocations alongside expectations

00f0540

First bunch of commits from #394. Co-authored-by: Nitish Rathi <[email protected]>

floehopper mentioned this pull request Nov 8, 2019

Encapsulate invocations inside cardinality and handle Expectaction#throws scenario #410

Closed

floehopper closed this Nov 8, 2019

floehopper added a commit that referenced this pull request Nov 16, 2019

Encapsulate invocations inside cardinality

4470825

Also handles Expectaction#throws scenario. PR: #410 Second bunch of commits from #394. Co-authored-by: Nitish Rathi <[email protected]>

floehopper mentioned this pull request Nov 16, 2019

Display matching invocations alongside their respective expectations in error message #178

Closed

floehopper added the feature-request label Nov 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display matching invocations alongside expectations #394

Display matching invocations alongside expectations #394

nitishr commented Oct 24, 2019

floehopper commented Oct 27, 2019

floehopper left a comment

floehopper commented Nov 4, 2019

floehopper commented Nov 6, 2019

nitishr left a comment

nitishr commented Nov 6, 2019

floehopper commented Nov 8, 2019 •

edited

Loading

floehopper commented Nov 8, 2019 •

edited

Loading

floehopper commented Nov 8, 2019

floehopper commented Nov 8, 2019

Display matching invocations alongside expectations #394

Display matching invocations alongside expectations #394

Conversation

nitishr commented Oct 24, 2019

floehopper commented Oct 27, 2019

floehopper left a comment

Choose a reason for hiding this comment

floehopper commented Nov 4, 2019

floehopper commented Nov 6, 2019

nitishr left a comment

Choose a reason for hiding this comment

nitishr commented Nov 6, 2019

floehopper commented Nov 8, 2019 • edited Loading

floehopper commented Nov 8, 2019 • edited Loading

floehopper commented Nov 8, 2019

floehopper commented Nov 8, 2019

floehopper commented Nov 8, 2019 •

edited

Loading

floehopper commented Nov 8, 2019 •

edited

Loading