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

Implemented #634: Arg.AnyType can be used in place of a generic type parameter #715

Merged
merged 6 commits into from Aug 27, 2023

Conversation

icalvo
Copy link
Contributor

@icalvo icalvo commented May 22, 2023

The following is a complete NUnit test with all the implemented features. The only compromise I took is Arg.DoForAny(action), instead of Arg.Do<Arg.AnyType>(action), but I couldn't make it work.

[TestFixture]
public class GenericArguments
{
    public interface ISomethingWithGenerics
    {
        void Log<TState>(int level, TState state);
    }

    [Test]
    public void Return_result_for_any_argument()
    {
        string argDoResult = null;
        int? whenDoResult = null;
        bool whenDoCalled = false;

        ISomethingWithGenerics something = Substitute.For<ISomethingWithGenerics>();
        something.Log(Arg.Any<int>(), Arg.DoForAny(a => argDoResult = a.ToString()));
        something
            .When(substitute => substitute.Log(Arg.Any<int>(), Arg.Any<Arg.AnyType>()))
            .Do(info =>
            {
                whenDoResult = info.ArgAt<int>(1);
                whenDoCalled = true;
            });

        something.Log(7, 3409);

        something.Received().Log(Arg.Any<int>(), Arg.Any<Arg.AnyType>());
        something.Received().Log(7, 3409);
        Assert.That(whenDoCalled, Is.True);
        Assert.That(argDoResult, Is.EqualTo("3409"));
        Assert.That(whenDoResult, Is.EqualTo(3409));
    }
}

@dtchepak
Copy link
Member

dtchepak commented Jun 3, 2023

This is really cool, thanks!
I'll have a play around and see if I can sort something out with Arg.Do<AnyType>.

@dtchepak
Copy link
Member

dtchepak commented Jun 4, 2023

I'm not convinced this alternative is a good idea, but one possible option is to store the specific value in AnyType:

public class AnyType {
    internal AnyType(object value) => Value = value;
    public object Value { get; private set; }
}
// ... 
public static ref T Do<T>(Action<T> useArgument)
{
    if (typeof(T) == typeof(AnyType))
    {
        return ref ArgumentMatcher.Enqueue<T>(
            new AnyArgumentMatcher(typeof(T)),
            x => {
                // Need to cast AnyType to object, then to T to be compatible with Action<T>
                object arg = new AnyType(x!);
                useArgument((T) arg);
            }
        );
    }

    return ref ArgumentMatcher.Enqueue<T>(new AnyArgumentMatcher(typeof(T)), x => useArgument((T) x!));
}

This let's us use the underlying value in the callback, but the .Value is a bit clunky to use.

[Test]
public void When_AnyType_Do() {
    var something = Substitute.For<ISomethingWithGenerics>();
    var output = new List<object>();
    // can we remove the `.Value`?
    something.Log(1, Arg.Do<Arg.AnyType>(x => output.Add(x.Value)));

    something.Log(1, 2);
    something.Log(1, "abc");

    Assert.AreEqual(new object[] { 2, "abc"}, output.ToArray());
}

I'm worried requiring .Value is non-obvious to callers, as it is natural to not see Arg.AnyType as an actual type, but instead more like as generic T. It might be better to stick with the existing DoForAny and exception from this MR.

@icalvo do you have any thoughts on this? (@alexandrnikitin , @zvirja , would be good to get your views on this too if you get a chance.)

@icalvo
Copy link
Contributor Author

icalvo commented Jun 5, 2023

@dtchepak I found a solution using an overload that seems to solve all the problems, please take a look!

@icalvo icalvo marked this pull request as ready for review June 13, 2023 09:32
@dtchepak
Copy link
Member

Hi @icalvo ,

Sorry for how long it has taken for me to get to this. 😓

I think I have a fix for the AppVeyor build. Please cherry-pick from #724 and review that commit to make sure it looks ok.

@icalvo
Copy link
Contributor Author

icalvo commented Aug 13, 2023

Hi @icalvo ,

Sorry for how long it has taken for me to get to this. 😓

I think I have a fix for the AppVeyor build. Please cherry-pick from #724 and review that commit to make sure it looks ok.

I'll do in a couple weeks as soon as I'm back from holiday.

@icalvo
Copy link
Contributor Author

icalvo commented Aug 25, 2023

Hi @icalvo ,

Sorry for how long it has taken for me to get to this. 😓

I think I have a fix for the AppVeyor build. Please cherry-pick from #724 and review that commit to make sure it looks ok.

It works and looks fine to me!

@dtchepak dtchepak merged commit 41bba78 into nsubstitute:main Aug 27, 2023
1 check passed
@dtchepak
Copy link
Member

Thanks so much for this @icalvo ! Sorry for bothering you on your holiday. Hope you had a nice break 😊

@dtchepak
Copy link
Member

@tpodolak Are any analyser changes required for this?

@akousmata
Copy link

When is this available? I pulled v 5.0.0 via nuget and this change wasn't present.

@ivanfranco502
Copy link

When is this available? I pulled v 5.0.0 via nuget and this change wasn't present.

v5.0.0 is from February. There is another fix related to AnyType done by icalvo, so probably both need to be merged before releasing it.

@tpodolak
Copy link
Member

tpodolak commented Sep 6, 2023

@tpodolak Are any analyser changes required for this?

@dtchepak I am not 100% sure, but we might start getting false positives from call info analyzers in cases like
https://github.com/nsubstitute/NSubstitute/pull/715/files#diff-3e581ff709baca8685f9657dd9c87a951216e44daf2eccf15986552881bd2499R29
but I would need to check that. Do we have any prerelease package ready so I can test couple of scenarios?

@dtchepak
Copy link
Member

dtchepak commented Sep 8, 2023

Do we have any prerelease package ready so I can test couple of scenarios?

The latest CI build should have this change:
https://ci.appveyor.com/project/NSubstitute/nsubstitute/build/artifacts

@tpodolak
Copy link
Member

tpodolak commented Sep 8, 2023

image
CallInfo analyzers will produce false positives. Added task to disable analysis when Arg.AnyType used nsubstitute/NSubstitute.Analyzers#212

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

5 participants