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

Make NSubContainer configurable #705

Closed
wants to merge 1 commit into from
Closed

Conversation

ffMathy
Copy link
Contributor

@ffMathy ffMathy commented Sep 26, 2022

This allows us to follow the open/closed principle by just overriding specific classes.

Use case: I am using Unity, and in Unity, we often use a UniTask instead of a Task, so I needed to override IAutoValueProvidersFactory with my own providers factory which would then register a custom IAutoValueProvider, but I didn't want to override anything else.

On another note, I didn't find a default IAutoValueProvider for ValueTask either - is this intentional?

Currently I am just casting, but that breaks encapsulation. It'd be better if we exposed the interface directly, as made in this pull request.

I don't see it as a bad thing to expose more, since it is used for testing.

This allows us to follow the open/closed principle by just overriding specific classes.

**Use case:** I am using Unity, and in Unity, we often use a `UniTask` instead of a `Task`, so I needed to override `IAutoValueProvidersFactory` with my own providers factory which would then register a custom `IAutoValueProvider`, but I didn't want to override anything else.

_On another note, I didn't find a default `IAutoValueProvider` for `ValueTask` either - is this intentional?_

Currently I am just casting, but that breaks encapsulation.

I don't see it as a bad thing to expose more, since it is used for testing.
@dtchepak
Copy link
Member

Hi @zvirja , as our container guru, do you have any thoughts on this? :)

@dtchepak dtchepak requested a review from zvirja October 15, 2022 04:40
@zvirja
Copy link
Contributor

zvirja commented Oct 20, 2022

Hey @ffMathy!

Actually the API you are changing is designed exactly with open/closed principle in mind 😊 It's open for extensibility, but is closed for modification and is in fact implementing the immutability pattern. To modify the container you can create a copy and then customize it as needed. I've even added the .Decorate() method which allows you to decorate the existing implementation by enhancing it - yet another example of the open/closed principle 😉

I do admin though that the flow is not really well-documented, so it might be a bit non-trivial to discover it.

The original design was to follow the following steps:

  • Create a derived container by calling NSubstituteDefaultFactory.DefaultContainer.Customize();
  • Customize it as needed
  • Replace the SubstitutionContext.Current as early as possible. Usage of Module Initializers is ideal here.

Here is example of the code to follow. From what you mentioned above, it seems it should perfectly cover the flow 😊

using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using NSubstitute;
using NSubstitute.Core;
using NSubstitute.Core.DependencyInjection;
using NSubstitute.Routing.AutoValues;
using Xunit;

namespace NSub;

internal static class ModuleInitializer
{
    [ModuleInitializer]
    internal static void ConfigureNSubstitute()
    {
        var customizedContainer = NSubstituteDefaultFactory.DefaultContainer.Customize();
        customizedContainer.Decorate<IAutoValueProvidersFactory>((factory, _resolver) => new CustomAutoValueProvidersFactory(factory));

        SubstitutionContext.Current = customizedContainer.Resolve<ISubstitutionContext>();
    }
}

internal class CustomAutoValueProvidersFactory : IAutoValueProvidersFactory
{
    private readonly IAutoValueProvidersFactory _original;

    public CustomAutoValueProvidersFactory(IAutoValueProvidersFactory original)
    {
        _original = original;
    }
    
    public IReadOnlyCollection<IAutoValueProvider> CreateProviders(ISubstituteFactory substituteFactory)
    {
        var originalProviders = _original.CreateProviders(substituteFactory);

        var customTaskProvider = new CustomTaskProvider(originalProviders);

        return new[] { customTaskProvider }.Concat(originalProviders).ToArray();
    }

    private class CustomTaskProvider : IAutoValueProvider
    {
        private readonly IReadOnlyCollection<IAutoValueProvider> _allProviders;

        public CustomTaskProvider(IReadOnlyCollection<IAutoValueProvider> allProviders)
        {
            _allProviders = allProviders;
        }
        
        public bool CanProvideValueFor(Type type) => type == typeof(Task);

        public object GetValue(Type type)
        {
            // use _allProviders to recursively resolve inner value if needed
            
            return Task.FromException(new InvalidOperationException("Failed"));
        }
    }
}

public class ExampleTest
{
    [Fact]
    public async Task ShouldResolveFailedTask()
    {
        // arrange
        var substitute = Substitute.For<IInterface>();

        // act
        var autoGeneratedTask = substitute.GetTask();

        // assert
        var ex = await Assert.ThrowsAsync<InvalidOperationException>(async () => await autoGeneratedTask);
        Assert.Equal("Failed", ex.Message);
    }

    public interface IInterface
    {
        Task GetTask();
    }
}

On another note, I didn't find a default IAutoValueProvider for ValueTask either - is this intentional?

I wouldn't say we had a deep though behind that, it just wasn't implemented.
In my day-to-day life I'm using NSubstitute in a combination with the AutoFixture library which provides way richer auto-generation capabilities that pure NSubstitute. And that one is handling the ValueTask type. I would say it perfectly complements NSubstitute, but of course requires a bit of learning 🤓

Let me know the snippet above is helping you to solve the needs.

As for the PR, I suggest to not merge it to stick to the current design.

@304NotModified
Copy link
Contributor

I think we could close this PR?

@dtchepak
Copy link
Member

Thanks for the contribution @ffMathy. Will close based on @zvirja 's feedback, but please re-open if you have additional questions.

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.

4 participants