Expected calls matching several actual calls #1018
Conversation
…s that can match many actual calls (i.e. range of calls). The changes are for now just semantic: The nomenclature of "expectation fulfillment"-related methods has been changed, and also some methods have been duplicated and renamed, because expected calls will soon be able to fulfill (match) more than one single actual call, and therefore some of the internal state of expected calls will be devoted to check if the expected call matches the actual call that is being processed, and other state will check if the expected call is fulfilled as a whole (i.e. it has been called/matched a minimum number of times).
Now expectedCalls can match multiple actual calls, between a minimum and a maximum number of calls as defined by the user. 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. Other methods where also added to MockSupport to expect a range of calls (between a minimum and a maximum), at least a number of calls (i.e. minimum), at most a number of calls (i.e. maximum), and any number of calls. Strict ordering checks are not working for the moment, except when all expected calls are for exactly 1 call (i.e. only using expectOneCall). Actual calls order is not registered properly, so for the meantime failure messages just show the expectations that were fulfilled. Composite calls are not needed any more, they have been disabled by pre-compiling them out (#if 0) along with their tests.
…preference than fulfilled ones.
…lfilled ones have higher preference than fulfilled ones.
…ional calls (i.e. expectOneCall and expectNCalls are allowed, but not calls where the minimum and maximum number of calls are not equal).
…id sign mismatch errors when compiling with sign strictness.
… proper unsigned integers, and also removed unused method MockActualCall::withCallOrder (and its overridden counterparts), except in MockActualCallTrace, where it's been redefined and re-enabled.
…ulti-matching expected call with zero expected calls, thus avoiding the usage of a dedicated list to keep track of "unexpectations" and therefore simplifying the architecture of the MockSupport class. Additionally, MockUnexpectedCallHappenedFailure now reports, when expectations exists for a function, the additional call number of the unexpected call according to previous actual calls to the same function (instead of the max number of calls expected for that function, which could be misleading when optional calls are used).
…here not being used in production code.
… of using optional calls with strict ordering.
…f actual calls. The implementation has been chosen as a queue instead of a simple list to allow limiting the number of registered calls to the last ones, which is convenient when performing tests that involve a large number of calls in a target platform with limited memory resources.
… MockSupport instead of a reference to the list of expected calls, in preparation for the implementation of a registry of actual calls that will be stored in the MockSupport objects.
… to generate a representation of their contents as a string, in preparation implementing an actual calls log.
…tional addition of actual calls registry to MockSupport.
…ures can now report again the actual calls that actually happened in call order.
…ctual calls log. The actual calls log limit is inherited hierarchically between mock scopes.
|
For some reason, if you check the commits in this PR, some appear out of order, so be aware of this when reviewing them to avoid confusion. You can always review them instead in my own branch, where they appear to be in the proper order: |
|
Commit 1: (dae0892) Preparation of mock library... These modifications don't change or add any functionality, they just rename some methods and attributes, and add a couple of additional methods, in preparation for the actual implementation of multi-matching expected calls. The key concepts changed are about matching actual calls, and fulfilling expected calls. Until now, an expected call could match a single actual call, and when this match was done, then the expected call was fulfilled, so matching and fulfilling was rather the same thing. From now on, these two concepts will be separated: During the processing an actual call, expected calls will be able to match it under certain conditions (i.e. parameters and object match, the expected call hasn't reached the maximum number of actual calls that it can match, etc.). However, an expected call will be considered fulfilled when it has matched between its minimum and its maximum number of actual calls. This means that an expected call will be considered to be fulfilled as soon as it reaches its minimum number of matched actual calls, but that it will be able to match actual calls until it reaches its maximum. Additionally, notice that the state stored in expected calls relative to "matching" will be only relevant while an actual call is being processed, but the state stored relative to "fulfilling" will be relevant during the whole life cycle of the expected call. |
|
Commit 2: (ed56f39) Basic implementation of "multi-matching" expectedCalls These modifications implement the basic features, but they break for now strict ordered checks when using methods apart from expectOneCall. Additionally, the existing feature by which fulfilled expected calls are reported in call order (on failures) is also broken, because it has no sense now: As expected calls can match more than an actual call, and those could happen in any order, they are just reported in declaration order, and the actual calls order is not reported at all for now. |
|
Commit 3: (8ee564a) Added C language new calls for multi-matching expected calls These modifications are rather trivial. |
|
Commit 5: (e94cd93) Added test that checks that non-fulfilled calls have higher matching... These modifications ensure that expected calls that aren't yet fulfilled (i.e. not having reached the minimum number of matching calls) have higher precedence over expected calls that are already fulfilled (because they've reached their minimum), to avoid the latter being greedy. |
|
Commit 7: (162d117) Renamed MockExpectedCallsDidntHappenFailure... The rename was made to improve consistent nomenclature, as the new name better suits the actual failures that are handled by that class. |
|
Commit 17: (bc33181) Added functionality to MockCheckedActualCall and MockActualCallsQueue… Here the functionality to generate strings with the description of actual calls (including call order) has been added, which will be used later in the failure reports. While implementing this, a bug has been detected and fixed, that made printing of missing parameters appear improperly indented when a failure because of missing parameters was thrown and multiple expected calls were matching previously. |
|
Commit 18: (044a693) Added actual calls reporting to MockFailure, and preliminary… With these modifications the failure classes can now report the actual calls in call order. Take in account that at this point, the actual calls queue is always passed empty in production code. |
|
Commit 19: (b3a20a4) Fixed compilation for Dos platform. Trivial modification to fix compilation in travis ci. |
|
Commit 20: (eb3aae1) Implemented a log of actual calls in MockSupport… This finally implements the functionality to keep a log of actual calls in each mock object, effectively re-introducing the capability to print the actual calls in call order when a failure happens. |
|
Commit 21: (fb5653f) Added functionality to MockSupport to limit the maximum size… This commits implement the functionality to limit the size of the actual calls log. This feature is essential when performing integration or simulation tests that perform lots of actual calls in embedded target platforms with scarce memory, but it's also very useful in other cases to avoid saturating the failure report. In fact, I'm wondering if it would be convenient to have a default limit value like 20 or so, because in the vast majority of cases that should provide enough information to the user, and in the rest of cases he can just change that limit as needed for individual tests. |
|
Commit 23: (faf07f9) Added unit test for MockActualCallsList::hasFinalizedMatchingExpectat… Just added some additional trivial unit tests to improve coverage. |
|
Wow, what happened to small pull requests :) This thing is huge. First question. In which scenario would you want to use expectAtLeastOneCall (or any of them) ? |
|
I know the PR is huge, the problem is that it's an all or nothing thing, because the different "pieces" are useless alone and moreover, they would break some capabilities of the current implementation. Regarding the family of "expect call methods", I'll admit that expectOneCall, expectNCalls, expectAnyCalls and expectNoCalls are the most useful ones, especially with simple white-box unit tests, but the other ones are useful when performing TDD or for more complex integration tests written according to a behavior specification (black-box testing) and not according to the actual implementation (white-box testing). I'll put an example based on a real-world case: We have a Configuration class that manages the configuration for a device stored in EEPROM, and it has a method "bool IsFeatureXActivated()" that returns a boolean indicating whether a certain feature is activated. We have a class DigitalOutputs with a method "void ActivateOutputY()". We have a class SoundManager with a method "void PlaySound(enum...)". Then we have a function "void OnEventZ()" to be tested which behavior is specified as, among other things, "if feature X is activated, then the output X shall be activated and a long beep sound shall be played". We could write a test like that: This test seems fine, but it will fail if the implementation does something like: It fails because conceptually it's OK to assume that the "output" calls shall be called just once, but unfortunately the "input" calls can be invoked more than once without breaking the specs. Therefore, a more appropriate test would be: This way, the test won't break each time that the implementation is changed without actually breaking the specs. Keep in mind that this is a simplified example, but my team and I had to struggle with the maintenance of lots of tests for complex functionalities developed in "agile ways" (i.e. the customer changing each five seconds some details of the specs), that were breaking all the time during development because of being defined "too strictly". That's also one of the key factors that induced me to implement these new mocking methods. |
|
A similar case where this sort of desire crops up is where you're doing "loose" AAA tests -- you have lots of little test cases that each want a little bit of setup and one main thing that you're asserting on -- eg. you care that calling It all depends on what you're wanting to do with the mocks, whether they're exact-implementation-replay or something more loose. Sometimes people call the latter kind "stubs" or "fakes" but they're still mocks at heart. Generally the closer you tie the test to the actual implementation, the more brittle it is and the harder it is to refactor things without breaking a large number of tests, often in trivial ways. |
|
Totally agree with uecasm, I usually call that "aspect testing": Sometimes integration tests have to be performed on a fairly complex module made of several pieces (each tested with their own unit tests), and we need to have the confidence (and evidence!) that the pieces work well together, but then the resulting module is too complex to test it thoroughly as a whole. Then, in a similar way than when system or hardware tests are performed, the module is tested based on different individual "aspects" of its behavior. A real-world example is testing the behavior of a task defined in a Time-Triggered RTOS. That task is defined as a class that has a method that shall be called exactly once every 1 milliseconds. To test the behavior of the task over time, a battery of tests is made which call the task method in a loop, stimulating some inputs and checking some outputs, while the rest of the inputs are just mocked so they don't interfere with the "aspect" that we're testing in each particular test by returning specific values (which can't be achieved using ignoreOtherCalls()). Usually, the most useful method when doing this kind of tests is expectAnyCalls(), but the other ones are also useful: for example, expectAtMostNCalls() can be used to simulate an input that changes its value over time when we don't know exactly how many times the call that reads the input will be called. In my particular example, the task amongst other things has to check that a digital input signal changes its state each 100 milliseconds, and that if it doesn't change it shall enter a specific state of its internal FSM at most 50 milliseconds after the missing edge. Let's take a look at a test that checks that for two proper cycles the task doesn't trigger any state change, then the related signal becomes stuck and the task finally triggers the state change: A deep white-box analysis (or trial and error) would permit replacing the optional calls with an exact number of calls, but this wouldn't make the tests more robust and reliable, because a non-important change (in this context) like modifying the filtering algorithms for inputs would break this test, when that should only break other tests that check specifically aspects related to filtering. This kind of integration tests are common and necessary when developing critical systems (like in automotive or railway). It's true that for systems with critical functional safety requirements higher than SIL-2 or ASIL-B a certified unit testing tool is almost mandatory, but for less critical systems CppUTest is just perfect, and these new methods will make that kind of tests easier and more robust. |
|
I'm never going to be able to get through this pull request as one. Would you mind trying to split it up in smaller steps. My suggestions for first steps:
|
|
Related to the AtLeast kind-of calls. I'm still a bit worried about these, adding these might cause people to use them... what I mean is that they start writing less strict assertions and weaker unit tests. I think I'll probably eventually end up integrating them, but want a short discussion here first. Please be patient (although we can start with smaller pull requests related to the expectNCalls, as mentioned above) |
|
Don't worry @basvodde, I already knew such a big PR would require a lot of review and possibly submitting it as smaller pieces. Doing like you say will provide smaller PRs that don't brake almost anything. |
|
@basvodde: Regarding your concerns related to less strict assertions and weaker unit tests, I think that the new methods are as good or evil as ignoreOtherCalls() or ignoreOtherParameters(). In fact, other mocking frameworks like Google Mock, Mockito, EasyMock or jUnit all have this kind of features, because they're very useful. From my experience, writing proper unit tests does not depend on the unit testing framework or the lack of some "dangerous" features, but only on proper judgement, and because that last thing sometimes seems too scarce, performing code reviews on the unit tests, not only on the production code. I've seen programmers under deadline pressure write totally deceiving tests that just increase coverage, but that don't actually check that the behavior is right: |
|
@jgonzalezdr , I have elaborated on this mock scenario for a long time because my colleague @maxilai submitted almost same PR #973 . I realize that you both have one common pain point, getting extra maintenance effort when dealing with but not with This bring out to one idea I'm thinking for a long time, are we combine the two mock functionality "what mock class should action" and "what mock class should check" into the expectOneCall().andReturnValue() expression?
If we have one separate expression like mock().andReturnValue(), then if you just only want to mock some behavior, you can suppress the mock check. Will this handle most pain case in @jgonzalezdr situation? From the utility point of view, comparison between CppUTest with GTest, it's hard to say who is better, GTest has many utility and keep as flexible as possible, and CppUTest keep as simple enough as possible. The more flexible is, the harder to learn and use. Anyway, if we accept this PR strategy, i have following concern
|
|
Replying to @cuitoldfish comments:
|
|
ok, agree. And how do you think my previous part If we have one separate expression like mock().andReturnValue(), then if you just only want to mock some behavior, you can suppress the mock check. Will this handle most pain case in your situation? |
|
I mean, if we add another function to let user specify what mock class will action but not to check, such as mock().andReturnValue(), then your example test case will look like TEST(TestGroup, OnEventZ_FeatureXActivated)
{
mock("Configuration::IsFeatureXActivated").andReturnValue(true);
mock().expectOneCall("DigitalOutputs::ActivateOutputY");
mock().expectOneCall("SoundManager::PlaySound").withParameter("type", LONG_BEEP);
...
OnEventZ();
mock().checkExpectations();
}then no matter how many times isFeatureActivated() are invoked, test case won't be hurted. |
|
or we could directly use the mock().setData, I forget this one :( TEST(TestGroup, OnEventZ_FeatureXActivated)
{
mock().setData("Configuration::IsFeatureXActivated::returnValue", true);
mock().expectOneCall("DigitalOutputs::ActivateOutputY");
mock().expectOneCall("SoundManager::PlaySound").withParameter("type", LONG_BEEP);
...
OnEventZ();
mock().checkExpectations();
}bool MockConfiguration::IsFeatureXActivated()
{
return mock().getData("Configuration::IsFeatureXActivated::returnValue").getIntValue();
}If I understand your example correctly, could this solution eliminate your test maintenance effort? yes, your above example is a simple one, but I think, from unit test point of view, although the "input call" from outside should not be checked so strictly, the "output" call to the outside should be checked strictly in most case, isn't it? |
|
I find too many drawbacks to your proposal, and also to the one in #PR973:
|
|
thanks for the clarification, I think right now I have gotten the key point in your example and motivation for this new feature. I think it is the more balanced design for some use scenario which need more advanced mock control. |
Basically the feature that I've added is the capability for a single expected call to match multiple actual calls, between a minimum and a maximum number of actual calls.
From the library user perspective, some new methods have been added to MockSupport:
The first improvement to the actual implementation is that expectNCalls doesn't internally generate N expected call objects, therefore memory usage is optimized and this can be fairly important when using the mock framework to perform integration or simulation tests in target platforms where memory resources are scarce.
The second improvement is the capability to declare "optional" expected calls, i.e. expected calls that can match actual calls according to some parameters and then return some specific value, but that we don't know and/or don't care when writing the test how many times they may be called. This is essential when using the mock framework to perform simulation tests, and that was actually the key motivation to implement that new feature, as I needed at work to simulate the behavior of a battery state of charge algorithm under different driving cycle scenarios, and the mocking framework was successfully used to simulate inputs and to check outputs.
The changes are rather extensive, but they have been performed as a series of incremental commits, and each commit is relatively simple and self-contained, solving small problems each time. Therefore, to review this PR, I recommend to inspect each commit in order.
In the following messages I'll comment a bit more about each commit, to make review a bit simpler.