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

`IRenderedComponent.InvokeAsync` should support return values #1296

Closed Jcparkyn closed 10 months ago

Jcparkyn commented 11 months ago

Is the feature request related to a problem? Please elaborate.

I have a method I want to test that triggers a re-render via StateHasChanged, and also returns a value. Due to the re-render, I need to call this inside cut.InvokeAsync, which doesn't provide a way to access the return value.

The suggested solution

Add new overloads for InvokeAsync that return the value returned by workItem. E.g.:

public static Task<T> InvokeAsync<T>(this IRenderedFragmentBase renderedFragment, Func<T> workItem)
public static Task<T> InvokeAsync<T>(this IRenderedFragmentBase renderedFragment, Func<Task<T>> workItem)

Conveniently, these don't need any changes to the method body, since Dispatcher already has the necessary overloads.

This might cause some existing code to start depending on the new overloads, but as far as I can tell there wouldn't be any breaking changes due to this.

Describe any alternative solutions

None that I can think of.

Additional context

n/a

Jcparkyn commented 11 months ago

I can open a PR for this if it's wanted.

linkdotnet commented 11 months ago

Hey @Jcparkyn

I am not sure if I can follow entirely here. Especially the part about returning a value - it seems you want to invoke something on the component under test and want to retrieve the method return value. Do I grasp that correctly?

If not, can you create a small example and show where the current InvokeAsync isn't enough?

Jcparkyn commented 11 months ago

Yes exactly, I need to access the method return value so that I can assert on it, and in some cases also need it for the rest of the test (see below). Here's a small example:

The CallbackObject is a domain object that triggers a render for subscribed components, and also returns a value. The same use case would apply for methods on components directly, but they're probably less likely to be part of the public interface and less likely to have return values.

Helper class:

public class CallbackObject
{
    public event Action? SomethingChanged;

    public int DoSomething()
    {
        SomethingChanged?.Invoke();
        return 123;
    }
}

InvokeAsyncTestComponent.razor:

@implements IDisposable

@_count

@code {
    [Parameter]
    public CallbackObject CallbackObject { get; set; } = null!;
    private int _count = 0;

    private void CountUp()
    {
        _count++;
        StateHasChanged();
    }

    protected override void OnInitialized() => CallbackObject.SomethingChanged += CountUp;
    public void Dispose() => CallbackObject.SomethingChanged -= CountUp;
}

Test:

[Fact]
public async Task Test() // passes
{
    var obj = new CallbackObject();
    using var cut = RenderComponent<InvokeAsyncTestComponent>(
        ("CallbackObject", obj)
    );

    cut.MarkupMatches("0");

    // Can't do this because it triggers a render:
    // int doSomethingResult = obj.DoSomething();

    // Workaround using proposed extension method:
    int doSomethingResult = await cut.InvokeAsync(obj.DoSomething);

    // Another workaround I don't particularly like:
    // int doSomethingResult = -1;
    // await cut.InvokeAsync(() => doSomethingResult = obj.DoSomething());

    cut.MarkupMatches("1");
    Assert.Equal(123, doSomethingResult);
}

My workaround extension method:

static class RenderedFragmentExtensions
{
    /// Version of
    /// <see cref="RenderedFragmentInvokeAsyncExtensions.InvokeAsync(IRenderedFragmentBase, Action)"/>
    /// with a return value.
    public static Task<T> InvokeAsync<T>(this IRenderedFragmentBase renderedFragment, Func<T> workItem)
    {
        ArgumentNullException.ThrowIfNull(renderedFragment);

        return renderedFragment.Services.GetRequiredService<ITestRenderer>()
            .Dispatcher.InvokeAsync(workItem);
    }
}

My real use case is a bit more complicated, because the return value I want to access is actually a Task<T>, and I want to await this after InvokeAsync() returns. The new overloads still support this use-case though, by specifying the overload to use.

linkdotnet commented 11 months ago

Thanks for the clarification.

I am going back and forth in my mind whether or not I think it is a good idea to a) Have the overloads proposed by you and b) In general: Testing methods on your instance is a good idea.

