Verify methods with parameters passed by reference

[bcachet]

Hello,

We have interfaces with methods that take parameters by reference.
Stubing such methods seem to work but when trying to Verify invocations, I got troubles.

struct SomeInterface {
  virtual ~SomeInterface() {}
  virtual int foo(int&) = 0;
};

TEST_CASE("FakeIt integration", "[fakeit]") {
  Mock<SomeInterface> mock;
  When(Method(mock, foo)).AlwaysDo([](int& i)
  {
    std::cout << i << std::endl;
    return 1;
  });
  auto &si = mock.get();
  SECTION("We mock a method with param passed by reference") {
      auto i = 0;
      REQUIRE(si.foo(i) == 1);
      ++i;
      REQUIRE(si.foo(i) == 1);
      Verify(Method(mock, foo).Using(0), Method(mock, foo).Using(1)).Once();
  }
}

I got the following output:

0
1
...
... some lines related to Catch output
...
  Verification error
  Expected pattern: mock.foo(0) ... mock.foo(1)
  Expected matches: exactly 1
  Actual matches  : 0
  Actual sequence : total of 2 actual invocations:
    mock.foo(1)
    mock.foo(1)

All the invocations seem to be done with last value of the parameter.

Is there a way to indicate to FakeIt that this parameter should be treated as by copy. Here we only pass it by reference for optimization purpose.

Thanks for your help

Sincerely

Bertrand

[eranpeer]

Well, that's an interesting corner case.
Reference types are problematic since fakeit can only store the reference and not the value the variable at the moment of the invocation.
This is because:

1. references are used with abstract types and fakeit has no knowledge about the concrete type.
2. The type of the variable may not be copy constructible.

Thus, you can only verify with the original reference at the moment of invocation. The following line will work with your example:

Verify(Method(mock, foo).Using(i), Method(mock, foo).Using(i)).Once();

Do you think we need a special behavior for reference of scalar types?
Scalar types are all concrete, copy-able & comparable types. I may be able to treat them differently, though I'm not sure it is the right thing to do.
What do you think?

[bcachet]

You are right, I'm really in a corner case.
Because what I expect from FakeIt is to treat my parameter as a copy
parameter (not attended to change the value, I always declare this
parameter as "const&" by the way) but my signature indicate that I pass it
by reference.

Maybe I should use Do/AlwaysDo methods to test this corner case.

On Thu, Aug 13, 2015 at 2:49 PM, eranpeer notifications@github.com wrote:

Well, that's an interesting corner case.
Reference types are problematic since fakeit can only store the reference
and not the value the variable at the moment of the invocation.
This is because:

1. references are used with abstract types and fakeit has no knowledge
   about the concrete type.
2. The type of the variable my not be copy constructible.

Thus, you can only verify with the original reference at the moment of
invocation. The following line will work with your example:

Verify(Method(mock, foo).Using(i), Method(mock, foo).Using(i)).Once();

Do you think we need a special behavior for reference of scalar types?
Scalar types are all concrete, copy-able & comparable types. I may be able
to treat them differently, though I'm not sure it is the right thing to do.
What do you think?

—
Reply to this email directly or view it on GitHub
#31 (comment).

Bertrand

. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
☎: +41 78 664 20 88
🏠: Route de Neuchâtel 3; 2520 La Neuveville; Switzerland

[eranpeer]

For example:

struct SomeInterface {
    virtual ~SomeInterface() {}
    virtual int foo(int&) = 0;
};

std::vector<int> values;
Mock<SomeInterface> mock;
When(Method(mock, foo)).AlwaysDo([&](int& i)
{
    values.push_back(i);
    return 1;
});

auto &si = mock.get();
auto i = 0;
si.foo(i);
++i;
si.foo(i);

ASSERT_EQUAL(0, values[0]);
ASSERT_EQUAL(1, values[1]);

[bcachet]

That's what I am ending with.

I think it's the way to do it.
Not sure it is a good idea to add additional operator to FakeIt to handle
such a strange behavior.

Thanks a tonn for your library.

I am migrating from HippoMocks and I have to say that my tests are fare
more readable now.

On Thu, Aug 13, 2015 at 3:41 PM, eranpeer notifications@github.com wrote:

For example:

struct SomeInterface {
virtual ~SomeInterface() {}
virtual int foo(int&) = 0;
};

std::vector values;
Mock mock;When(Method(mock, foo)).AlwaysDo([&](int& i)
{
values.push_back(i);
return 1;
});
auto &si = mock.get();auto i = 0;
si.foo(i);
++i;
si.foo(i);
ASSERT_EQUAL(0, values[0]);ASSERT_EQUAL(1, values[1]);

