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

Argument matcher for a struct with a field with non-default value #766

Open
eValker opened this issue Jan 8, 2024 · 8 comments
Open

Argument matcher for a struct with a field with non-default value #766

eValker opened this issue Jan 8, 2024 · 8 comments
Labels
bug Reported problem with NSubstitute behaviour

Comments

@eValker
Copy link

eValker commented Jan 8, 2024

Describe the bug
I am suspecting that something is not working well with argument matchers that matches structs with a field with non-default value initializer. Maybe I am doing something wrong - int this case please point me to the right direction :)

To Reproduce

using NSubstitute;

var service = Substitute.For<IService>();

var executor = new ExecutorService(service);

executor.Execute();

service.Received(1).DoSomething(Arg.Any<StructObject>());
//service.Received(1).DoSomething(Arg.Is<StructObject>(static a => a.Index > 0)); // I tried Arg.Is<> but is does not change anything

public interface IService
{
    void DoSomething(StructObject objA);
}

public class ExecutorService
{
    private readonly IService _service;

    public ExecutorService(IService service)
    {
        _service = service;
    }

    public void Execute()
    {
        var objInstance = new StructObject
        {
            Index = 5
        };

        _service.DoSomething(objInstance);
    }
}

public struct StructObject
{
    public required int Index { get; init; } = -1;

    public StructObject()
    {
    }
}

Expected behaviour
I think is should just match provided object's instance. Instead of that I am getting exception below:

Unhandled exception. NSubstitute.Exceptions.RedundantArgumentMatcherException: Some argument specifications (e.g. Arg.Is, Arg.Any) were left over after the last call.

This is often caused by using an argument spec with a call to a member NSubstitute does not handle (such as a non-virtual member or a call to an instance which is not a substitute), or for a purpose other than specifying a call (such as using an arg spec as a return value). For example:

    var sub = Substitute.For<SomeClass>();
    var realType = new MyRealType(sub);
    // INCORRECT, arg spec used on realType, not a substitute:
    realType.SomeMethod(Arg.Any<int>()).Returns(2);
    // INCORRECT, arg spec used as a return value, not to specify a call:
    sub.VirtualMethod(2).Returns(Arg.Any<int>());
    // INCORRECT, arg spec used with a non-virtual method:
    sub.NonVirtualMethod(Arg.Any<int>()).Returns(2);
    // CORRECT, arg spec used to specify virtual call on a substitute:
    sub.VirtualMethod(Arg.Any<int>()).Returns(2);

To fix this make sure you only use argument specifications with calls to substitutes. If your substitute is a class, make sure the member is virtual.

Another possible cause is that the argument spec type does not match the actual argument type, but code compiles due to an implicit cast. For example, Arg.Any<int>() was used, but Arg.Any<double>() was required.

NOTE: the cause of this exception can be in a previously executed test. Use the diagnostics below to see the types of any redundant arg specs, then work out where they are being created.

Diagnostic information:

Remaining (non-bound) argument specifications:
    any StructObject

All argument specifications:
    any StructObject

To get rid of the exception I can do one of the things below:

  • change StructObject to become a class
  • remove Index property initializer (or make it equal to 0)

Environment:

  • NSubstitute version: 5.1.0
  • Platform: .net8 on windows

Additional context
I reproduced this case with the Moq library and it works fine.

Argument matcher for a struct with a field with non-default value

@alexandrnikitin
Copy link
Member

alexandrnikitin commented Jan 9, 2024

@eValker Thank you for the issue and the great repro. Yes, that's a bug indeed.

It happens because when we check the provided argument specification in

public bool IsSpecificationCompatible(IArgumentSpecification specification, object? argumentValue, Type argumentType)
{
var typeArgSpecIsFor = specification.ForType;
return AreTypesCompatible(argumentType, typeArgSpecIsFor)
&& IsProvidedArgumentTheOneWeWouldGetUsingAnArgSpecForThisType(argumentValue, typeArgSpecIsFor);
}
private bool IsProvidedArgumentTheOneWeWouldGetUsingAnArgSpecForThisType(object? argument, Type typeArgSpecIsFor)
{
return _defaultChecker.IsDefault(argument, typeArgSpecIsFor);
}

