eranpeer / FakeIt

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

Add ReturnCapture for methods returning a ref to capture a value inside the mock then return a ref to it. #332

Closed FranckRJ closed 2 months ago

FranckRJ commented 2 months ago

Follow up on was was started by #280 (and #310).

What this PR contains:

What changed compared to the previous PR:

struct Child : Parent { int val = 0; int getVal() override { return val; } };

struct Interface { virtual Child& getChild() = 0; virtual Parent& getParent() = 0; };

void testCopy() { Mock mock;

When(Method(mock, getChild)).ReturnCopy(5); // You can pass only "5" if you want, the "Child" object will be constructed from it.
When(Method(mock, getParent)).ReturnCopy(Child{5});

mock.get().getChild().getVal() // Will return 5.
mock.get().getParent().getVal() // Will return -1 because of the object slicing.

}

void testCapture() { Mock mock;

// When(Method(mock, getChild)).ReturnCapture(5); // ERROR: The parameter must be boundable to Child&.
When(Method(mock, getChild)).ReturnCapture(Child{5}); // You're forced to write it this way.
When(Method(mock, getParent)).ReturnCapture(Child{5}); // Valid.

mock.get().getChild().getVal() // Will return 5.
mock.get().getParent().getVal() // Will return 5 because we store the original type.

}


The last point make the syntax a bit worse than `(Always)ReturnCopy`, but it prevents a bug so I guess it's ok, and the error message is somewhat nice if the wrong type is passed so it's not that bad:

[...] /home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp: In instantiation of ‘fakeit::MethodStubbingProgress<R, arglist ...>& fakeit::helper::BasicReturnImpl<R, true, arglist ...>::ReturnCapture(T&&) [with T = const char (&)[15]; R = ReferenceTypesTests::AbstractType&; arglist = {}]’: /home/franckrj/Projects/FakeIt/tests/referece_types_tests.cpp:302:61: required from here /home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp:93:107: error: static assertion failed: The type captured by ReturnCapture() (named T) must be compatible with the return type of the function (named R), i.e. T t{...}; R& r = t; must compile without creating temporaries. 93 | typename std::remove_cv<typename std::remove_reference::type>::type&>::value, | ^~~~~ [...]



The things I'm not so sure about:
- `(Always)ReturnCapture` is only implemented for methods returning a reference, as it doesn't really make any sense for methods returning a value as `(Always)Return` already captures. But maybe it's better to make it available for all methods to make the API more consistent, I don't know.
- These changes (and the ones of the previous PRs about returning move-only types) only concern classic returns, not `(Always)ReturnAndSet` or `Method(mock, func) = obj`, maybe they should be expanded to impact them as well.
- Maybe `(Always)Return` should work for methods returning rvalue references. But `(Always)ReturnCapture` should already work with these methods so maybe it's enough (but it's not tested I guess so tests should be added to ensure it works).
- Is `(Always)ReturnCapture` a good name ?

@malcolmdavey Does it seem like the feature still covers your needs ?
coveralls commented 2 months ago

Coverage Status

coverage: 99.926%. remained the same when pulling c579f5ad1362b4ab2186f5e560eab07cd48b6eee on iress-OSS-7-copy-return-for-return-by-ref-methods into 1ed5f80d07a89e1ea441ca0cece2b30852b2a4f8 on dev.

malcolmdavey commented 2 months ago

TL;DR - I'm happy enough for the PR to be merged as is.

Good pickup with the slicing issue.

"But maybe it's better to make it available for all methods to make the API more consistent, I don't know."

My own thinking, that when there is only one possible implementation, then it might make sense. The only potential gotcha.complication is if editing the reference value after getting it from an AlwaysReturn. But if you are Always returning a reference then you may expect it to be a ref to the same object, so it makes sense

From memory I added a test which indicates this, but haven't checked the updated PR.

However not to fussed for the current PR. At least there is an option to work around it. And we could change our minds later as it wouldn't be a breaking change.

ReturnCapture - I guess it's okay as a name. Don't want to hold it up based on that.

These changes (and the ones of the previous PRs about returning move-only types) only concern classic returns,

I guess they are lower priority.

For me I sort of wanted to fix these hopefully straight forward Return improvements to handle all sensible and common C++11 scenarios. (it's simple and easy to pass in temporaries to a (Always)Return).

After that I was hoping there would be an agreed way forward to handle the dangling reference problem with argument passing for mocked methods and then validating afterwards ... I think this is the biggest problem with the library as it is, and was also meant to be a selling point. I think the assumption that args are either values, or long lived is usually not the case for things like strings or container classes. If it's a breaking change then we may need a way to opt into it.

FranckRJ commented 2 months ago

Thanks for the feedback. I think you're right, the PR looks good enough as is and future improvements can always be added later in another PR, as it seems they won't break anything.

If you're talking about #274, yeah it's a tricky issue, I've never looked too deep into it. But I guess that an opt-in way of copying reference arguments (instead of the reference itself) could be a short-term workaround for most usages (it will only work for copyable types that don't have too disruptive side effects on their copy constructor, which is probably true for the majority of types but not for all).