dotnet-operator-sdk icon indicating copy to clipboard operation
dotnet-operator-sdk copied to clipboard

MockKubernetesClient challenging to test with as singleton

Open tillig opened this issue 4 years ago • 4 comments

Is your feature request related to a problem? Please describe.

I have a controller I'm writing and it's handling created/updated/not-modified events. As part of that, I'm testing that the information retrieved from and sent to Kubernetes using the IKubernetesClient is correct.

The MockKubernetesClient implementation, while simple and convenient, currently has a couple of challenges.

  1. I can't set up a sequence of responses from the same call. Like if I wanted to have two Get operations, I can't set that up unless I write my code in a way such that I can change the MockKubernetesClient.GetResult between calls.
  2. It's a singleton, so (as noted in the docs) I can't run tests that use it in a parallel fashion.
  3. I can't very easily replace it because the KubernetesOperatorFactory removes and overrides any other mock I might try to use.

Describe the solution you'd like

It'd be nice if I could somehow hook, say, Moq in here, such that it's a Mock<IKubernetesClient> rather than MockKubernetesClient.

After digging into how that might work, I have a few sort of disjointed but slightly related ideas.

First, if there was an equivalent for WebApplicationFactory<T>.WithWebHostBuilder for KubernetesOperatorFactory I could see a person might be able to do something like this:

public class MyControllerTests : IClassFixture<KubernetesOperatorFactory<TestStartup>>
{
  private readonly KubernetesOperatorFactory<TestStartup> _factory;
  public MyControllerTests(KubernetesOperatorFactory<TestStartup> factory)
  {
    this._factory = factory.WithWebHostBuilder(builder =>
    {
      builder.ConfigureTestServices(services =>
      {
        services.AddScoped<IKubernetesClient>(sp => Mock.Of<IKubernetesClient>());
      });
    });
  }
}

A lot of issues might be solved if the MockKubernetesClient itself was Scoped instead of Singleton so tests don't interfere with each other. It'd mean the MockedKubernetesClient property wouldn't work since you'd have to get the appropriate service out of request services.

Which leads me to thinking about possibly an IHttpContextAccessor style thing where an AsyncLocal<T> could allow access to the current request services, sort of like HttpContext but for operators. You could probably get around that if the controller had a public property on it to hold the IKubernetesClient and folks could access the mock that way, but it seems possibly too opinionated over getting the mock from the request services directly.

I also wonder if the functionality on the KubernetesOperatorFactory<T> should be extension methods on WebApplicationFactory<T> instead. The convenience properties for the mock client and whatnot wouldn't be available, but on first glance it looks like most of it could be handled by some extensions, which would mean you wouldn't need to replicate stuff like WithWebHostBuilder. It'd just work.

I could absolutely be missing something obvious here like maybe this already exists or maybe I should I'm doing it wrong.

tillig avatar Sep 13 '21 22:09 tillig

I've gone spelunking through WebApplicationFactory and HostFactoryResolver and I have a better understanding of some of the challenges here. Just thinking out loud to organize my thoughts...

  • WebApplicationFactory really wants to find a Program.CreateHostBuilder method in an assembly and run that. It makes sense why KubernetesOperatorFactory overrides CreateHostBuilder and ConfigureWebHost - to avoid that requirement.
  • KubernetesOperatorFactory.EnqueueEvent is where things become challenging for extension methods because MockManagedResourceController is internal.
  • Individual event-level mocks would need to be registered in the logical equivalent of HttpContext.RequestServices but there's no point to set up those mocks between the creation of the child scope and the event being enqueued and handled. EnqueueEvent is an atomic thing - create scope, execute controller with event. The mocks would need to be registered at a higher level (e.g., globally) to work, which means "global" needs to be per-test. WebApplicationFactory does that using WithWebHostBuilder, so having an equivalent becomes really interesting here.

I'm starting to wonder if basing the KubernetesOperatorFactory on a derived WebApplicationFactory is interesting. It seems like there's about 50% of the WebApplicationFactory logic that's interesting, but the other 50% (stuff like setting up the fake HttpClient, enumerating test assemblies, assuming Program.CreateHostBuilder is a thing) that is really not interesting and requires workarounds. Given ASP.NET Core is MIT license doing a sort of fork-and-modify to support the stuff here would be a possibility. Maybe I'll try that in my current project just to see what that would look like.

tillig avatar Sep 14 '21 14:09 tillig

That was somewhat of a short-lived experiment. Due to IManagedResourceController and IComponentRegistrar being internal, I couldn't really replicate this bit from KubernetesOperatorFactory:

services.RemoveAll<Func<IComponentRegistrar.ControllerRegistration, IManagedResourceController>>();
services.AddSingleton(
  s => (Func<IComponentRegistrar.ControllerRegistration, IManagedResourceController>)(r =>
    (IManagedResourceController)ActivatorUtilities.CreateInstance(
      s,
      typeof(MockManagedResourceController<>).MakeGenericType(r.EntityType),
      r)));

I mean, it's not technologically impossible but the reflection and manual System.Linq.Expressions work to make it happen requires more time than I have to allocate to it.

That said, I got far enough into it that it seems like a custom KubernetesOperatorFactory that doesn't actually derive from WebApplicationFactory would be possible. The drawback would be if you wanted to test the health checks, the HttpClient bit wouldn't be there. Maybe that's OK?

In the meantime, I guess I'll change the design of my controller and my testing such that:

  • I won't test the actual event handlers since it all ends up in a general "reconcile" sort of handler anyway.
  • I'll create the instance of my controller directly rather than using the KubernetesObjectFactory. I'll pass in the mocked IKubernetesClient directly to the constructor.
  • I'll call my "reconcile" method for testing (e.g., make it public) and do all IKubernetesClient work from in there.

tillig avatar Sep 14 '21 15:09 tillig

Hey @tillig

Thanks for the detailed analysis! I totally agree with you on this matter. When I first created the application factory, I just had simple testing in mind. With the discussion over at #281, the general "reconcile" is a possible outcome. During this breaking change, we're able to break / change other things. This might be a good opportunity to change the testing helpers as well.

I do agree that it'd be a better solution if some frameworks like Moq could be used to inject a IKubernetesClient instance. While I don't like to depend on a fixed framework, passing the interface would solve that matter.

I do think this matter over as soon as I have the time, or if you like, you can create a proposal on how to implement such a testing helper.

buehler avatar Sep 19 '21 10:09 buehler

Then only thing I could think of right now, is to add some method to set the IKubernetesClient in the KubernetesOperatorFactory. The client is inserted as "transient" and overwritten in the factory to be singleton. When the factory would provide a way to insert the kubernetes client with a transient factory method, then each call would have a new "mock instance". But I cannot grasp a concept now on how to improve the testing system.

buehler avatar Oct 11 '21 07:10 buehler