eranpeer / FakeIt

C++ mocking made easy. A simple yet very expressive, headers only library for c++ mocking.
MIT License
1.22k stars 169 forks source link

Object slicing in verification arguments #240

Closed rpadrela closed 2 years ago

rpadrela commented 3 years ago

I've come across an issue that can be demonstrated with the following code:

class A
{
public:
    virtual ~A() = default;
};

class B : public A
{
public:
    ~B() override = default;
};

bool operator==(const A& lhs, const A& rhs)
{
    const std::type_info& tlhs = typeid(lhs);
    const std::type_info& trhs = typeid(rhs);
    return tlhs == trhs;
}

class Interface
{
public:
    virtual ~Interface() = default;
    virtual void Foo(const A& param) = 0;
};

TEST_CASE("Test", "[Whatever]")
{
    fakeit::Mock<Interface> mock;
    fakeit::Fake(Method(mock, Foo));

    Interface& i = mock.get();

    B arg;
    i.Foo(arg);

    fakeit::Verify(Method(mock, Foo).Using(arg));
}

The verification will fail because operator==(const A& lhs, const A& rhs) will return false. This operator is returning false because the references passed in are references to objects of different types. One is of type B whilst the other is of type A. The object of type A is a copy of the original arg that was sliced when passed in to Using(arg).

Changing the following line

https://github.com/eranpeer/FakeIt/blob/1576b26efca0a91361ff1974062c6da25ca944f6/include/fakeit/argument_matchers.hpp#L45

to const T& _expected; fixes the issue but creates other problems. I haven't fully investigated these other problems but they seem to be related to keeping a reference to a temporary variable (e.g. int) that has already gone out of scope by the time Verify() executes.

Any thoughts?

rpadrela commented 3 years ago

Replacing fakeit::Verify(Method(mock, Foo).Using(arg));

with fakeit::Verify(Method(mock, Foo).Matching([&](const A& a){ return a == arg; }));

seems to do the trick. Is Matching() the right tool for the job here?

FranckRJ commented 2 years ago

Your workaround is ok, but ideally we should fix the matcher. It seems like it's easy to fix, matchers aren't a big chunk of code, I may take a look.

FranckRJ commented 2 years ago

I added a test and I guess #268 fixed it.