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

The condition determining whether a parameter is 'out' parameter is changed #645

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

koutinho
Copy link
Contributor

@koutinho koutinho commented Jul 18, 2018

According to that rules ('Mapping method parameter keywords in C# to IDL's Directional attributes' section) and MSDN description about Directional Attributes, the c# 'out' method parameter keyword maps to only [out] directional attribute. But in the current implementation if the parameter has [in,out] directional attributes the Moq framework is gonna take it as 'out' parameter, which is wrong, because according to the rules in the article and msdn reference i posted above it gotta take it as 'ref' parameter, not the out. This situation may ocuur for example when you try to mock the COM interface, which has a method with [in,out] directional attributes in COM layer. In interop assembly this method wil be presented as 'ref' parameter, but moq framework is gonna see it as 'out' therefore you are not gonna be able to mock this method propperly and use it unit tests. So when determining the 'out' parameter we need to see not only that 'out' directional attribute is present, but that 'In' directional attribute is not present or otherwise it is the 'ref' parameter not the 'out'.

@koutinho koutinho changed the title The condition determining whether a parameter is 'out' parameter is Edit The condition determining whether a parameter is 'out' parameter is edited Jul 18, 2018
@koutinho koutinho changed the title The condition determining whether a parameter is 'out' parameter is edited The condition determining whether a parameter is 'out' parameter is checked Jul 18, 2018
@koutinho koutinho changed the title The condition determining whether a parameter is 'out' parameter is checked The condition determining whether a parameter is 'out' parameter is changed Jul 18, 2018
Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @koutinho, I haven't yet studied your pull request in detail, but it looks like a legitimate change. I will give a more detailed response later, for now I just have one small refactoring suggestion.

@@ -130,7 +130,8 @@ public MethodCall(Mock mock, Condition condition, LambdaExpression originalExpre
var argument = arguments[index];
if (parameter.ParameterType.IsByRef)
{
if ((parameter.Attributes & ParameterAttributes.Out) == ParameterAttributes.Out)
if ((parameter.Attributes & ParameterAttributes.Out) == ParameterAttributes.Out &&
(parameter.Attributes & ParameterAttributes.In) != ParameterAttributes.In)
Copy link
Contributor

@stakx stakx Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition could be simplified to:

if ((parameter.Attributes & (ParameterAttributes.In | ParameterAttributes.Out)) == ParameterAttributes.Out)

@koutinho
Copy link
Contributor Author

koutinho commented Jul 20, 2018

@stakx, Thanks for your response and good suggestion. This suggestion realy simplifies the condition and makes it more readable, so i refined the condition for it to conform to your suggestion. And i hope this change will be in the next release, beacuse i faced this problem in my work and currently i have to use my local modifyed copie of Moq and it would be good to use package from NuGet as usual. But i also belive that someone else may as well encounter with this problem and this change would do well for them too. Thanks.

@stakx stakx added this to the 4.9.1 milestone Jul 20, 2018
@stakx stakx merged commit 87545dc into devlooped:master Jul 20, 2018
@stakx
Copy link
Contributor

stakx commented Jul 20, 2018

@koutinho - Merged. Thank you for your contribution! 👍 Your changes will be in the next patch release (4.9.1), which will be released approx. in a few weeks' time.

@koutinho
Copy link
Contributor Author

koutinho commented Jul 20, 2018

Yes, sure, i can describe my problem. Basically my problem is as folllows : The project i am workin on uses Third Party COM library and this library has interface with this signature:
public interface IDualServer
{
//Some methods
int OutSteps(ref object vBegSignal, ref object vEndSignal, double dT1, double dTstep, int nCountSteps, out int nEndStep);
//Some other methods
}

So my code has a dependency on this interface and calls OutSteps method. In the unit-test project i have created the mock of this interface so that my class is gonna use this mock, not the real implementation of IDualServer that comes with COM library. So in the one test method i needed to mock this OutSteps method and use callback in it to perfome some actions upon calling it. So i wrote this:
mockDevice
.Setup(foo => foo.OutSteps(ref It.Ref<object>.IsAny, ref It.Ref<object>.IsAny,
It.IsAny<double>(), It.IsAny<double>(), It.IsAny<int>(), out stepCount))
.Returns(0)
.Callback((object vBegSignal, object vEndSignal, double dT1, double dTstep, int nCountSteps, int nEndStep) =>
{
//Some actions
});
But i was very suprised to see that vEndSignal in the callback was set to null not the value i passed when i call the stubbed method. But what was even more intersting is that vBegSignal was set propperly, even though it seemed to be defined the same
as vEndSignal was, they are both 'ref' parameters. After that i decided to look into sources of Moq. And it turns out that vEndSignal has [in, out] directional attributes, but vBegSignal only [in] in COM layer. It is the full IDL defenition of this interface:
[id(18), helpstring("method OutSteps")] HRESULT OutSteps([in] VARIANT* vBegSignal, [in,out] VARIANT* vEndSignal, [in] DOUBLE dT1, [in] DOUBLE dTstep, [in] LONG nCountSteps, [out] LONG* nEndStep, [out,retval] LONG* Result); This was a couse of this problem, Moq identified [in,out] as 'out' but instead it should've identified it as ref. I changed the condition and the problem solved. And after that i have created pull request to merge it into originall repository.
So i wanna thank you for quieck response, for your refactoring suggestion and your approval. it was very good to deal with you.

@stakx
Copy link
Contributor

stakx commented Jul 20, 2018

@koutinho - Thanks for the explanation. And thanks again for contributing! It's great that you didn't just post a bug report, but also took the initiative to look for the cause & provide a bugfix. 😃

I'll likely post a short notice here once 4.9.1 is released.

@stakx
Copy link
Contributor

stakx commented Sep 8, 2018

@koutinho - I've just pushed Moq 4.10.0 (which includes your improvement) to NuGet. It should be available shortly.

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