—
Reply to this email directly or view it on GitHub
#31 (comment).

Bertrand

. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
☎: +41 78 664 20 88
🏠: Route de Neuchâtel 3; 2520 La Neuveville; Switzerland

[eranpeer]

Thank's for the feedback.
If you have any idea about how exactly the additional operator will be used, please feel free to send your suggestions.
Cheers,

[coombez]

I also just ran into this case but with a const & to a temporary on the callers stack. Verifying the Method with Using in this case leads to undefined behaviour.

I agree with the assessment that you can't always take a copy. I'm wondering if you can make calling Using an error in these cases to avoid potential segfaults?

It would be pretty clever to detect if a type is copy constructable and take a copy in that case too, but that's going above and beyond.

Also liking fakeit so far. Thanks!

[eranpeer]

I have some idea of how to fix this and I'm working on a POC (Proof Of Concept).
Hope to have some news soon.

[amerry]

Any news on this? I'm currently having to hack together custom call sequence recorders for methods with std::string const& arguments.

[dirkharbinson]

Run into the same problem unit testing some 3d math code. E.g. a function that does a vec3 + vec3 and passes the local result to a mock class by const&. This scenario is too common for us so without a clean solution we have to stick with google mock (which solves this problem by defining your verifys before the invocation and capturing/matching the arguments at call time). Apart from that this library was working really good.

[jeduden]

any news on this issue ?

[estan]

