Expected calls matching several actual calls - (3) Implementation of "multi-matching" expectedCalls. #1023

jgonzalezdr · Jul 14, 2016

This PR adds at least the initial implementation of multi-matching expected calls, but only for a fixed number of expected calls.

The basic idea is simple: An actual call is not considered to be made until checkExpectations() is called on it (i.e. another actual call has been created or checkExpectations() is called on the mock support), therefore callWasMade() is not called on the matching expected call until there is only a single one selected (i.e. the one which fulfills the actual call). When this occurs, the corresponding count of matched actual calls on the expected call is increased.

The drawback of this is that actual calls order is not registered properly, so for the meantime
failure messages just show the expectations that were fulfilled in
declaration order, as it has no sense anymore to order them by call
order. This will be corrected in subsequent PRs.

Now a single expectedCall object can match multiple actual calls. The method expectNCalls is not handled any more as just a collection of N expectedCall objects, but just a single expectedCall that requires exactly N calls to be considered fulfilled. Strict ordering checks are not working for the moment, except when all expected calls are for exactly 1 call (e.g. only using expectOneCall). Actual calls order is not registered properly, so for the meantime failure messages just show the expectations that were fulfilled in declaration order, as it has no sense anymore to order them by call order. Composite calls are not needed any more, they have been disabled by pre- compiling them out (#if 0) along with their tests.

coveralls · Jul 14, 2016

Coverage decreased (-0.003%) to 99.872% when pulling 5ac48ad on jgonzalezdr:expected-calls-multimatch-7 into 33376c2 on cpputest:master.

jgonzalezdr · Jul 14, 2016

The decrease in coverage is just due to mathematics: Having deleted the composite calls, there are fewer lines of code, so the already uncovered ones gain a bit of weight.

basvodde · Jul 17, 2016

include/CppUTestExt/MockCheckedExpectedCall.h

    virtual ~MockCheckedExpectedCall();

    virtual MockExpectedCall& withName(const SimpleString& name) _override;
-    virtual MockExpectedCall& withCallOrder(unsigned int callOrder) _override;
+    virtual MockExpectedCall& withCallOrder(unsigned int callOrder) _override { return withCallOrder(callOrder, callOrder); }


Can you avoid inline implementations, as they have caused problems in the past (on some platform)

It's just temporary, in subsequent PRs I clean this up, removing that superfluous call.

But I can fix that anyway for the meantime if you want.

@basvodde would you like to give more information about the unexpected behavior for inline function :)? I have also met unexpected behavior on one TI's platform but can't dig out the reason

I actually do not know. I just know that @arstrube removed all inline functions over time :)

Maybe @arstrube could give his opinion on this. Shall I fix that in this PR, or is it OK to leave it for a short while, taking in account that the next 2 PRs that I've planned are cleanup ones, where actually that superfluous method is removed?

@cuitoldfish I'm not sure if this is the reason why @arstrube removed inline implementations, but it might be just because, for testing, inline methods can't be mocked up.

In my projects I only accept inline implementations when these are only trivial calls to other overloaded methods for the same function (i.e. just adding default values or simple transformations of parameters). In these cases, it's fine doing it like that because it doesn't affect testability.

It's been a long time, and it had to do with the TI Cl2000 compiler. I can't recall what the problem was and, unfortunately, have no longer access to that compiler for testing. The only way to find out would be to look at old commit comments..... I can't completely rule out that it was unexpected runtime behavior, although I am fairly certain it wasn't.

@jgonzalezdr I think it's okay to leave it until the method is removed. There were worse problems for the Ti Cl2000 in the past and the only person who noticed them was me :)

@jgonzalezdr ...after many moons.

basvodde · Jul 17, 2016

This PR is rather huge. But, based on the comments, won't this cause a problem by not failing the test in the actualCall as soon as it is possible to fail it?

jgonzalezdr · Jul 17, 2016

The effect of delaying the callWasMade has no incidence on failures, because out of order calls actually don't make the tests fail immediately when a call is made out of order, that behavior hasn't changed at all: an out of order failure is triggered only when calling checkExpectations() on the mock support, and only if there isn't any other failure (like expected calls not fulfilled).

In fact, with these changes, the internal state of expected calls is more coherent: In the current implementation, when strict ordering is enabled, all the unfulfilled expected calls that match the actual call function name are marked as "out of order" if they don't match the order (i.e. normally all the calls except one), even if they are later discarded and not actually "called" at all. If the test doesn't fail it's not relevant, because finally all the calls will match and be marked as "in order"; and if the test does fail it's also irrelevant, because this information is not displayed to the user.

With the new implementation, the expected call is considered "out of order" only if it's actually called out of order, in other cases it's considered to be "in order".

jgonzalezdr · Jul 17, 2016

You may find it easier to review this PR if you review the individual commits:

