Improve internal object relationship graph for easier testing

[tillig]

Problem Statement

In creating (internal) unit tests for new features you sometimes need to create new instances of things like RequestContext (or RequestContextBase), ResolveOperation, and so on.

Unfortunately...

• The base/abstract classes don't have zero-parameter constructors so you can't use Moq or other mocking frameworks to create them.
• The required constructor parameters themselves are also not easy to mock or set up.

This makes setup for tests somewhat fragile.

Desired Solution

I think we should analyze the relationships between things like:

• ResolveOperation (and ResolveOperationBase)
• ResolveRequest
• ResolveRequestContext (and ResolveRequestContextBase)
• Associated interfaces involved in the resolve pipeline

...and see if there can be some cleanup/simplification of the relationships between these things.

Alternatives You've Considered

As noted, I've tried using Moq to mock these things but they don't have zero-parameter constructors. The only other option is to try to create all of these things.

Additional Context

Here's what it looks like to try and create a ResolveOperation, a ResolveRequestContext, and an IResolveMiddleware for testing some of the diagnostics:

using var container = new ContainerBuilder().Build();
using var scope = container.BeginLifetimeScope() as ISharingLifetimeScope;
var operation = new ResolveOperation(scope);
var request = new ResolveRequest(
  new TypedService(typeof(string)),
  new ServiceRegistration(Mock.Of<IResolvePipeline>(), Mock.Of<IComponentRegistration>()),
  Enumerable.Empty<Parameter>());
var context = new ResolveRequestContext(operation, request, scope);
diagnostics.MiddlewareStart(operation, context, Mock.Of<IResolveMiddleware>());

I think the ResolveOperation construction is reasonable. However, I see that ResolveRequestContext requires three things in the constructor and... maybe it needs a zero-parameter constructor and settable properties.

By way of comparison, if you want to test something in ASP.NET Core, HttpContext (the abstract class) has a zero-parameter constructor so it can be mocked and DefaultHttpContext can be newed-up so if you need something concrete without mocking you can do that.

I think over time, as we've added ResolveRequest to the system, added pipelines, and done some major overhaul on the internals this has sort of grown and changed organically. If there's a way to clean it up and make it easier to work with, I think it might be more flexible for better testing as we add new features.

[alsami]

By way of comparison, if you want to test something in ASP.NET Core, HttpContext (the abstract class) has a zero-parameter constructor so it can be mocked and DefaultHttpContext can be newed-up so if you need something concrete without mocking you can do that.

I personally found it very hard to write isolated tests for HttpContext because of missing interfaces. Given that you have to create claims principles and things alike to have an actual user set in there.

I think the ResolveOperation construction is reasonable. However, I see that ResolveRequestContext requires three things in the constructor and... maybe it needs a zero-parameter constructor and settable properties.

Wouldn't it make sense to have interfaces for the type(s) you have mentioned, so one could actually write fully isolated unit tests, when required?

[alistairjevans]

I did start out with an interface IResolveRequestContext, but it was suggested to change it to a class because the implementing type has mandatory behaviour for everything to work properly.

Nullable Reference Types really aren't a fan of having a default constructor in a type that doesn't set things up correctly. It also means you couldn't use readonly properties for the things that really shouldn't be changed.

Perhaps we could have a derived mock type from ResolveRequestContext that provides a default constructor with forced nulls to the base?

[tillig]

I personally found it very hard to write isolated tests for HttpContext because of missing interfaces.

I think it depends on what you're testing. Like, yeah, testing authentication stuff can be challenging. But, like, if you have a controller that stores something in HttpContext.Items or something, or maybe you have logic that checks for some headers in the inbound request... that stuff isn't that hard to set up.

Wouldn't it make sense to have interfaces for the type(s) you have mentioned

Well, yeah. Maybe. I mean, some of the things have abstract base classes already; we could use those with some minor updates. In some cases we have public interfaces for end user/application consumption but abstract classes for internal use. Of course, if they're for internal use, we could update them to be more usable for internal testing.

