-
Notifications
You must be signed in to change notification settings - Fork 533
Added support for memory buffer parameter checking in mocks. #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Now MockActualCall and MockExpectedCall both have new methods withMemoryBufferParameter(const SimpleString& name, const unsigned char* value, size_t size) and withParameter(const SimpleString& name, const unsigned char* value, size_t size), that allow checking that the buffer have the same length and contents.
I see that this PR doesn't seem to be reviewed. Do you need any extra info? I've successfully used this feature to mock low-level drivers in embedded projects like EEPROM memory controllers or even a dot-matrix graphical display controller to unit test raster font rendering routines, so it might be useful to others. If you may be wondering, regarding the MockActualCall methods I chose to only offer a version that needs both the buffer pointer and the length for a simple reason: To encourage a good programming practice that dictates that when passing "generic" pointers to memory areas, the length of the area behind the pointer shall always be known, either explicitly or implicitly (e.g. nowadays it's totally discouraged to use non-safe versions of sprintf; instead snprintf shall be used). In fact, with just a cast, these methods can also be used to compare known-length structs or arrays of any kind. If anyone has any questions don't hesitate to ask. |
Exactly how does this differ from withOutputParameterReturning()? |
@jgonzalezdr Sorry, I should be looking at it but been busy with other things. I will look at it soon-ish. Sorry for the late response. Thanks for the contribution though! |
@arstrube It's a different concept than withOutputParameter() / withOutputParameterReturning(): the latter, as its name implies, is for output parameters, and just copies the buffer provided in the expected call to the actual call's buffer (in fact without checking that the actual call's buffer is big enough); withMemoryBufferParameter() is for input parameters, and what it does is checking that the contents (and size) of the actual call's buffer are identical to the ones provided in the expected call. When analyzing the code to find the best way to implement this I of course looked at the implementation of withOutputParameter(), and thought of implementing a safer version of it by also providing a method that gets the buffer size as an argument and checks it before copying. But I preferred to delay that change 'till the current ones where accepted. @basvodde OK, take the time that you all need, I just was wondering if the PR was just overlooked or if it was being discussed somewhere else (I just don't know of any CppUTest developers' mailing list or such besides the Google Groups). |
I had a use case with an I/O parameter that took some trickery to get to work around it with withOutputParameterReturning(). The thing was, it was a struct with say 20 fields, where one field had to be set by the test, but the other 19 had to be unchanged (yet their values being unknown to the test). So, would this be possible / easier to handle with withMemoryBufferParameter(), perhaps? Just wondering..... |
I generally prefer to get full control of the testing environment, if necessary adding functions and code intended only for unit testing (i.e. conditional compiling it by enclosing it between Anyway, in the case that I wanted to check only a portion of an input buffer or an input struct, I would think of creating a specific comparator that only checked what is necessary (e.g. converting the You can even change the buffer or struct fields in the Besides this, for input only buffers, my actual implementation could also be expanded with an additional method such as In the case on input/output buffers, I'm not sure but maybe chaining If that didn't work, an easy implementation that comes to my mind would consists in an additional method In the case of input/output buffers with partial checking, well, it might also need to implement an additional |
Thank you for this very detailed explanation. My own attempt at dealing with this situation is here: I was just hoping your pull might help in this situation... |
Either of your implementations solve the problem, but they have a problem from my point of view: Mocks are not generic. I'll explain: When writing unit tests, I prefer (and at work I tyrannically impose to my team) that mocks shall be generic. You write the mocks just once and they are shared by all the unit tests (e.g. in a common folder), and they shall be trivial and depend only on the function parameters. Therefore, all the test "logic" shall remain in the test implementation. That's the point of mocks (vs. stubs), isn't it? If you wanted to modify different fields, then you have to write different mocks for each case... ouch! Of course, in your specific use case I would do something like what you've done, but just as a workaround because the mocking framework doesn't support that specific functionality. That's why, taking in account that this use case will appear in many real-world tests, I think that the framework should be expanded to provide the functionality that allows doing these tests in an easy and "elegant" way. I'll give it a couple of more thoughts to this use case to see if I can come up with something that is suitable to resolve the problem in a generic and elegant way (e.g. what if we want to modify fields that are not contiguous in memory...?). |
I totally agree with you. I wish there were an elegant solution for this kind of situation, as it is not uncommon. But it's also not easy to deal with and I'm afraid we might overburden the poor mock eventually... ;-) Generally, getting a pointer to the argument solves many problems. But, in the case of a mock, the target of this pointer needs to be manipulated during the mock call -- not before, and not after. Maybe a pass a pointer to a callback function, which can be set on a per-test basis? Maybe CppUTest needn't even do this, as the mock is defined in my tests? Good point about fields not contiguous in memory. The simpler one of my workarounds can't do that. |
I have in mind something like that, but using a functor class instead of a callback for coherence with comparators. That would be generic and powerful enough to do any kind of modifications. When I'll get some free time I'll give a try to an implementation. |
Sounds great. |
@@ -82,6 +82,7 @@ class MockNamedValue | |||
virtual void setValue(void* value); | |||
virtual void setValue(const void* value); | |||
virtual void setValue(const char* value); | |||
virtual void setValue(const unsigned char* value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overloading char* and unsigned char* is asking for trouble. Can we not do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried to just use void* in the method signatures, and in MockNamedValue
I added a method setValue(void* bufferPtr, size_t bufferSize)
, but then I found that internally the actual type is stored as a string instead of an enum or whatever (I presume that for an easy implementation of object types).
So I'd had to come with a strange type name to set type_
, like "memory buffer", "void* with size", "void* buffer" or whatever, and that didn't appeal me a lot. Maybe a bit influenced by my own programming guidelines (char*
is for strings, unsigned char*
for buffers which are not strings or which are generic), and considering that the "simple" type unsigned char*
was not being overloaded, I decided to use it.
Anyway, taking in account that MockNamedValue
is not used by framework users and that in MockExpectedCall
and MockActualCall
the actual withParameter
method can't be accidentally miscalled because of the different signatures (i.e. the one with unsigned char*
always takes an additional size parameter), I don't think that this is a problem. Anyway, the setValue(const unsigned char* value)
could be replaced with a setValue(const unsigned char* value, size_t size)
to avoid confusions.
Otherwise, it could be feasible to replace the actual type_
with an enum and add an additional objectType_
just for "user defined" types (i.e. for withParameterOfType
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, it is probably better not to use the same sane (SetValue) for it, to avoid accidental errors.
@jgonzalezdr Uhm, you aren't using the memory buffer printing and comparison that was recently added to CppUTest, right? Would you mind using that one to avoid duplication and not adding another PlatformSpecific call? |
@basvodde Ok, I'll take a look on how to do it. |
Can you let me know when its done, then I'll look at the code again :) |
… to have a differentiated setMemoryBuffer method (instead of setValue)
@basvodde Refactoring done |
@jgonzalezdr a functor class for copying would be great. See #670. We could then have something like MockExpectedCall& withOutputParameterOfTypeReturning(SimpleString name, SimpleString type,
void* object) |
} | ||
return str; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you decided to re-implement this instead of using the StringFromBinaryOrNull method? Could you use that one instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use it because it doesn't print the buffer length, but I can refactor the code to use it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thinking about it, I guess the printing of the length of the buffer maybe shouldn't be in the StringFrom?
Sorry for the delay. Thanks for the refactoring. Would you mind checking if we can get the printing of the memory buffer equal between the mocking framework parameter and the MEMCMP_EQUAL output? I guess there is no reason why they are different, correct? |
… hex dump of mocks memory buffers
@basvodde I've refactored the code to use StringFromBinary. |
Thanks! |
Added support for memory buffer parameter checking in mocks.
is C interface support withMemoryBufferParameter? |
It's an oversight. Will add it shortly. |
This is addressed in PR #721 |
Now MockActualCall and MockExpectedCall both have new methods withMemoryBufferParameter(const SimpleString& name, const unsigned char* value, size_t size) and withParameter(const SimpleString& name, const unsigned char* value, size_t size), that allow checking that the buffer have the same length and contents.