I don't know why you call this a corner case. const & parameters are very common, especially in Qt projects. Our project has tons and tons of such functions, and as it is, I can't Verify(Method(mock, method).Using(..)) them :(

The only resort seems to be the suggestion @eranpeer made in #31 (comment) , but that takes away the whole point of using fakeit (its nice and fluent mocking API).

[f1xpl]

It looks like issue still persists.

[otterovich]

I've done a small modification to the library that solves this issue for me. This modification doesn't cover all the cases but it works perfect for my project.

When you open single-header fakeit.hpp you can see something like this in first few lines of code:

    template< class T > struct tuple_arg         { typedef T  type; };
    template< class T > struct tuple_arg < T& >  { typedef T& type; };
    template< class T > struct tuple_arg < T&& > { typedef T&&  type; };

    template<typename... arglist>
    using ArgumentsTuple = std::tuple < arglist... > ;

Just replace this code with the following:

	template< class T > struct tuple_arg { typedef typename std::remove_reference<T>::type  type; };

	template<typename... arglist>
	using ArgumentsTuple = std::tuple < typename std::remove_reference<arglist>::type... >;

The idea behind this modification is the following. tuple_arg and ArgumentsTuple are generic types used for internal storing of what has been passed to a function as parameters. Just remove reference for them.

[helmesjo]

@eranpeer Should this really be closed? Is @otterovich solution something that can be built on?
This "random" crash currently happening causes a lot of head-scratching and frustration (the call stack is not particularly helpful).

As previously mentioned, without reliably being able to verify arguments passed to a mock one is severely limited.

[mmatthe]

I'm not sure if this solution is something one can build upon. I just stumbled over this bug while trying out FakeIt. I believe the current solution might fail to compile in cases the argument is not copy-constructible.

Also, it would be great if this workaround would be integrated into master/tagged branch.

[mmatthe]

This modification does inhibits to mock methods that receive const-ref arguments:

struct S{
  int a;
  int b;
};


class C2 {
public:
  virtual int callWithConstRef(const S& s) = 0;
};

TEST_CASE("Fakeit does not compile with const ref args?") {
  using namespace fakeit;
  Mock<C2> mock;
  When(Method(mock, callWithConstRef)).Return(2);
  REQUIRE(mock.get().callWithConstRef(S{2,3}) == 2);
}

Error message when compiling with gcc7

/tests/fakeit.hpp: In instantiation of ‘R fakeit::Repeat<R, arglist>::invoke(fakeit::ArgumentsTuple<arglist ...>&) [with R = int; arglist = {const S&}; fakeit::ArgumentsTuple<arglist ...> = std::tuple<const S>]’:
/tests/test_fakeit.cpp:57:1:   required from here
/tests/fakeit.hpp:7095:58: error: no matching function for call to ‘fakeit::TupleDispatcher::invoke<int, const S&>(std::function<int(const S&)>&, fakeit::ArgumentsTuple<const S&>&)’
             return TupleDispatcher::invoke<R, arglist...>(f, args);
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/tests/fakeit.hpp:6211:18: note: candidate: template<class R, class ... arglist> static R fakeit::TupleDispatcher::invoke(std::function<R(ArgsF& ...)>, const std::tuple<_Elements ...>&)
         static R invoke(std::function<R(arglist &...)> func, const std::tuple<arglist...> &arguments) {
                  ^~~~~~
/tests/fakeit.hpp:6211:18: note:   template argument deduction/substitution failed:
/tests/fakeit.hpp:7095:58: note:   mismatched types ‘const S&’ and ‘const S’
             return TupleDispatcher::invoke<R, arglist...>(f, args);
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/tests/fakeit.hpp: In instantiation of ‘R fakeit::RepeatForever<R, arglist>::invoke(fakeit::ArgumentsTuple<arglist ...>&) [with R = int; arglist = {const S&}; fakeit::ArgumentsTuple<arglist ...> = std::tuple<const S>]’:
/tests/test_fakeit.cpp:57:1:   required from here
/tests/fakeit.hpp:7117:58: error: no matching function for call to ‘fakeit::TupleDispatcher::invoke<int, const S&>(std::function<int(const S&)>&, fakeit::ArgumentsTuple<const S&>&)’
             return TupleDispatcher::invoke<R, arglist...>(f, args);
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/tests/fakeit.hpp:6211:18: note: candidate: template<class R, class ... arglist> static R fakeit::TupleDispatcher::invoke(std::function<R(ArgsF& ...)>, const std::tuple<_Elements ...>&)
         static R invoke(std::function<R(arglist &...)> func, const std::tuple<arglist...> &arguments) {
                  ^~~~~~
/tests/fakeit.hpp:6211:18: note:   template argument deduction/substitution failed:
/tests/fakeit.hpp:7117:58: note:   mismatched types ‘const S&’ and ‘const S’
             return TupleDispatcher::invoke<R, arglist...>(f, args);
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~

Compilation exited abnormally with code 1 at Mon Mar 11 07:43:59

(see the mismatched types const S& and const S line.

I believe this is due to the std::remove_reference proposal. Any workarounds?

[scgroot]

I'm using the following workaround:

class I {
 public:
  virtual void test(const std::string& d) = 0;
};

class IFix : public I {
 public:
  virtual void testF(std::string d) = 0;
};

using namespace fakeit;

TEST_CASE("blaat") {
  Mock<IFix> mock;
  When(Method(mock, testF)).AlwaysReturn();
  When(Method(mock, test)).AlwaysDo(std::bind(&IFix::testF, &mock.get(), std::placeholders::_1));
  mock.get().test("123");
  mock.get().test("456");
  Verify(Method(mock, testF).Using("123"), Method(mock, testF).Using("456"));
}

Basically, you forward the method using references to a call that does not use them. Than build the test on that method. Not super pretty, but at least you don't have to change production code or fakeIt itself. The *Fix interface can just exist in the test code. Note that both test and testF are triggered, so using Verify(... + ...) instead of Verify(... , ...) would not have worked here.

It's something...

[canatella]

This makes fakeit nearly useless for me, the whole point of having a mock library is that I don't have to write the mock by hand. Now I have to write a wrapper that defines a place where to store the argument, define the mock method that copy the ref argument in there and triggers the fakeit mock so that I can call verify. I'll stop using faking when I have a bit of time to switch to something else.

[malcolmdavey]

Any progress on this? Does seem like quite more than a corner case, and mostly because the verify logic is done after the invocation step, not at the same time.

Yes, strings are a big issue here.

Also who are the main maintainers of this now?

[henning-erlandsen]

I guess this is the reason why Google Mock requires expectations to be set before executing the methods that are supposed to call the mock and not after the fact.

[malcolmdavey]

I'm planning an opt in fix/change to support this, to enable copying of parameters.

[jdoubleu]

I ran into the same issue today. It caused a SEGFAULT, thanks to debug runtime checks. Unfortunately, it took me some debugging to figure out the root cause.

I'd prefer if this footgun wasn't possible, as @coombez suggested:

I agree with the assessment that you can't always take a copy. I'm wondering if you can make calling Using an error in these cases to avoid potential segfaults?

My workaround is to patch fakeit's ActualInvocation, to copy the params on construction. Luckily I was only using copy-able types.

Furhtermore, I believe that this problem is not solvable: Besides reference parameters, one may use non-owning views (e.g. std::string_view or std::span). In this case, you cannot easily copy that underlying data.

That's probably the reason, why with gMock you have to make the assertion upfront (as @dirkharbinson mentioned):

we have to stick with google mock (which solves this problem by defining your verifys before the invocation and capturing/matching the arguments at call time).

[FranckRJ]

Yeah, it's a known issue, as you have seen I tried to centralize the discussion about it here: #274.