I don't have exact suggestions on what to do right now. I think part of this is to look at what's there and see if there are things like:

• Abstract classes that can't be mocked
• Lots of required constructor parameters that make something hard to set up for a test
• Constructors that require duplicate stuff, like, if it takes a ResolveRequestContext and also a ResolveOperation but when the thing gets constructed normally it's just like new WhateverThing(context, context.Operation) - we could just pass in the context and not the extra/duplicate item.

I was just setting up the tests for some of the diagnostics work I'm doing and while it was easy enough to test at the very top, black-box level, it was really hard to set up tests for the internals and that seemed like a sorta red flag.

[tillig]

Nullable Reference Types really aren't a fan of having a default constructor

Ooooo this is a good point. I'm not sure how to work around stuff like that.

[alistairjevans]

Most of the recommendations are either:

• Don't have a default constructor if your object isn't 100% valid without one.
• If you know that some subsequent initialisation code that runs immediately after the constructor always sets a given property (e.g. EF Core) then cheat.

I actually contributed a section to the .NET docs about this: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-migration-strategies#late-initialized-properties-data-transfer-objects-and-nullability

[tillig]

Perhaps the abstract base classes should use abstract properties along with their default implementation logic? Or maybe that last bit, with the null! is an answer.

I could totally get in the weeds on this in the diagnostic work so I'm not going to (see: this. references 😆 ). But I think there could be some low-hanging fruit here.

For example, I see ResolveRequestContext takes a ResolveOperationBase and an ISharingLifetimeScope. ResolveOperationBase also has an ISharingLifetimeScope. ResolveRequestContext gets created during ResolveOperationBase.GetOrCreateInstance... but I think this is always the same lifetime scope instance. That is, we wouldn't need to pass in a lifetime scope in the constructor because it's always the same as the one in the ResolveOperationBase. I think. There's one other reference to GetOrCreateInstance but, again, it looks like it's only ever passing the same scope in. Probably easy enough to test out.

Point being, I don't know if we have to go full-bore into perfect testability, but maybe there are some things that could be cleaned up a little to just make it easier. Maybe?

[alistairjevans]

Agreed that it's definitely worth taking a look at it and seeing if it can't be improved.

The pipelines type relationships were designed quite...iteratively...so would be good to have another look.

[alsami]

I'd like to look into it this weekend a bit if you guys don't mind. About time to get a better understanding of internals anyway.

[tillig]

@alsami That'd be awesome. Go for it!

[alsami]

Small update:
I've made some diagrams visualizing the dependencies for me to get a better understanding.

Related because of the missing tests: I've asked myself, if it's okay if we were to add coverlet.collector and coverlet.msbuild to generate code-coverage with some open-coverage report tool?

I'd wait for #1169 to be merged and add it to the pipeline, if you guys don't mind @tillig @alistairjevans

[alistairjevans]

I'm definitely in favour of measuring our coverage, provided we don't rely solely on those numbers when evaluating PRs. I've done this before in my AutoStep tests, with that same report generator, and it's a really handy report.