My issue goes into the direction that testing the functions inside your component leads to exposing implementation details1 and doesn't test observable behavior. Now, that can be legitimate (component libraries, maybe?). The problem I see is that adding the function opens the door to overusing that pattern.

The way you solved it is nice and is "somewhat contained" in your code base - and maybe as last resort because testing the observable behavior isn't possible/has "high cost".

That is why I am leaning more towards not adding this function for a general audience. Maybe @egil has different thoughts here.

1 EDIT: Maybe some more explanation. While methods might be public (and maybe they shouldn't be), that doesn't mean they are public to your users. Obviously there are cases, like dialogs, where you have to surface the API to pen and close it - but that would be observable without knowing the specifics on how it is achieved.

egil commented 11 months ago

This is an interesting problem. On one hand, leading users to write better tests and being opinionated about it is a noble endeavor, and on the other hand .net libraries from Microsoft in general, including bUnit, tend to be unopinionated about specific styles/patterns.

I think I am leaning towards adding for completeness sake, since we already have the other InvokeAsync method on IRenderedFragment that already does expose this.

But I really like the idea of having a guiding principles section in our docs that showcase what is considered best practices for maintainable/valuable tests in bUnit, like https://testing-library.com/docs/guiding-principles.

linkdotnet commented 11 months ago

.net libraries from Microsoft in general, including bUnit, tend to be unopinionated about specific styles/patterns.

Interesting thought - I, personally, would argue .NET is very much opinionated. Even more so in recent years. Prominent examples might be EF Core, Minimal API's and the most recent example .NET Aspire . You could argue Blazor / ASP.NET Core itself too. There are many conventions, analyzers and what not pushing you towards a certain direction. It boils down to every very own definition of opinionated.

Jcparkyn commented 11 months ago

@linkdotnet In my case the method I'm testing (and its return value) are both parts of the public API for my library. The test accesses it via a component parameter for convenience, but the method is actually on an object which is exposed to the user (see example below). I know this is a little unconventional, but from my perspective there's not really any other way to write similar functionality in Blazor. I'd definitely be curious to hear an opposing view though.

Here's an example use-case for the API I'm trying to test, taken from my sample app. Note the call to context.LoadNextPage near the bottom. This also has an async version with a return value, which is what I need the new overloads for.

@inject HackerNewsApi Api

<UseEndpointInfinite Endpoint="Api.GetTopStories"
                     Arg="Arg"
                     GetNextPageArg="pages => (pages[^1].Arg?.GetNextPageArgs(), pages.Count < pages[^1].Data?.NbPages)">
    <ul class="post-list">
        @foreach (var query in context.Pages)
        {
            @if (query.HasData)
            {
                var posts = query.Data;
                foreach (var post in posts.Hits)
                {
                    <li>
                        <PostPreview @key="post.Id" Post="post" />
                    </li>
                }
            }
            else if (query.IsError)
            {
                <strong>
                    Something went wrong!
                    @query.Error?.Message
                    <button @onclick="query.Refetch">Retry</button>
                </strong>
            }
        }
    </ul>
    @if (context.HasNextPage || context.IsLoadingNextPage)
    {
        <div style="display: flex; justify-content: center; margin: 2em">
            <button @onclick="context.LoadNextPage" disabled="@context.IsLoadingNextPage" style="width: 10em; font-size: 1.3em;">
                @(context.IsLoadingNextPage
                    ? "Loading..."
                    : "Load More")
            </button>
        </div>
    }
</UseEndpointInfinite>

@code {
    [Parameter, EditorRequired]
    public HackerNewsApi.SearchStoriesArgs Arg { get; set; } = null!;
}

I think it's also quite common for libraries to have public API methods that result in component renders. These usually don't have return values, but I don't see any distinction that makes ones with return values "worse". Here are a couple of examples from libraries I've used:

linkdotnet commented 11 months ago

@Jcparkyn - convinced. For component libraries, the picture is different. That said - feel free to create a Pull Request that adds those two extensions.