dascandy / hippomocks

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

Class mocking example doesn't work #88

Open mje-nz opened 6 years ago

mje-nz commented 6 years ago

The "Sample usage of class mocking" example on the wiki fails, with the error "Function called without expectation!".

Is it possible to mock some methods of a class and leave the rest untouched? If not, the wiki needs to updated.

Here's the code I used:

class CFoo {
public:
    bool Func1() {
        bool ret = false;
        if (Func2()) {
            //do something
            ret = true;
        }
        else {
            //do something else
        }
        return ret;
    }

    bool Func2() {
        return true;
    }

};

TEST_CASE("Hippomocks example", "[sanity]") {
    MockRepository mocks;
    CFoo* pFoo = mocks.Mock<CFoo>();
    mocks.ExpectCall(pFoo, CFoo::Func2).Return(true);
    REQUIRE(pFoo->Func1() == true);
    mocks.ExpectCall(pFoo, CFoo::Func2).Return(false);
    REQUIRE(pFoo->Func1() == false);
}
dkgs commented 6 years ago

I don't have the same vision of mocking as you.

When you test a class, you test the behaviour of your entire class. If you just want to check a method, it means it doesn't get to be owned by the class. The mock is useful to isolate the class you want to test from everything around and be sure your class is responding well from certain scenarios that you can't set normally.

Therefore, in your example, you should either extract your Func2 from your class into another that you'll mock or you should stick to testing your CFoo class as a whole.

mje-nz commented 6 years ago

Thanks for your feedback @dkgs, but I think we've gotten off on the wrong foot here.

Firstly, this is nothing to do with my vision of mocking: this is about the usage example on the HippoMocks wiki. If you don't feel the example is representative of the correct way to use HippoMocks then as I said above the wiki needs to be updated.

Secondly, I agree with you about how to test classes. I'm testing a class which which contains a class from a third-party library by replacing the third-party dependency with a test stub. The stub has half a dozen methods that I don't care about but which need to set up some state, and three methods for which I'd like to verify the arguments when they're called. I can make it work by storing the state in my test function and using ExpectCall().Do() all over the place, but it makes my tests unmaintainable.

Is there a way of just spying a function, like in FakeIt?

dascandy commented 6 years ago

I have to agree with @dkgs on use of mocks - you either mock a part of an interaction, or you don't. If you feel the need to mock half your class to test the other half, you probably have more than one responsibility in one class. Then again, that does exist in code and you may want to test it before refactoring, so even if you agree there's still value to your argument as well.

If you want a function to exist and have some defined behavior, use OnCall. This just sets up the functions existence and its basic behavior (return, throw). Just spying on functions is not supported sadly - but I think FakeIt requires you to set up a return value in the exact same way, for the same reason.

mje-nz commented 6 years ago

Again, I'm certainly not mocking half of my class to test the other half, that's what your usage example does!

Here's a simplified version of the class I'm testing:

class MyClass {
public:
    MyClass(INetworkClient *client)
      : client_(client) {
        client_->setConnectCallback(std::bind(&MyClass::onConnect, this));
        client_->setMessageCallback(std::bind(&MyClass::onMessage, this, _1));
        // Other callbacks and assorted setup functions
    }

    void connect() {
        client_->connect();
    }

    bool receivedMessage() {
        return received_message_;
    }

private:
    void onConnect() {
        // Need to test this method's behavior
        client->send(/* Session info */);  
    }

    void OnMessage(const char *message) {
        // Need to test this method's behavior
        received_message_ = true;  // Obviously not the real behaviour
    }

    INetworkClient *client;
    bool received_message_;
};

To test onConnect and onMessage, I need to hold onto the callbacks and call them later. From using test frameworks in other languages, I imagined it would go something like this:

class FakeNetworkClient : INetworkClient {
public:
    void setConnectCallback(std::function<void()> callback) {
        connect_callback_ = callback;
    }

    void setMessageCallback(std::function<void(const char*)> callback) {
        message_callback_ = callback;
    }

    // Other callbacks and assorted setup functions

    void connect() {
        if (connect_callback_) connect_callback_();
    }

    void send(const char *message) {
        // Need to verify this method's arguments
    }

    void fakeMessage(const char *message) {
        if (message_callback_) message_callback_(message);
    }

private:
    std::function<void()> connect_callback_;
    std::function<void(const char*)> message_callback_;
    // Other callbacks and assorted state
};

TEST_CASE("MyClass sends session info on connect") {
    MockRepository mocks;
    INetworkClient *client = mocks.Mock<FakeNetworkClient>();
    MyClass my_class(client);

    mocks.ExpectCall(client, INetworkClient::send).With(/* Session info */);
    my_class.connect();
    mocks.VerifyAll();
}

TEST_CASE("MyClass sets flag on receiving message") {
    FakeNetworkClient client;
    MyClass my_class(&client);

    client.fakeMessage("test");
    REQUIRE(my_class.receivedMessage());
}

// Other tests

Are you saying that the HippoMocks idiom is something like this?

TEST_CASE("MyClass sends session info on connect") {
    MockRepository mocks;
    INetworkClient *client = mocks.Mock<INetworkClient>();
    std::function<void()> connect_callback;
    mocks.OnCall(client, INetworkClient::onConnect).Do([&](std::function<void()> callback) {
        connect_callback = callback;
    });
    mocks.OnCall(client, INetworkClient::onMessage);
    // Mock other setup functions and hold onto other state as appropriate
    MyClass my_class(client);

    mocks.ExpectCall(client, INetworkClient::connect);
    my_class.connect();

    mocks.ExpectCall(client, INetworkClient::send).With(/* Session info */);
    connect_callback();
    mocks.VerifyAll();
}

TEST_CASE("MyClass sets flag on receiving message") {
    MockRepository mocks;
    INetworkClient *client = mocks.Mock<INetworkClient>();
    std::function<void(const char*)> message_callback;
    mocks.OnCall(client, INetworkClient::onConnect);
    mocks.OnCall(client, INetworkClient::onMessage).Do([&](std::function<void(const char*)> callback) {
        message_callback = callback;
    });
    // Mock other setup functions and hold onto other state as appropriate again
    MyClass my_class(client);

    message_callback("test");
    REQUIRE(my_class.receivedMessage() == false);
}

For the record, FakeIt's spies don't require you to set up a return value.

dkgs commented 6 years ago

When you talk about the HippoMocks wiki, do you refer to http://www.hippomocks.com ? I didn't know this website until now. @dascandy are you aware of it ? I don't know who has setup this wiki, but in fact their example won't work as there is no "default behaviour" for methods of a mocked class. HippoMocks will throw a NotImplementedException in this case.

As for your use case, don't do as in your first example, the goal of using HippoMocks is to not create a FakeNetworkClient but only to mock the functions used by the class you want to test.

Yes, your second example is, at least for me, the way to use HippoMocks.