Bonus points for getting it included in the PR check results (I've seen that done elsewhere, but not sure how you get AppVeyor to output a report as a CI check...).

[alsami]

Bonus points for getting it included in the PR check results (I've seen that done elsewhere, but not sure how you get AppVeyor to output a report as a CI check...).

Hehe, will go for the minimum now and build based on that. I am sure we can do that. On gitlab it's possible to add some regex to show the regression of coverage within the PR's, I am sure there is a way for AppVeyor as well.

[tillig]

Here are a couple that are sort of annoying that I'm hitting right now trying to get the DOT graph diagnostics tests working:

In order to report success or failure on a request/operation, I need a ResolveOperationBase and a ResolveRequestContextBase. These are public abstract classes where the derived concrete classes ResolveOperation and ResolveRequestContext are internal.

I can't Mock.Of<ResolveOperationBase>() because it takes constructor parameters. Same thing with ResolveRequestContextBase.

Now, granted, I can create some test derived versions of these that basically just pass along the constructor parameters, but it'd be nice if it was easier. I feel like I'm sort of fighting the framework to write an extension and get it tested. I do see the Autofac.Pooling stuff didn't require quite as many gyrations, so maybe it's just inherent in the type of extension I'm working on and there's nothing we want to do about it... but it'd be nice if it was easier.

[tillig]

Correction: I'm totally blocked because the ResolveRequestContextBase constructor is internal so I can't create my own derived class and get it instantiated.

I'll have to submit a PR to get a couple of these things unlocked a bit for testing.

[alsami]

Correction: I'm totally blocked because the ResolveRequestContextBase constructor is internal so I can't create my own derived class and get it instantiated.

What would be a quick solution for that? Make the constructor public? I guess it was intended to make it internal.

[tillig]

I don't know. Maybe make it protected? I haven't looked at it yet, trying to wrap up some other stuff. Might not be today. Day job and all.

[alsami]

Maybe make it protected?

Can do that quickly.

[tillig]

I don't know if that's the right solution. It's not a recommendation, it's a total stab in the dark. As internal, you can call it directly from other internal things. As protected, you're limited to the inheritance chain. It needs more time dedicated to it than what I'm able to give it literally right now, sorry.

[alsami]

Correction: I'm totally blocked because the ResolveRequestContextBase constructor is internal so I can't create my own derived class and get it instantiated.

I'll have to submit a PR to get a couple of these things unlocked a bit for testing.

I am just playing with the latest sources a bit. Seeing now what's the problem with ResolveRequestContextBase.

Basically for this class, to make isolated testing for Diagnostic i.e. easier, I'd suggest to create an interface:

public interface IResolveRequestContext : IComponentContext {
   // public stuff like properties in here.
}

Now for instance the extensions of DiagnosticListener would consume IResolveRequestContext, rather than ResolveRequestContextBase.

Before I start doing that, anyone has a better idea or something that speaks against using an interface for that scenario?
Would definitely simplify scenarios like that:

var diagnostics = new DiagnosticListener("SomeDiagnosticListener");
using var container = new ContainerBuilder().Build();
using var scope = container.BeginLifetimeScope() as ISharingLifetimeScope;
var operation = new ResolveOperation(scope!, diagnostics);
var request = new ResolveRequest(
    new TypedService(typeof(string)),
    new ServiceRegistration(Mock.Of<IResolvePipeline>(), Mock.Of<IComponentRegistration>()),
    Enumerable.Empty<Parameter>());
var context = new ResolveRequestContext(operation, request, scope, diagnostics);
diagnostics.MiddlewareStart(context, Mock.Of<IResolveMiddleware>());

[alistairjevans]

I'm ok with defining an interface that exposes the necessary properties (I believe that was how it was originally, but the presence of some behaviour in the context object prompted a change to an abstract class instead).

[alsami]

I'm ok with defining an interface that exposes the necessary properties (I believe that was how it was originally, but the presence of some behaviour in the context object prompted a change to an abstract class instead).

I'd still keep the abstract class, just change all parameters to use IResolveRequestContext and let the callers still decide on which implementation they want to pass in.

[alistairjevans]

It might be one of those "try it and see how it feels" deals.

[tillig]

I'm OK with it but... it feels like an over-abundance of abstractions. We've got the actual ResolveRequestContext, and there's a ResolveRequestContextBase, and then there's an IResolveRequestContext? Why do we need a public ...Base if we switch to the interface? Relating it to HttpContextBase and DefaultHttpContext, the point of the...Base is to be that public abstraction.

I think if we switch to an interface, then the public base/abstract class is not interesting. If we keep the base abstract class, perhaps the required properties become abstract instead of virtual or something. I'm just not sure about having both the abstract and interface.

[alsami]

Why do we need a public ...Base if we switch to the interface? Relating it to HttpContextBase and DefaultHttpContext, the point of the...Base is to be that public abstraction.

As far as I remember @alistairjevans recently mentioned that he wanted to use it as in extension point in other libs.
If that's not the case, I could move the existing behavior of ResolveRequestContextBase into ResolveRequestContext. Which would also mean that ResolveRequestContext has to be made public.

[tillig]

Well, right - but if it's an extension point then the constructor can't be internal, which is the whole issue with that particular class. I'm happy to have it remain if the constructor is also usable outside core Autofac, which would also, then, negate the need for the interface. The simple test would be: In a non-core-Autofac library (not the unit tests, either, which have access to internals) can I make a derived class of this thing?

[alsami]

Well, right - but if it's an extension point then the constructor can't be internal

That's a valid point. I am gonna go for the interface everywhere solution, removing the base-implementation and making ResolveRequestContext itself public.

[alistairjevans]

If we're going interface everywhere, shouldn't ResolveRequestContext be completely internal, and then if someone wants to test something that takes the context, they need to mock it?

I.e. perhaps the only public item should be the interface.

[alsami]

I.e. perhaps the only public item should be the interface.

Sounds good, I am gonna give it a try now, plenty of stuff to adjust here and there. Will submit a PR as soon as it builds again... which might take a while 😄

[alsami]

• Object relation for ResolveRequestContext & ResolveRequestContextBase optimization -> interface has been introduced
• Object relation for ResolveOperation & ResolveOperationBase optimization
• Bring back ResolveRequestContextBase only containing abstract methods and no interface, because of performance regression and adjust tests
• Make use of refactoring changes in ActivatorPipelineExtensions (Autofac.Tests)
• Adjust tests that were weirdly creating stubs / mocks of ResolveOperation & ResolveOperationBase optimization

---

• Maybe see how ResolveRequest can be improved and improve it.
• Maybe see how ServiceRegistration can be improved and improve it.

[alistairjevans]

@tillig, now that the names and structures are stable, are you unblocked on the DOT graph implementation?

[alistairjevans]

On a related note, I'd like to do the merge into develop this week so we can start looking at the integration packages and the remaining style changes; @alsami, are you ok to leave your unit test changes until we're on develop?

[alsami]

@alsami, are you ok to leave your unit test changes until we're on develop?

Yes sure. I wanted to start with the remaining stuff somewhere this week. Just let me know when you have merged to develop, will wait till then.

[tillig]

I didn't know if it was all stable yet. Yeah, if it's good to go I'll get back on the DOT stuff.

I think merging to develop is probably fine. I can't see that we'd be issuing any 5.x fixes between now and 6.

[alsami]

Looking at the tests, they use the implementation of ResolveOperation but passing along mocks as parameters (mostly). Sometimes they use a container to get the ISharingLifetimeScope. Which actually seems fine to me. I'd think, that there are no changes required. What do you think?

@tillig @alistairjevans

[tillig]

The changes that were made were a definite improvement. Being able to mock stuff is a great change to what was going on.

ResolveRequest and ServiceRegistration were the next items you couldn't mock - again, no zero-parameter constructor, so you have to pass mocks for parameters.

private static ResolveRequest MockResolveRequest()
{
    return new ResolveRequest(
        new TypedService(typeof(string)),
        new ServiceRegistration(Mock.Of<IResolvePipeline>(), Mock.Of<IComponentRegistration>()),
        Enumerable.Empty<Parameter>());
}

Should we look at messing with it? I mean, it's just slightly more difficult than it needs to be, but I also don't know if lots of folks will be writing tests at this level so maybe it's good enough?

[alsami]

Yeah, if it's hard to test or not as easy as it should be, I guess making it easier makes sense. Putting it on the list and will look into it this weekend.

[alistairjevans]

I'd propose that we draw a line under these changes now. v6 is pretty stable, and we're mostly doing tidy up in prep for a release.

I'm happy to take any test updates/improvements, but I think we're probably done with any further internals refactoring.

[tillig]

I'll buy that. It's been improved and I'm not sure I have any actionable suggestions on further changes.

[alistairjevans]

Cool; I'm going to close this then. Thanks @alsami for all your work on the changes.