bUnit icon indicating copy to clipboard operation
bUnit copied to clipboard

Shared dispatcher/synchronization context with Renderer

Open egil opened this issue 3 years ago • 5 comments

Motivation:

Tests should be deterministic, and one of the biggest challenges with that is that the Renderer can asynchronous (re)render components while test code is being executed.

This can lead to subtle bugs, e.g. where a cut.Find("button").Click() results in the button being found and having a event handler attached when Find is executing, but when Click starts running, that event handler has changed. There are a lot of work arounds and safe guards in bUnit currently to make this as unlikely to happen, but there are probably still edge cases where users would have to wrap their test code in a cut.InvokeAsync(() => ...) call to ensure it runs without the renderer doing renders.

There are still some things we need to figure out:

  • [ ] How to control and set the sync context at test start
  • [ ] How to make the "wait for" APIs async (probably needed)
  • [ ] How to minimize the API surface changes
  • [ ] Will this affect event triggering?
  • [ ] A generic solution that works with all testing frameworks (xUnit, NUnit, MSTest)
  • [ ] How to lead the users to not deadlocking themselves, e.g. if they have a component that is waiting for data and a test that is waiting for a change.

egil avatar May 26 '22 18:05 egil

Not sure if this is related to #687 or not, but I do believe the tests should wait for the render to complete.

My ideal scenario would be to not have the "wait for" APIs at all, but instead, make each call only return once the handlers/rendering are complete. e.g.,

var cut = await RenderComponentAsync<CounterComponentDynamic>();

// don't get to here until the render is complete

await cut.Find("[data-id=1]")).ClickAsync();

// don't get to here until the click event and subsequent render is complete

My understanding is that I can already call .ClickAsync() ... it's just the RenderComponentAsync<T>() that's missing. Is that the intention of this issue?

FlukeFan avatar Jun 06 '22 15:06 FlukeFan

... you might want to write a test ...

My thinking is that that should be the exception, not the rule. (i.e., what you have now). Not allowing the async-style testing forces the tests to handle scenarios that they are not interested in. (i.e., forcing the test author to figure out when to use "wait for" APIs, and when they need to use InvokeAsync().)

Note, that I would imagine that even though the test doesn't continue until the render (the second render in your example) completes, the two renders still occur. (Catching, for example, exceptions caused by the 'loading...' not working.)

FlukeFan avatar Jun 06 '22 15:06 FlukeFan

@FlukeFan thanks for the input. The problem is that this will not work in all scenarios.

Suppose you have a component that renders a "loading" text while it waits for an async service to return data in OnInitializeAsync.

In that case, you might want to write a test that verifies that the loading text is displayed before the async service returns. So you pass a mock of the service to the component under test that returns an incomplete task, and that allows you to deterministically test that the loading content is shown correctly.

However, if bUnit were to block until OnInitializeAsync completes, the test is essentially deadlocked.

Another problem is that Blazor does not return an incomplete task if there is async code on OnAfterRenderAsync, it just allows the task to continue to run in the background. So a RenderComponentAsync will not work consistently, and I want to avoid APIs like that.

egil avatar Jun 06 '22 15:06 egil

Not allowing the async-style testing forces the tests to handle scenarios that they are not interested in. (i.e., forcing the test author to figure out when to use "wait for" APIs, and when they need to use InvokeAsync().)

bUnit uses Blazor's renderer under the hood, and have to play by its rules. That means that any time an async method is hit during a render of a component, the render is reported as "completed" to the caller, and then the continuation of that async method is scheduled for a later render. So RenderComponentAsync just wont make a difference unfortunately.

It maybe that the sync event handler trigger methods gets deprecated though, such that users will have to call e.g. ClickAsync() when they want to invoke a click event handler, and then the event returned should match when the event handler method completes.

We can avoid having to call InvokeAsync completely if the test code runs in the same sync context as the renderer. When you call InvokeAsync you are essentially switching over to it. Having a shared sync context also means that no render can happen while test code is executing, which avoids race conditions between test code and code under test.

The rule for when you would need to use the WaitFor methods, which would become async methods returning tasks, would be whenever you have something in your components that does not complete synchronously, e.g. if you have at Task.Delay in there. The problem now, which you and others have run into, is that it has not always worked as expected, due to some of the race conditions that have hopefully been mostly fixed now.

egil avatar Jun 07 '22 11:06 egil

@FlukeFan Turns out I was not correct. At least in .NET 6 and later, it is possible to get a task back from the Blazor renderer that will not complete before there are no unfinished tasks in the render pipeline. Feel free to jump in on the discussion in #757.

egil avatar Jun 16 '22 17:06 egil

Ill close this as we already merged in a change where we use the users sync context to update rendered fragments after each render.

egil avatar May 08 '23 15:05 egil