bUnit-dev / bUnit

bUnit is a testing library for Blazor components that make tests look, feel, and runs like regular unit tests. bUnit makes it easy to render and control a component under test’s life-cycle, pass parameter and inject services into it, trigger event handlers, and verify the rendered markup from the component using a built-in semantic HTML comparer.
https://bunit.dev
MIT License
1.14k stars 105 forks source link

Fine grained disposing of components #1068

Closed linkdotnet closed 1 year ago

linkdotnet commented 1 year ago

Current state

In our current state, the TestContext and ITestRenderer offers a DisposeComponents method that basically removes all components from the RenderTree so they get disposed of. That has three flaws:

  1. It is an all-or-nothing approach. There is no way of controlling which component should be removed.
  2. The API is rather ugly as we expose DisposeComponents to too many hierarchies (and objects).
  3. If we decide to go with a Renderer instance per Render call our DisposeComponents wouldn't work anymore.

Fine-grained disposing

With v2 we have the ability to correct such things. The Renderer (from Blazor itself) offers a RemoveRootComponent that allows us the remove specific elements from the tree.

API

The API would sit on top of the IRenderFragment type and some API inside the TestRenderer:

TestRenderer.cs

internal void RemoveComponent(IRenderedFragment fragment)
            => RemoveRootComponent(fragment.ComponentId);

Extensions for IRenderFragment:

public static void DisposeComponent(this IRenderedFragment fragment)
{
    ArgumentNullException.ThrowIfNull(fragment);

    var renderer = fragment.Services.GetRequiredService<TestRenderer>();
    renderer.RemoveComponent(fragment.ComponentId);
}

Usage

The usage would be fairly trivial:

var cut = RenderComponent<Component>();

cut.DisposeComponent();

Alternative Design

Alternatively, we already have a IDisposable implementation inside IRenderFragment. That said, we could leverage this:

internal class RenderedFragment : IRenderedFragment
{
  public void Dispose()
  {
    // Here normal dispose logic 
    testRenderer.RemoveComponent(ComponentId);
  }
}

That would feel more natural to the language itself. It seems odd to have two dedicated functions taking care of disposing (from an outside point of view).

The inherent issue with this approach is that the markup is set to empty - anyway, there should be no check on the markup if your component gets disposed of.

As the IRenderFragment already has IDisposable that could lead to side effects for users that already do things like:

using var cut = RenderComponent<...>();

Seems to be some usage like this: https://grep.app/search?q=using%20var.%2ARenderComponent&regexp=true&case=true&filter[lang][0]=C%23.

What are your thoughts @egil ?

Edit: I am leaning towards the second approach of leveraging the already existing Dispose method.

linkdotnet commented 1 year ago

Hmm, I spiked the idea, and it seems that the RemoveRootComponent is a dealbreaker for us mainly because you can only remove components with no parent making it virtually useless for our case.

protected internal void RemoveRootComponent(int componentId)
{
    Dispatcher.AssertAccess();

    // Asserts it's a root component
    _ = GetRequiredRootComponentState(componentId);
...

private ComponentState GetRequiredRootComponentState(int componentId)
{
    var componentState = GetRequiredComponentState(componentId);
    if (componentState.ParentComponentState is not null)
    {
        throw new InvalidOperationException("The specified component is not a root component");
    }

    return componentState;
}

As all of our components under test have a ParentComponentState we will always be kicked out in this line of code.

egil commented 1 year ago

A few loose thoughts, I will have to think more about them before committing to anything.

  1. What is the purpose for a bUnit user to want to dispose/remove a component from the render tree? I suspect it is to test a component's dispose logic. If that is the scenario, I think in many cases that just being able to remove the root component is going to work out.

  2. Ideally I prefer not to have to store the renderer in Services, especially if we have one renderer per "root render". I would rather pass that to rendered component types created by the renderer.

linkdotnet commented 1 year ago

A few loose thoughts, I will have to think more about them before committing to anything.

  1. What is the purpose for a bUnit user to want to dispose/remove a component from the render tree? I suspect it is to test a component's dispose logic. If that is the scenario, I think in many cases that just being able to remove the root component is going to work out.
  2. Ideally I prefer not to have to store the renderer in Services, especially if we have one renderer per "root render". I would rather pass that to rendered component types created by the renderer.

Fair points - let's close this ticket and revisit if we need something once we have the one Renderer per Render-Call