Proposal: Prevent capturing arguments on match failure

[ocoanet]

Hi.

I would like to improve the capture helper to support the following scenario:

[Fact]
public void ShouldOnlyCaptureParameterForSpecificArgumentAfterCollection()
{
	var items = new List<string>();
	var mock = new Mock<IFoo>();
	mock.Setup(x => x.DoSomething(Capture.In(items), 1));

	mock.Object.DoSomething("Hello!", 1);
	mock.Object.DoSomething("World", 2);

	var expectedValues = new List<string> { "Hello!" };
	Assert.Equal(expectedValues, items);
}

This test currently fails because the capture match is evaluated on every method call, even when other matches do not pass. The goal of this PR is to perform the capture only after all parameter match were successfully evaluated.

The current implementation is a proposal and I am clearly open to suggestions or improvements ideas.

[stakx]

@ocoanet, thanks for submitting this proposal—it makes total sense! I'll try to review it in detail as soon as I can... might take a few days, though. Please be patient.

Right now, I'm suspecting that Moq's current behavior might simply be buggy: An unmatched setup should in all likelihood stay completely inert and not cause any observable side effects whatsoever. So we might eventually treat your PR as a bug fix, instead of as an enhancement. (One difference being that the former doesn't add new ctors in CaptureMatch.)

I'll get back to you soon.

[ocoanet]

Yes, I added overloaded constructors to CaptureMatch because I wanted to avoid breaking existing code. But I suppose that CaptureMatch is not often used directly, and the current behavior can be considered to be a bug.

Tell me if you want to treat this change as a bug fix, in that case I will remove the new constructors and simplify the code.

[stakx]

This is a somewhat superficial review but I wanted to give you some early feedback.

This PR raises an issue that needs some deeper thought (statefulness of matchers), I'll probably get back to you on that topic.

For now, the big point is probably that you may indeed treat this as a bug fix (with the consequences already discussed above).

Btw. we'd also need a changelog entry (under the Changed heading) for this so users are aware of the functional change.

Thanks for working on this!

[ocoanet]

I updated the PR:

• CaptureMatch is now immutable.
• CaptureMatch only captures the argument on success.

[ocoanet]

Should I update Match<T>.Equals to compare the SuccessCallback?

[stakx]

This is looking great, and the code has become simpler than what we've had before! 👍

Just one more request: Please do add an entry in CHANGELOG.md (either in the Changed or Fixed section) so users will be informed about this change.

Apart from that, most of the review comments below concern themselves with details (all but the long one), feel free to fix as little or as much of it as you have time for.

Should I update Match<T>.Equals to compare the SuccessCallback?

I think we can leave Equals as it is for now: Equals is mainly important when Moq reconstructs LINQ expression trees from plain delegates (as used with e.g. SetupSet, VerifySet), and I suspect that CaptureMatch already doesn't integrate well with that process, due to it inaccurately representing itself in render expressions as an It.IsAny<T>.

We can solve this if and when someone notices in the future that Capture.* isn't reconstructed correctly in SetupSet, VerifySet expressions.

[ocoanet]

I applied your advises and I added the missing unit test. I also rename OnSuccess to SetupEvaluatedSuccessfully in order to avoid introducing a new term and to make Condition and Match more similar.

I did not add the abstract method in Match, I will do it if you confirm that the breaking change is acceptable.

[ocoanet]

By the way, now that Match<T> has a success callback, I feel that CaptureMatch is almost unnecessary. It could be turned obsolete and replaced by a static factory method Match<T>.Capture.

[stakx]

Looks great! I'd be happy to merge your PR now, but I'll wait a little bit longer, just in case that you want to take care of the remaining bits (the changelog entry & that abstract method in Match).

I also rename OnSuccess to SetupEvaluatedSuccessfully in order to avoid introducing a new term and to make Condition and Match more similar.

I like the rename... makes good sense! 👍

now that Match<T> has a success callback, I feel that CaptureMatch is almost unnecessary. It could be turned obsolete and replaced by a static factory method Match<T>.Capture.

Good thought, we might want to look at this more closely in a separate issue. My gut feeling is to leave things as is for now, among other things because Match.Capture would be a somewhat contradictory name: Does it match? Or does it capture? Or does it do both? Name-wise it may be clearer to just keep Match.* and Capture.* as separate as possible (even though the latter are technically matchers, too).

[ocoanet]

Argh, I forgot the changelog again!

[stakx]

It appears you've edited an out-of-date version of CHANGELOG.md (since there have been a few changes since you opened your PR), can you rebase on top of current moq/moq4#master?

(For example, check out a new topic branch at your current master; then git reset --hard 9c0d33dd, which resets master to the last commit before your work began; then git pull upstream master to bring master up-to-date (upstream is a remote pointing to moq/moq4); then checkout your topic branch and git rebase it onto master and resolve any merge conflicts that may happen; then merge your topic branch into master (fast-forward) and force-push master to origin.)

[stakx]

Almost there! Please restore the removed changelog entries, then we're ready to merge!

[stakx]

🚀 Thank you for your contribution!