Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate delegates passed to Returns more strictly #520

Merged
merged 5 commits into from
Nov 10, 2017

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Nov 10, 2017

This PR adds minimal validation logic for delegates passed to Returns:

  • Return type must be assignment compatible
  • Parameter count must match

While it would be desirable from a technical standpoint, parameter types do not get checked for assignment compatibility due to two possibly widespread practices:

  • Callback uses a by-value callback parameter while method being set up has a by-ref-parameter:

    TArg expectedX = ...;
    mock.Setup(m => m.Method(ref expectedX).Returns<TArg>(actualX => ...);
  • Callback uses a subtype and setup uses a matcher to ensure runtime value conversion will always succeed:

    mock.Setup(m => m.MethodWithObjectParam(It.IsAny<int>()).Returns<int>(i => ...);
    mock.Setup(m => m.MethodWithObjectParam(It.IsAny<string>()).Returns<string>(str => ...);

While those are possibly not the cleanest programming practices (from a purely technical point of view), many people might find them useful in practice. Allowing them means that parameter type validation becomes pretty much pointless.

This closes #445.

This introduces one minor breaking change: `Returns` delegates with
non-matching return types could still succeed during an actual invoc-
ation if the return value was convertible from the "wrong" type to
the expected type (e.g. with `null` for any two reference types).
@stakx stakx merged commit 104c646 into devlooped:develop Nov 10, 2017
@stakx stakx deleted the returns-failfast branch November 10, 2017 18:52
@ohadschn
Copy link

@stakx Not sure what you mean "Allowing them means that parameter type validation becomes pretty much pointless". If I understand correctly, you would be preventing stuff like mock.Setup(m => m.MethodWithStringParam(It.IsAny<int>()).Returns<int>(i => ...); which is a win in my book...

@stakx
Copy link
Contributor Author

stakx commented Nov 10, 2017

Having been more or less the only active contributor to Moq v4 for a while now, I've become aware that this also means I'm increasingly exposed as the one to be blamed when things break. For this reason I'm simply not as keen now as I might've previously when it comes to introducing breaking changes; especially when they're avoidable.

That being said, I still do agree it would be nice to be more consistent and thorough with validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants