Calls to configure internal methods will always call base class

[robertbissonnette]

Describe the bug NSubstitute is calling base methods even during a configure overidden call when the target member is internal

To Reproduce

// FAILS
[Fact]
public void TestInternalCall()
{
    var system = Substitute.ForPartsOf<MyClass>();
    var expected = "A new hope dawns";
    system.Configure().InternalCall().Returns(expected); //This line will call the method and result in an exception.

    var result = system.InternalCall();

    Assert.Equal(expected, result);
}

// FAILS
[Fact]
public void TestInternalCallWithArg()
{
    var system = Substitute.ForPartsOf<MyClass>();
    var expected = "A new hope dawns";
    system.InternalCallWithArg(Arg.Any<string>()).Returns(expected); //This line will call the method and result in an exception.

    var result = system.InternalCallWithArg("junk");

    Assert.Equal(expected, result);
}

// PASS
[Fact]
public void TestPublicCall()
{
    var system = Substitute.ForPartsOf<MyClass>();
    var expected = "A new hope dawns";
    system.Configure().PublicCall().Returns(expected); //No exception

    var result = system.PublicCall();

    Assert.Equal(expected, result);
}

public class MyClass
{
    // Call would need Configure to properly detect this is a configuration call
    internal virtual string InternalCall()
    {
        throw new Exception("I Was Called");
    }
    // Call would not need Configure to properly detect configuration call when used with Arg type
    internal virtual string InternalCallWithArg(string arg1)
    {
        throw new Exception("I Was Called");
    }
    public virtual string PublicCall()
    {
        throw new Exception("I Was Called");
    }
}

Expected behaviour Calls on internal virtual methods should not result in calls to the base type.

Environment:

• NSubstitute version: 3.1.0 && 4.0.0
• Platform: dotnetcore2.1 project on windows

Additional context The above uses xUnit 2.3.1 to perform the calls, but the scenarios can be extracted if desired.

[dtchepak]

Thanks for the excellent bug report and repro. 👍 Have reproduced this on dotnetcore2.1 on Mac.

[robertbissonnette]

Thanks!

I tracked down where and why this is happening, however I am not sure how to proceed at this point.

The issue while it is exhibiting in the calls for NSubstitute it actually is caused by the Castle.Core proxy generator. It doesn't appear that there is a manner in which the caller (NSubstitute) can override the functionallity.

Below is the call stack to the exclusion of the method from the emitted proxy. This is against Castle.Core.4.3.1 Commit # 1ba6c894ea4b011b524782f2eec6e89b6245627c NSubstitute.4.0.0 Commit # d22b66a00f6de7f92575dfe977b2d36f1b4d0157

Castle.Core.dll!Castle.DynamicProxy.ProxyUtil.AreInternalsVisibleToDynamicProxy(System.Reflection.Assembly   asm) Line 133
Castle.Core.dll!Castle.DynamicProxy.ProxyUtil.IsAccessibleMethod(System.Reflection.MethodBase   method) Line 197
Castle.Core.dll!Castle.DynamicProxy.Contributors.ClassMembersCollector.GetMethodToGenerate(System.Reflection.MethodInfo   method, Castle.DynamicProxy.IProxyGenerationHook hook, bool isStandalone)   Line 31
Castle.Core.dll!Castle.DynamicProxy.Contributors.MembersCollector.AddMethod(System.Reflection.MethodInfo   method, Castle.DynamicProxy.IProxyGenerationHook hook, bool isStandalone)   Line 181
Castle.Core.dll!Castle.DynamicProxy.Contributors.MembersCollector.CollectMethods(Castle.DynamicProxy.IProxyGenerationHook   hook) Line 105
Castle.Core.dll!Castle.DynamicProxy.Contributors.MembersCollector.CollectMembersToProxy(Castle.DynamicProxy.IProxyGenerationHook   hook) Line 77
Castle.Core.dll!Castle.DynamicProxy.Contributors.ClassProxyTargetContributor.CollectElementsToProxyInternal(Castle.DynamicProxy.IProxyGenerationHook   hook) Line 47
Castle.Core.dll!Castle.DynamicProxy.Contributors.CompositeTypeContributor.CollectElementsToProxy(Castle.DynamicProxy.IProxyGenerationHook   hook, Castle.DynamicProxy.Generators.MetaType model) Line 51
Castle.Core.dll!Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateType(string   name, System.Type[] interfaces, Castle.DynamicProxy.Generators.INamingScope   namingScope) Line 57
Castle.Core.dll!Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateCode.AnonymousMethod__0(string   n, Castle.DynamicProxy.Generators.INamingScope s) Line 45
Castle.Core.dll!Castle.DynamicProxy.Generators.BaseProxyGenerator.ObtainProxyType(Castle.DynamicProxy.Generators.CacheKey   cacheKey, System.Func<string, Castle.DynamicProxy.Generators.INamingScope,   System.Type> factory) Line 415
Castle.Core.dll!Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateCode(System.Type[]   interfaces, Castle.DynamicProxy.ProxyGenerationOptions options) Line 45
Castle.Core.dll!Castle.DynamicProxy.DefaultProxyBuilder.CreateClassProxyType(System.Type   classToProxy, System.Type[] additionalInterfacesToProxy,   Castle.DynamicProxy.ProxyGenerationOptions options) Line 68
Castle.Core.dll!Castle.DynamicProxy.ProxyGenerator.CreateClassProxyType(System.Type   classToProxy, System.Type[] additionalInterfacesToProxy,   Castle.DynamicProxy.ProxyGenerationOptions options) Line 1538
Castle.Core.dll!Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(System.Type   classToProxy, System.Type[] additionalInterfacesToProxy,   Castle.DynamicProxy.ProxyGenerationOptions options, object[]   constructorArguments, Castle.DynamicProxy.IInterceptor[] interceptors) Line   1440
NSubstitute.dll!NSubstitute.Proxies.CastleDynamicProxy.CastleDynamicProxyFactory.CreateProxyUsingCastleProxyGenerator(System.Type   typeToProxy, System.Type[] additionalInterfaces, object[]   constructorArguments, Castle.DynamicProxy.IInterceptor[] interceptors,   Castle.DynamicProxy.ProxyGenerationOptions proxyGenerationOptions) Line 82
NSubstitute.dll!NSubstitute.Proxies.CastleDynamicProxy.CastleDynamicProxyFactory.GenerateProxy(NSubstitute.Core.ICallRouter   callRouter, System.Type typeToProxy, System.Type[] additionalInterfaces,   object[] constructorArguments) Line 38
NSubstitute.dll!NSubstitute.Proxies.ProxyFactory.GenerateProxy(NSubstitute.Core.ICallRouter   callRouter, System.Type typeToProxy, System.Type[] additionalInterfaces,   object[] constructorArguments) Line 21
NSubstitute.dll!NSubstitute.Core.SubstituteFactory.Create(System.Type[]   typesToProxy, object[] constructorArguments, bool callBaseByDefault) Line 61
NSubstitute.dll!NSubstitute.Core.SubstituteFactory.CreatePartial(System.Type[]   typesToProxy, object[] constructorArguments) Line 48