we compare the provided argument with its default value
private object DefaultInstanceOfValueType(Type returnType)
{
// Performance optimization for the most popular types.
if (returnType == typeof(bool))
{
return BoxedBoolean;
}
if (returnType == typeof(int))
{
return BoxedInt;
}
if (returnType == typeof(long))
{
return BoxedLong;
}
if (returnType == typeof(double))
{
return BoxedDouble;
}
return Activator.CreateInstance(returnType)!;
}

Where we use Activator.CreateInstance to create an instance of the specified type. It in its turn calls the default constructor of the struct which initializes the field with the default value not the specified one. That's why it doesn't consider the provided argument spec and throws RedundantArgumentMatcherException

It looks like a fix would be to propagate the matchArgs from ArgumentSpecificationsFactory down to the call stack and don't compare default values if it's MatchArgs.Any
https://github.com/nsubstitute/NSubstitute/blob/f470b5de58a0bb1f41157b7ce6210e35dd4f977f/src/NSubstitute/Core/Arguments/ArgumentSpecificationsFactory.cs#L49C16-L51

But I don't remember that part of code and frankly don't understand why we need to compare the provided spec with the default value of its type. @dtchepak @zvirja maybe you remember that? Is there a better fix?

@agarbutt
Copy link

agarbutt commented Apr 6, 2024

I am specifically encountering this behavior as well while exploring strongly typed ids (defined as structs wrapping the supporting value) in my code base.

It's not obvious if there is a work-around without changing how we would model these values.

@dtchepak
Copy link
Member

dtchepak commented Apr 9, 2024

Hi @alexandrnikitin 👋 😄

But I don't remember that part of code and frankly don't understand why we need to compare the provided spec with the default value of its type. @dtchepak @zvirja maybe you remember that? Is there a better fix?

IIRC we looked at default values to try to match up enqueued arg matchers with the actual args passed to members. (I'm not sure what the other options are there?)

The instance we return from Arg.* uses DefaultValueContainer. Can we use the same mechanism instead of Activator.CreateInstance?

@304NotModified 304NotModified added the bug Reported problem with NSubstitute behaviour label Apr 29, 2024
@davidlloyduk
Copy link

I've run across the same issue and I have attached a similar repro scenario

public class Tests
{
    [Test]
    public void Test1()
    {
        var service = Substitute.For<IService>();

        service.Query(Arg.Any<RequestDto>())
            .Returns(new ResponseDto());

        var result = service.Query(new RequestDto());
    }
}

public interface IService
{
    Task<ResponseDto> Query(RequestDto request);
}

public class SimpleService : IService
{
    public Task<ResponseDto> Query(RequestDto request)
    {
        return Task.FromResult(new ResponseDto());
    }
}

public struct RequestDto()
{
    public string Request { get; init; } = string.Empty;
}

public class ResponseDto
{
    public string Response { get; set; } = string.Empty;
}

The solution to get around this was to swap the struct to a class

public class RequestDto
{
   public string Request { get; init; } = string.Empty;
}

@jorik
Copy link

jorik commented Oct 7, 2024

Also having issues with value types generated by Vogen.

@SteveDunn
Copy link

Hi - I've got a bug report in Vogen about this. If I was to take a look at fixing it in NSubstitute, are there any pointers on how to start, e.g. testing etc.?

@davidlloyduk
Copy link

Yes you can use my minimal reproduction above

#766 (comment)

@SteveDunn
Copy link

Yes you can use my minimal reproduction above

#766 (comment)

Thanks, I was more looking for some pointers on what areas to look at in nsub itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reported problem with NSubstitute behaviour
Projects
None yet
Development

No branches or pull requests

8 participants