Git Product home page Git Product logo

Comments (8)

rbeurskens avatar rbeurskens commented on June 24, 2024

It might be adding this to the proxy classes solves this: (looking at code of ArgumentFormatter, which checks if object.ToString is overridden.)

public override string ToString() => this._matcher.ToString();

As an alternative, a new interface IDescribeMatches (?) could be added to test on and determine if an argument matcher provides a custom implementation to describe the expected argument. (instead of testing if object.ToString() is overridden and calling that for this purpose)

from nsubstitute.

dtchepak avatar dtchepak commented on June 24, 2024

Thanks for raising this @rbeurskens . This definitely needs to be improved.

ToString() seemed to work if enqueued directly (without being wrapped by a GenericToNonGenericMatcherProxy):

class CustomMatcher : IArgumentMatcher, IDescribeNonMatches, IArgumentMatcher<int>
{
    public string DescribeFor(object argument) => "failed";
    public bool IsSatisfiedBy(object argument) => false;
    public bool IsSatisfiedBy(int argument) => false;
    public override string ToString() => "Custom match";
}

[Test]
public void CustomArgMatch() {
    //ArgumentMatcher.Enqueue<int>(new CustomMatcher());
    var spec = new ArgumentSpecification(typeof(int), new CustomMatcher());
    SubstitutionContext.Current.ThreadContext.EnqueueArgumentSpecification(spec);
    _something.Received().Add(23, 0);
}
/*
  Failed CustomArgMatch [52 ms]
  Error Message:
   NSubstitute.Exceptions.ReceivedCallsException : Expected to receive a call matching:
        Add(23, Custom match)
Actually received no matching calls.
*/

I think we can just delegate to the matcher for these proxies?

    private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher
    {
        protected readonly IArgumentMatcher<T> _matcher;

        public GenericToNonGenericMatcherProxy(IArgumentMatcher<T> matcher)
        {
            _matcher = matcher;
        }

        public bool IsSatisfiedBy(object? argument) => _matcher.IsSatisfiedBy((T?)argument!);

        public override string ToString() => _matcher?.ToString() ?? ""; // TODO: fix nullability stuff here
    }

    private class GenericToNonGenericMatcherProxyWithDescribe<T> : GenericToNonGenericMatcherProxy<T>, IDescribeNonMatches
    {
        public GenericToNonGenericMatcherProxyWithDescribe(IArgumentMatcher<T> matcher) : base(matcher)
        {
            if (matcher is not IDescribeNonMatches) throw new SubstituteInternalException("Should implement IDescribeNonMatches type.");
        }

        public string DescribeFor(object? argument) => ((IDescribeNonMatches)_matcher).DescribeFor(argument);

        public override string ToString() => _matcher?.ToString() ?? "";    // TODO: fix nullability stuff here
    }

from nsubstitute.

rbeurskens avatar rbeurskens commented on June 24, 2024

I think we can just delegate to the matcher for these proxies?

This would not work as like there would be no proxy if the custom matcher does not override .ToString()

The problem with using .ToString() for this and reflection to get the type name of the matcher if it is not overridden makes this difficult to have the output the same as if there is no proxy in between because the formatter cannot access the matcher being proxied. (and the proxy does not know of the used formatter, unless it would be included/injected in its state).
It would be easier if there was an interface for this purpose, but that could mean a breaking change in the API.

public class ArgumentFormatter : IArgumentFormatter
{
    internal static IArgumentFormatter Default { get; } = new ArgumentFormatter();
    public string Format(object? argument, bool highlight)
    {
        var formatted = Format(argument);
        return highlight ? "*" + formatted + "*" : formatted;
    }

    private string Format(object? arg)
    {
        return arg switch
        {
            null => "<null>",
            string str => $"\"{str}\"",
            { } obj when HasDefaultToString(obj) => arg.GetType().GetNonMangledTypeName(), // <===
            _ => arg.ToString() ?? string.Empty
        };

        static bool HasDefaultToString(object obj)
            => obj.GetType().GetMethod(nameof(ToString), Type.EmptyTypes)!.DeclaringType == typeof(object); // <=== if obj is a proxy, we can't tell if it's overridden on the matcher itself
    }
}

Maybe this would be an option:

public interface IFormatable
{
    string Format(Func<object? ,string> format);
    // Alternative if exposing the proxied object through this property is no problem:
    // object? Arg {get;}
}
public class ArgumentFormatter : IArgumentFormatter
{
    private string Format(object? arg)
    {
        return arg switch
        {
            IFormattable f => f.Format(Format),
            // IFormattable f => Format(f.Arg),
            null => "<null>",
            string str => $"\"{str}\"",
            { } obj when HasDefaultToString(obj) => arg.GetType().GetNonMangledTypeName(), // <===
            _ => arg.ToString() ?? string.Empty
        };
        // (...)
    }
}
private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher, IFormatable
{
    public string Format(Func<object? ,string> format) => format(_matcher);
    // public object? Arg => _matcher;
    // (...)
}

It would still require custom IArgumentFormatter implementations to add support for the new interface, though just forwarding the .ToString() calls in the proxy may not return desired default output if the custom IArgumentMatcher<T> does not override it as the default string is created in the IArgumentFormatter implementation. (doing so would return the default object.String() output instead in that case)

