dascandy / hippomocks

GNU Lesser General Public License v2.1
196 stars 67 forks source link

Calling an unexpected method on mock crashes on Linux/gcc when return value is an object #43

Closed mrAtari closed 8 years ago

mrAtari commented 9 years ago
class Argument 
{
  public:
    virtual ~Argument() { };
     int value;
 };

class ISS
{
public:
    virtual Argument getValue() = 0;
 };

FUNC(checkUnexpectedCall)
{
MockRepository mocks;

ISS* is = mocks.Mock<ISS>();
bool unexpectedCall = false;

try
{
    Argument arg = is->getValue();
}
catch (HippoMocks::NotImplementedException)
{
    unexpectedCall = true;
}

CHECK(unexpectedCall == true);
}

On a windows /MSVC system this is no issue.

I debugged the scenario and found out, that in case of an object instance as return argument of a function call, there is a "hidden argument" pushed on the stack. So when the call reaches NotImplemented() func the hidden argument is taken as "this" by mistake. I have no idea, how to fix that properly. The only viable option for me seems to avoid the use of instance data in NotImplentled() at all, directly raising the NotImplemented exception, without tracing the repo data.

Is this an acceptable option? If yes, I would provide a fix for that scenario.

Here's the actual implementation of NotImplemented() function:

void NotImplemented() {
    mock<T> *realMock = getRealThis();
    if (realMock->isZombie)
        RAISEEXCEPTION(ZombieMockException(realMock->repo));
    MockRepository *repository = realMock->repo;
    RAISEEXCEPTION(:: HM_NS NotImplementedException(repository));
}
dascandy commented 8 years ago

It does mean that it'll have to remove the isZombie check on that function. Given that you cannot check it if you do not know the function signature it seems impossible. IT's not a big loss though, because it'll only mean that when you call an unmocked function on an already-deleted mock, you will get the other error that you did make. When you fix that you will then get the zombie mock exception anyway.

Sounds like this is an acceptable solution. Implementing now. Thanks for the detailed explanation!

dascandy commented 8 years ago

Fix is now in; I've also added in your test case verbatim to check it. Thanks again!