simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 154 forks source link

Dependencies that Implement IDisposable in a Testing context. #968

Closed IfThingsThenStuff closed 9 months ago

IfThingsThenStuff commented 1 year ago

I have a dependency that implements IDisposable and I'm struggling to figure out how to write tests that use this dependency. When I use the dependency in a web-request as I understand it there is automating scoping taking place to ensure all dependencies are resolved within a scope.

I'm not sure how to achieve scoping within tests to handle one dependency I have that implements IDisposable. I'm hoping I don't need to wrap every single test in using block with AsyncScopedLifestyle.BeginScope in it. I'm also using the Transient lifestyle thinking that separate tests should get their own instances of dependencies to allow for test isolation. When working within a web request things behave fine if I register my instance using container.Register<IClassThatHasDispose, ClassThatHasDispose>(Lifestyle.Scoped);.

Within the context of a test I was hoping that each test would effectively be it's own scope and that I wouldn't have to worry about that. When I register my disposable class using container.Register<IClassThatHasDispose, ClassThatHasDispose>(Lifestyle.Scoped); in a test I get the error "ClassThatHasDispose is registered using the 'Async Scoped' lifestyle, but the instance is requested outside the context of an active (Async Scoped) scope".

When I try to use the example from https://docs.simpleinjector.org/en/latest/disposabletransientcomponent.html as shown in my code below I get this error "This method can only be called within the context of an active (Scoped) scope".

I'm not sure whether it actually matters in a test whether I call RegisterForDisposal. I could just disable the warning for my one registration then things behave. I could not worry about that. But I'm not really sure and would appreciate some advice. Below I've included a sample that should help explain what I'm trying to do.

[TestClass]
public class TestDisposableForGithub
{
  private static Container _container;

  [ClassInitialize]
  public static void Initialize(TestContext context)
  {
    _container = SetupContainerPrivate();
  }

  private static Container SetupContainerPrivate()
  {
    var container = new Container();

    container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();
    // Transient, so we don't cache instances resolved off the container.  The thinking is
    // that 2 tests in the same class would be similar to 2 separate requests that would
    // have their own instances of classes.
    container.Options.DefaultLifestyle = Lifestyle.Transient; 

    container.RegisterDisposableTransient<IClassThatHasDispose, ClassThatHasDispose>();

    return container;
  }

  [TestMethod]
  public void TempTestItShouldResolveEseClient()
  {
    var instance = _container.GetInstance<IClassThatHasDispose>();
  }
}

public interface IClassThatHasDispose
{
  void TestMethod();
}

public class ClassThatHasDispose : IDisposable, IClassThatHasDispose
{
  public void TestMethod() { }
  public void Dispose() { }
}

public static class TestDisposableExtensions
{
  public static void RegisterDisposableTransient<TService, TImplementation>(
    this Container c)
    where TImplementation : class, IDisposable, TService
    where TService : class
  {
    var scoped = Lifestyle.Scoped;
    var r = Lifestyle.Transient.CreateRegistration<TImplementation>(c);
    r.SuppressDiagnosticWarning(DiagnosticType.DisposableTransientComponent, "ignore");
    c.AddRegistration(typeof(TService), r);
    c.RegisterInitializer<TImplementation>(o => scoped.RegisterForDisposal(c, o));
  }
}
Ergamon commented 1 year ago

I think you have to re-read what is written there.

The construction of objects is made by SimpleInjector. So your classes dont do "new" in a dependency injection scenario.

But what about disposable objects? Scoped instances are automatically disposed when the scope ends, Singleton instances are disposed when the container is disposed, but for Transient instances you as the coder have to decide when it is time for disposal, After all the idea is to free resources asap. But that leads to hard to maintain code. If your class has 2 dependencies of different lifestyles you have to treat them different. But the whole point of DI is that the using class should not have knowledge about the registration.

The code you found in the documentation, is a "hack" to at least have some kind of automatic disposal. It makes SimpleInjector dispose these objects at the end of a scope. You have to suppress a warning, cause the author of the library still thinks it is a bad idea.

Personally, if I really need a disposable transient, I do not register the class itself, but a factory. This way the user knows he has to create the instance himself and is therefore responsable to dispose it.

But back to your test, you used the "hack" to avoid Scope creation, but to make it work, you need a scope.

First I strongly advice against using a static container. By going this road, you will never be capable of running any tests in parallel.

It is not that annyoing as you think. You dont have to repeat the code in every test over and over. Just write it in the per/post code.

E.g. like this:

[TestClass]
public class TestDisposableForGithub : IDisposable
{
  private readonly Scope _scope;

  public TestDisposableForGithub()
  {
    var container = new Container();
    container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();
    container.Register<IClassThatHasDispose, ClassThatHasDispose>(Lifestyle.Scoped);
    _scope = AsyncScopedLifestyle.BeginScope(container);
  }

  [TestMethod]
  public void TempTestItShouldResolveEseClient()
  {
    var instance = _scope.GetInstance<IClassThatHasDispose>();
  }

  public void Dispose()
  {
    _scope.Dispose();
    _scope.Container!.Dispose();
  }
}