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

Arg.AnyType() for help with matching generic calls #634

Closed
dtchepak opened this issue Dec 5, 2020 · 13 comments
Closed

Arg.AnyType() for help with matching generic calls #634

dtchepak opened this issue Dec 5, 2020 · 13 comments
Labels
feature-request Request for a new NSubstitute feature

Comments

@dtchepak
Copy link
Member

dtchepak commented Dec 5, 2020

From #152 (comment):

Stumbled upon this while attempting to validate a call into Microsoft.Extensions.Logging.ILogger.

The logger interface takes a T state, but the "public" APIs (i.e. common extension methods) do not expose this directly.

For example, consumer code calls it like this:

logger.Critical("A critical problem has occurred!");

This is translated into a call to the interface's method, which has this signature:

public void Log<TState>(
            LogLevel logLevel,
            EventId eventId,
            TState state,
            Exception exception,
            Func<TState, Exception, string> formatter)

The string parameter that represents the message in the original call is not passed directly into this method, but instead is wrapped in an internal type and passed as state. Thus, it is actually impossible to perform an assertion on ILogger, since you cannot specify the internal type without getting a compilation error.

I actually solved this before by creating an abstract TestableLogger that exposes a non-generic version of the base interface method and then redirecting the implementation to it, and then using .Received on top of that new method. Obviously, this is extremely convoluted.

I'd really appreciate if NSubstitute provided a way to specify Any<AnyType>() that would check for any generic argument on a given parameter.

@dtchepak dtchepak added the feature-request Request for a new NSubstitute feature label Dec 5, 2020
@icalvo
Copy link
Contributor

icalvo commented May 18, 2023

My workaround was to use Moq for this specific case 😉. Simpler, but I'd rather uninstall Moq for good! Please implement this, since I believe that ILogger substitutes should be quite a frequent use case that NSubstitute users are forced to implement by themselves.

In my case I just wanted a callback to output the state parameter so that I can see the logs in the test output. So it'd be fine if this feature also plays nice with Arg.Do.

icalvo added a commit to icalvo/NSubstitute that referenced this issue May 22, 2023
@icalvo
Copy link
Contributor

icalvo commented May 23, 2023

I did a tentative implementation of this issue, you can look at this draft PR: #715

It's not a ready PR because I took a compromise that maybe has a better solution and also because the AppVeyor build is failing. It seems that the latest PR (#711 by @alexandrnikitin) broke it somehow.

@edandersen
Copy link

Any chance this can be merged? With Moq currently self destructing this is the last thing holding many people back from using NSubstitute.

@dtchepak
Copy link
Member Author

Hi @edandersen , I'll try to get to sort this out this weekend. Sorry for the delay.

@dtchepak
Copy link
Member Author

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.

FlorianHockmann added a commit to apache/tinkerpop that referenced this issue Aug 24, 2023
Still missing are the logging tests. Looks like NSubstitute still lacks
some functionality for this, but it should be supported soon:
nsubstitute/NSubstitute#634
@PieterJanse2001
Copy link

Any updates on this?

@icalvo
Copy link
Contributor

icalvo commented Aug 24, 2023

Any updates on this?

I was on holiday, today I will take a look!

@icalvo
Copy link
Contributor

icalvo commented Aug 25, 2023

@dtchepak Everything looks fine now, the build is passing so I think this is ready!

@akousmata
Copy link

Glad this is getting worked on! Any ETA on the publishing timeline for this set of changes? We are eagerly awaiting this feature 😄

@dtchepak
Copy link
Member Author

dtchepak commented Sep 3, 2023

@akousmata just waiting on review for 5.1.0.

Anyone that has time please take a look: #731

The PR updates CHANGELOG and bumps Castle.Core and System.Threading.Tasks.Extensions deps to latest versions, so no NSubstitute-internals knowledge required.

@alex289
Copy link

alex289 commented Sep 11, 2023

Hey @dtchepak, the PR seems to be completed. Is there a plan when the next Release is?

@dtchepak
Copy link
Member Author

@alex289 just published 5.1.0 now, should be available on nuget as soon as nuget.org finishes indexing it.

@zlangner
Copy link
Contributor

Since this issue is all about using NSubstitute to verify use of the ILogger, I'd like to let everyone know I have released NSubstitute.Community.Logging which aims to make this as easy as possible by providing extension methods that mirror the ILogger extension methods but they work with NSubstitute's Received/DidNotReceive methods. v0.0.1 is on nuget now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a new NSubstitute feature
Projects
None yet
Development

No branches or pull requests

7 participants