The first one implements the usage of a single expected call object for multiple calls, but sets aside for the meanwhile strict ordered tests proper implementation.
The second one deletes the now unnecessary composite call class.
The third one fixes the strict ordered tests.
The fourth one adds some additional test to compensate some coverage loss due to indirect testing made by the composite call tests.

basvodde · Jul 17, 2016

Could you next time make the PRs still smaller :)

jgonzalezdr · Jul 17, 2016

Any way, from the users perspective, the behaviour of the framework is the same than before, except for the information displayed when a test fails.

jgonzalezdr · Jul 17, 2016

I would have loved to push a smaller PR, but sincerely I didn't manage to reduce this one without passing by an intermediate state where the framework is actually broken :-(.

jgonzalezdr · Jul 21, 2016

@basvodde: Do yo need any extra information that may help you review this PR? May I help in any way?

basvodde · Jul 23, 2016

For now, just need more time. Been trying to have a short vacation :)

jgonzalezdr · Jul 23, 2016

Then enjoy your vacation! ;-)

matthewpiatt · Dec 27, 2017

Any update on this stale PR? Still very interested in this major performance enhancement.

…ated by cpputest#1023

basvodde · Jan 7, 2018

Hi all.

Sorry for the delay on this one. I finally spend a couple hours checking this (and checking an alternative implementation). I guess the above implementation is pretty clean. Though the mocking code needs to be cleaned up a bit more in the future.

I'll merge this. Hope everyone can make smaller commits in the future. Sorry for being the bottleneck on this important change.

Thanks @jgonzalezdr !!

matthewpiatt · Jan 7, 2018

Woohoo! Better late than never. Thanks @basvodde

matthewpiatt · Jan 7, 2018

Woohoo! Better late than never. Thanks Bas!

…

On Sun, Jan 7, 2018 at 1:48 AM, Bas Vodde ***@***.***> wrote: Merged #1023 <#1023>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1023 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACGwY-vi-2QkJUjKbzWz2UkvfCza16cXks5tIJLpgaJpZM4JM26p> .

offa · Jan 7, 2018

Great news! Thanks @jgonzalezdr and @basvodde.

jgonzalezdr · Jan 12, 2018

Thank you @basvodde, I didn't see the merge until today! :-)

I'll do PRs as small as possible for pending work from #1018 so you can review them faster. ;-)

jgonzalezdr added 4 commits July 14, 2016 10:20

Removed MockExpectedCallComposite.

65c5329

Re-enabled strict ordering checks.

1824802

Improved unit tests for MockExpectedCall.

5ac48ad

basvodde reviewed Jul 17, 2016
View reviewed changes

offa mentioned this pull request Aug 1, 2016

Optimize the mock calls #22

Closed

offa mentioned this pull request Dec 15, 2016

Too many expectNCalls? #1071

Closed

matthewpiatt pushed a commit to matthewpiatt/cpputest that referenced this pull request Dec 27, 2017

Investigating possibility of sharing string pointers in mocks. Duplic…

5177842

…ated by cpputest#1023

basvodde merged commit f3563d5 into cpputest:master Jan 7, 2018

jgonzalezdr deleted the expected-calls-multimatch-7 branch February 21, 2018 04:19

Search code, repositories, users, issues, pull requests...

Expected calls matching several actual calls - (3) Implementation of "multi-matching" expectedCalls. #1023

Expected calls matching several actual calls - (3) Implementation of "multi-matching" expectedCalls. #1023

Uh oh!

Conversation

jgonzalezdr commented Jul 14, 2016

Uh oh!

coveralls commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonzalezdr commented Jul 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgonzalezdr Aug 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basvodde commented Jul 17, 2016

Uh oh!

jgonzalezdr commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonzalezdr commented Jul 17, 2016

Uh oh!

basvodde commented Jul 17, 2016

Uh oh!

jgonzalezdr commented Jul 17, 2016

Uh oh!

jgonzalezdr commented Jul 17, 2016

Uh oh!

jgonzalezdr commented Jul 21, 2016

Uh oh!

basvodde commented Jul 23, 2016

Uh oh!

jgonzalezdr commented Jul 23, 2016

Uh oh!

matthewpiatt commented Dec 27, 2017

Uh oh!

basvodde commented Jan 7, 2018

Uh oh!

matthewpiatt commented Jan 7, 2018

Uh oh!

matthewpiatt commented Jan 7, 2018 via email

Uh oh!

offa commented Jan 7, 2018

Uh oh!

jgonzalezdr commented Jan 12, 2018

Uh oh!

Uh oh!

coveralls commented Jul 14, 2016 •

edited

Loading

jgonzalezdr Aug 6, 2016 •

edited

Loading

jgonzalezdr commented Jul 17, 2016 •

edited

Loading