Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shared dispatcher/synchronization context with Renderer #739

Closed
6 tasks
egil opened this issue May 26, 2022 · 6 comments
Closed
6 tasks

Shared dispatcher/synchronization context with Renderer #739

egil opened this issue May 26, 2022 · 6 comments
Labels
help wanted Extra attention is needed investigate This issue require further investigation before closing.
Milestone

Comments

@egil
Copy link
Member

egil commented May 26, 2022

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 egil added help wanted Extra attention is needed investigate This issue require further investigation before closing. labels May 26, 2022
@egil egil added this to the 2.0.0 milestone May 26, 2022
@FlukeFan
Copy link

FlukeFan commented Jun 6, 2022

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
Copy link

FlukeFan commented Jun 6, 2022

... 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.)

@egil
Copy link
Member Author

egil commented Jun 6, 2022

@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
Copy link
Member Author

egil commented Jun 7, 2022

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
Copy link
Member Author

egil commented Jun 16, 2022

@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
Copy link
Member Author

egil commented May 8, 2023

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 egil closed this as completed May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed investigate This issue require further investigation before closing.
Projects
None yet
Development

No branches or pull requests

2 participants