Adding the below allows for the class members to be included, but it requires changing the assembly under test to do so. And it also results in a leaking of internal implementation for NSubstitute using Castle at the assembly under test.

[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]

That's as far as I got with it, hopefully this is enough for you to proceed. It may require a feature request from Castle.Core; There may be a way to do this from a MixIn; I can't tell at this point.

[zvirja]

@robertbissonnette Thanks for reporting the issue and the initial investigation you did!

The way Castle Proxies library works is pretty simple:

• Generate a dynamic assembly
• Define a dynamic type which inherits/implement the required base class/interface
• Override all the virtual members (if applies) to intercept their invocation.

Library capabilities are limited to what you would be able to do manually if you define a regular assembly and try doing the same.

In this particular example, the internal member is not visible from the dynamic assembly, so Castle.Proxies cannot override it to enable interception. There are 2 ways to solve it:

• Define the InternalsVisibleTo attribute as you did
• Change modifies to protected internal, so member is visible from the external assemblies as well.

While the solution might work, you should also consider that the issue might be caused by the design - it's questionable whether internal members should be tested or not. Usually, best testing approaches stick to the idea that you should test the public contract only. Nevertheless, it's a separate topic 😉

[robertbissonnette]

@zvirja Thanks

It looks as if you came up with the exact same results that I did. It looks like that part of the library will have to be re-worked.

I do believe that the documentation should be updated for the project to state that this is a limititation with workarounds. It clearly mentions the virtual requirement but not the public/inheritance requirement.

The locations I would suggest are at minimum these two http://nsubstitute.github.io/help/creating-a-substitute/#substituting_infrequently_and_carefully_for_classes http://nsubstitute.github.io/help/getting-started/#using-nsubstitute-in-a-test-fixture

[dtchepak]

I've raised #498 to update the documentation. Thanks for tracking down the locations to update @robertbissonnette!

@tpodolak Is it worth including this case for analyzers? NS2003 detects and suggests workarounds for internal types, but I don't think there are detections for internal members at present.

@robertbissonnette Am I correct in guessing your code under test was in an assembly that already had InternalsVisibleTo("My.Test.Assembly") so you could access the internal members in the tests, but the tests failed until you also added InternalsVisibleTo("DynamicProxyGenAssembly2")?

[robertbissonnette]

@dtchepak Yes, you are correct. It was a failure of the test resulting in a Null Reference due to use of Arg.Any<T>(). Tests project had visibility to test method but not the castle proxy dynamic library.

In the case of fixes.. Any of the following worked:

• marking method under test as public
• marking method under test as protected internal
• adding assembly level attribute InternalsVisibleTo("DynamicProxyGenAssembly2") to the library containing the target of unit test.

I personally would recommend adding it to the analyzers, it is a rather subtle thing that I only caught due to a null reference being thrown. The visibility from test project into subject lib gives a false representation that it is actually able to override.

Referring to NS2003, there are no checks on members, only on the type itself. This is the analyzer method in question AnalyzeTypeAccessability

Based on the conventions in the Analyzers it would be another use case.

[dtchepak]

I've raised https://github.com/nsubstitute/NSubstitute.Analyzers/issues/66 on the Analyzers project to cover this case.

[tpodolak]

@dtchepak NS2003 checks type accessibility when creating a substitute, so internal member issue doesn't fit well into this category IMHO. I would probably put it into 5xxx or alternatively into 1xxx. One way or another I think it is good to have an analyzer for that. I will try to start working on that during the weekend