from nsubstitute.

rbeurskens avatar rbeurskens commented on June 24, 2024

On a second thought and looking a bit further, I see how the code ends up running ArgumentFormatter.Format, as it is invoked from ArgumentSpecification like this, and I think I was making it more complicated than it has to be.
First I was wondering why those proxies are needed at all, but I see the generic IArgumentMatcher<T> (which could be changed to be contravariant, btw) is not used internally, where parameter values are typed/boxed as object. So they just exist to avoid forcing custom matchers to implement both generic and non-generic versions of the interface.

public class ArgumentSpecification : IArgumentSpecification
{
    public string FormatArgument(object? argument)
    {
        var isSatisfiedByArg = IsSatisfiedBy(argument);

        return _matcher is IArgumentFormatter matcherFormatter
            ? matcherFormatter.Format(argument, highlight: !isSatisfiedByArg)
            : ArgumentFormatter.Default.Format(argument, highlight: !isSatisfiedByArg);
    }
    // (...)
}

This means that we can just simply have the proxies implement IArgumentFormatter instead of adding a new interface or overriding .ToString().
I think this would keep it relatively clean and simple, not have any unwanted side effects or API changes and also work as desired if the custom matcher does not override .ToString().

private class GenericToNonGenericMatcherProxy<T> : IArgumentMatcher, IArgumentFormatter
{
    string Format(object? arg, bool highlight) =>
        _matcher is IArgumentFormatter matcherFormatter
            ? matcherFormatter.Format(argument, highlight)
            : ArgumentFormatter.Default.Format(argument, highlight);
    // (...)
}

(Since GenericToNonGenericMatcherProxyWithDescribe<T> inherits GenericToNonGenericMatcherProxy<T>, it only has to be added to the latter.)

from nsubstitute.

dtchepak avatar dtchepak commented on June 24, 2024

One problem is we have two formatting concepts here: formatting matching/non-matching calls, and formatting specifications.

The latter is the one causing a problem here. It currently works off CallSpecification.ToString from ReceivedCallsExceptionThrower, which also relies on arg specification ToString. So if we are keeping that approach implementing ToString on the wrappers should be sufficient (and probably document that ToString should be overridden to describe custom arg matchers on IArgumentMatcher or similar).

Alternatively we could add an IArgumentSpecFormatter or similar to reflect this. I don't feel strongly about this but am leaning toward the simpler option to avoid an API change. 🤔

from nsubstitute.

rbeurskens avatar rbeurskens commented on June 24, 2024

So if I understand correctly, the 'problematic' output comes from the ArgumentSpecification.ToString() call (which always calls .ToString() on the matcher no matter if it is overridden or not) and not from ArgumentSpecification.FormatArgument(object?) ?
If so, it would currently also display undesired output (from default object.ToString() implementation) if .ToString() is not overridden by the matcher even if it is not wrapped in a proxy and the reason why default matchers have proper output is that all override .ToString() (it would indeed make sense the proxies would follow that convention)
I agree that if this would be a new/revised API, it would be better to have a separate interface for this, but as it is not, it would be better to document that custom matchers should override .ToString() to control what is displayed for "expected".
Maybe also add .ToString() to the IArgumentMatcher and IArgumentMatcher<T> interfaces (with xmldocs) to make it clear (even though it does not enforce overriding because it is implemented on object) and/or provide an optional base class similar to

public abstract class ArgumentMatcher<T> : IArgumentMatcher<T>, IArgumentMatcher
{
    /// <inheritdoc cref="IArgumentMatcher.ToString" />
    public abstract override string ToString();
    public abstract bool IsSatisfiedBy(T? argument);
    bool IArgumentMatcher.IsSatisfiedBy(object? argument) =>IsSatisfiedBy((T?)argument);
}

By the way, it would be handy for custom argument matchers to have access to ArgumentFormatter.Default.

.. and maybe also change code to only use proxies if they are needed:

    /// <summary>
    /// Enqueues a matcher for the method argument in current position and returns the value which should be
    /// passed back to the method you invoke.
    /// </summary>
    public static ref T? Enqueue<T>(IArgumentMatcher<T> argumentMatcher)
    {
        var matcher = switch argumentMatcher
        {
            null => throw new ArgumentNullException(nameof(argumentMatcher)),
            IArgumentMatcher m => m,
            IDescribeNonMatches => new ArgumentMatcher.GenericToNonGenericMatcherProxyWithDescribe<T>(argumentMatcher),
            _ => new ArgumentMatcher.GenericToNonGenericMatcherProxy<T>(argumentMatcher)
        }
        return ref ArgumentMatcher.EnqueueArgSpecification<T>(new ArgumentSpecification(typeof (T), matcher));
    }

from nsubstitute.

dtchepak avatar dtchepak commented on June 24, 2024

I've created #806 to try to address this. Please take a look at let me know if you feel it is sufficient. 🙇

from nsubstitute.

rbeurskens avatar rbeurskens commented on June 24, 2024

Looks good to me on a first glance. Best of both worlds: An interface with a clear purpose for it without breaking code that uses the existing mechanism.
I added some review comments about some technical details in code that may or may not be relevant..

from nsubstitute.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.