Make WaitForNextRender method asynchronous #27

duracellko · 2020-01-05T07:13:06Z

Make WaitForNextRender asynchronous, instead of blocking until NextRender Task is completed. Blocking on task may lead to deadlocks.
Add support for asynchronous tests in Fixture so that WaitForNextRender can be called properly.

egil · 2020-01-05T07:36:25Z

Hey thanks for this @duracellko. It looks like good additions. I will take good look at this tomorrow and get back to you.

egil · 2020-01-06T12:36:48Z

Hi @duracellko, thanks again for this.

I have some feedback, that I hope you have time to incorporate into the PR. If not, I will do it later this week (hopefully).

I agree that Razor tests (Fixtures) should allow the user to create async test methods in general. However, if we can figure out a way to avoid adding the AsyncTest methods to Fixture it would be great. Maybe a type that can be implicitly converted to from both Action and Func<Task>?
I am no expert in async/await, but it seems to me that WaitForNextRender might not be an issue to keep it blocking. Each unit test context gets its own TestRenderer, and all tests within a class/razor file are run synchronously (though multiple test classes can be run in parallel by the user), and it that we are also waiting in the DispatchAndAssertNoSynchronousErrors method.

Can you perhaps provide an example where a deadlock occurs, perhaps as a unit test in the tests project (not sample project)?
The WaitForNextRender should have an overload that allows an Func to be passed to it, which is also awaited.
I would love some focused unit tests around the WaitForNextRender functionality in the test project. Are you up for writing those? I am thinking:
- A test that explores what happens when if the renderTrigger action throws.
- A test for when timeout occurs
- A test for when things goes as planned
Can you change your merge request to point to the dev branch instead. It is updated and in sync with master. Then the dev branch will be where we collect changes for the next release.

Thanks again, let me know what you think.

duracellko · 2020-01-07T01:50:52Z

Hello, thank you for your feedback.

I tried following options:
1. Option implemented in PR. I agree it's not really nice, but it follows pattern in ComponentBase class like OnInitialized and OnInitializedAsync.
2. Setup and Test expects Func<Task>. Then user can set property Test="new Action(MyTest).AsAsync()", where as AsAsync would be simple extension method. However, it's not possible to do in C# something like Test="MyTest.AsAsync()". Another disadvantage is that test method is wrapped, and test failure would report wrong method name.
3. Setup and Test expects Func<Task>. Then user can write synchronous test:

Task MyTest()
{
    // do test
    return Task.CompletedTask;
}

I can imagine there may be a problem with something like testing progress indicator. In such case mocking business logic task has to be completed manually in test. And test may be blocked, because it's waiting for render, but render is not triggered until business logic task is finished, but it is not finished, because test mock didn't reach that point. I will create some tests for it.
I will think about it. However, I don't think asynchronous trigger is really needed. Usually trigger is UI action like button click, dropdown value change, and these operations are synchronous. They can trigger asynchronous operations, but never wait for the output. That's what WaitForNextRender does, it waits for the output.
Yes, I will check and improve test coverage of WaitForNextRender.
Will do.

egil · 2020-01-07T07:19:46Z

Awesome. Appreciate it.

If we cannot find a better solution for single test methods on Fixture, then let's at least rename them to follow the ComponentBase convention with "async" last in the name, i.e. TestAsync instead AsyncTest.

For triggering UI element the recommended way is though the dispatch event helper methods, e.g. cut.Find("button").Click(). All the event bindings that Blazor supports have event dispatch helpers in
https://github.com/egil/razor-components-testing-library/tree/master/src/EventDispatchExtensions

The WaitForNextRender is more for e.g. when a component is waiting for an async service to complete before it will render again. That service is usually something we inject during testing and have control over, e.g. the MockJsRuntime, and can set the result of inside the renderHandler passed to WaitForNextRender.

That said, if we cannot find a compelling usecase for adding an overload of WaitForNextRender that takes a Func<Task> it if better not to and apply the YAGNI principle.

Another thing. Naming is hard. I am not really a fan of WaitForNextRender. It is not clear from the name how to use it. If we go with the async version, I am open for naming it e.g. NextRenderAfter, which would lead to more clean code in tests, e.g. await NextRenderAfter(x=>some magic);
If we do not go with the async version I think the name AwaitNextRenderAfter(...) is also better. What do you think?

egil

See previous comment for suggested changes.

egil · 2020-01-07T14:29:32Z

FYI: I have added an issue that is somewhat related (#29). However, I do not think it should be included in this PR. Lets focus on figuring this out first, then we/I can take a look at that issue.

duracellko · 2020-01-07T15:44:21Z

I created TestContextTest to test WaitForNextRender.

src/Components/Fixture.cs

egil · 2020-01-07T20:50:10Z

tests/TestContextTest.cs

+
+namespace Egil.RazorComponents.Testing
+{
+    public class TestContextTest


Suggested change

public class TestContextTest

public class TestContextTest : ComponentTestFixture

By inherting from ComponentTestFixture, we get a TextContext implicitly, follow the convention from the other tests that also does this, and do not have to deal with disposing of the context after each test. It is handled by the fixture.

Actually I don't like this.

I don't think the test framework should be used to test itself. Or at least tests of TestContext should not depend on ComponentTestFixture that depends on TestContext.

Inheriting ComponentTestFixture from TestContext can lead to flaky tests (or tests depending on order). For example in one of my tests I setup Services. And this setup of services is then carried on to other tests. But it depends on order of execution to which tests.

In general test classes should be stateless and new state should be constructed in each test. So ComponentTestFixture is anti-pattern.

Order of tests does not matter at all since xUnit creates a new instance of the TestContextTest for each test/fact it executes, and when the test is done, it runs the dispose method inherited by TestContextTest.

That means every test has it's own test context.

As to the first point, I get what you are saying. But since ComponentTestFixture just provides helper methods for tests, and it is a test context, I do think it makes sense to do it like this.

Oh, yes. I forgot that xUnit works differently than NUnit and MSTest. I will update the pattern then.

egil

First of, this is very solid code. Thank you for the effort.

My comments and suggestions for changes are more directed at code style and my wish to keep things consistent in the library.

A question. Were you able to find an example of a combination of calls that actually causes a deadlock if WaitForNextRender is not async? If so I would very much like to see it. I am trying to understand the nature of this problem, and if this problem also exists with the TestRenderer which basically blocks everytime it renders in its DispatchAndAssertNoSynchronousErrors method (has a call to .Wait()).

Another thought: If we change WaitForNextRender to the async form you are proposing, I am tempted to rename it to NextRenderAfter(Action? renderTrigger, TimeSpan? timeout). It makes it more clear to the user that what they are awaiting is the next render after whatever action has completed they provide is invoked. What do you think?

egil · 2020-01-07T20:52:51Z

tests/TestContextTest.cs

+{
+    public class TestContextTest
+    {
+        [Fact(DisplayName = "Constructor setup default values")]


I am not sure this test has any benefit. What it verifies is tested implicitly by all the other tests. As such I think it can be safely deleted without loosing test coverage.

egil · 2020-01-07T20:54:03Z

tests/TestContextTest.cs

+        [Fact(DisplayName = "Wait for first render")]
+        public async Task Test002()
+        {
+            using (var target = CreateTestContext())


Suggested change

using (var target = CreateTestContext())

Now that the test classes is a Test Context through inheritance, this is not needed

egil · 2020-01-07T20:54:20Z

tests/TestContextTest.cs

+        public async Task Test002()
+        {
+            using (var target = CreateTestContext())
+            {


Suggested change

{

egil · 2020-01-07T20:54:34Z

tests/TestContextTest.cs

+        {
+            using (var target = CreateTestContext())
+            {
+                var renderTask = target.WaitForNextRender(NoOperation);


Suggested change

var renderTask = target.WaitForNextRender(NoOperation);

var renderTask = WaitForNextRender(NoOperation);

egil · 2020-01-07T20:54:48Z

tests/TestContextTest.cs

+                var renderTask = target.WaitForNextRender(NoOperation);
+                renderTask.IsCompleted.ShouldBeFalse();
+
+                target.RenderComponent<Simple1>();


Suggested change

target.RenderComponent<Simple1>();

RenderComponent<Simple1>();

egil · 2020-01-07T21:11:51Z

tests/TestContextTest.cs

+                var button = cut.Find("button");
+                var renderTask = target.WaitForNextRender(() => button.Click());
+
+                // Render is done on button click
+                asyncTestDepMock.Verify(o => o.GetData());
+                renderTask.IsCompleted.ShouldBeTrue();
+                await renderTask;


This part of the test is not entirely clear to me. button.Click() is synchronous and blocks until the render is done, so is this part relevant?

Could this be replaced by

Suggested change

var button = cut.Find("button");

var renderTask = target.WaitForNextRender(() => button.Click());

// Render is done on button click

asyncTestDepMock.Verify(o => o.GetData());

renderTask.IsCompleted.ShouldBeTrue();

await renderTask;

cut.Find("button").Click();

egil · 2020-01-07T21:12:34Z

tests/TestContextTest.cs

+            }
+        }
+
+        [Fact(DisplayName = "No render when asynchronous event fails")]


This test should be updated to remove the using(var target and related code, as the previous test.

egil · 2020-01-07T21:12:53Z

tests/TestContextTest.cs

+                var button = cut.Find("button");
+                var renderTask = target.WaitForNextRender(() => button.Click());
+
+                // Render is done on button click
+                asyncTestDepMock.Verify(o => o.GetData());
+                renderTask.IsCompleted.ShouldBeTrue();
+                await renderTask;


Same comment as with the previous test.

egil · 2020-01-07T21:13:07Z

tests/TestContextTest.cs

+        private static TestContext CreateTestContext()
+        {
+            return new TestContext();
+        }


Can be deleted.

egil · 2020-01-07T21:15:21Z

tests/TestContextTest.cs

+        private static void NoOperation()
+        {
+        }


The fact that you need a method like this in this particular test scenario makes me think that maybe the the signature for the WaitForNextRender should be WaitForNextRender(Action? renderTrigger, TimeSpan? timeout) instead, i.e. making renderTrigger optional. If none is passed in, we still do the heavy lifting of setting up a task that times out after a specific TimeSpan or a render completes.

What do you think?

I agree that renderTrigger could be optional. Now the question is, what is preference?

Method overload:

public Task WaitForNextRender(TimeSpan? timeout = null) => WaitForNextRender(null, timeout); public async Task WaitForNextRender(Action? renderTrigger, TimeSpan? timeout = null) { ... }

Default parameter:

public async Task WaitForNextRender(Action? renderTrigger = null, TimeSpan? timeout = null) { ... }

I prefer the default parameter variant. In the end the compiler turns it into the same, so why waste keystrokes :-)

I will update it.

FYI: It's not compiled exactly the same.

Overload is compiled in provider DLL. For example there are libraries A.dll references B.dll. B.dll provides methods void M(string val); void M() => M(null); And A.dll has code M();. Then later B.dll is updated to version 2 with M(string val); M() => M(string.Empty);. When B.dll is deployed, then result is that M(string.Empty) is executed without recompiling A.dll. New default is used without recompiling DLL.

Default parameter is compiled into client DLL. For example there are libraries A.dll references B.dll. B.dll provides methods void M(string val = null); And A.dll has code M();. Then later B.dll is updated to version 2 with M(string val = string.Empty);. When B.dll is deployed, then result is that M(null) is executed without recompiling A.dll. New default is used only after recompiling DLL.

Same difference is between public readonly string Value = "V" and public const string Value = "V".

Thanks for the explanation. Didn't know that.

duracellko · 2020-01-08T03:11:20Z

A question. Were you able to find an example of a combination of calls that actually causes a deadlock if WaitForNextRender is not async? If so I would very much like to see it. I am trying to understand the nature of this problem, and if this problem also exists with the TestRenderer which basically blocks everytime it renders in its DispatchAndAssertNoSynchronousErrors method (has a call to .Wait()).

I added test "WaitForNextRender is not blocked in Dispatcher context". It ensures that it is not blocked. Then I updated test in this branch https://github.com/duracellko/razor-components-testing-library/blob/WaitForNextRender-blocked/tests/TestContextTest.cs. And this test gets blocked.

I didn't realize that tests do not run in the same synchronization context as the Dispatcher by default. So deadlock happens, only when test explicitly runs on Dispatcher.

Also in general it's better to have non-blocking task. As you can see in other tests, it's easier to test, that the task is not finished.

duracellko · 2020-01-08T06:18:55Z

... if this problem also exists with the TestRenderer which basically blocks everytime it renders in its DispatchAndAssertNoSynchronousErrors method (has a call to .Wait()).

Yes, I noticed .Wait() in DispatchAndAssertNoSynchronousErrors yesterday. I believe it would be good if it's also returning task, instead of blocking. Also bigger question is, if it makes sense that whole test runs in context of Dispatcher, instead of xUnit context (or another synchronization context). Shall I create new issue to start the discussion about it?

Another thought: If we change WaitForNextRender to the async form you are proposing, I am tempted to rename it to NextRenderAfter(Action? renderTrigger, TimeSpan? timeout). It makes it more clear to the user that what they are awaiting is the next render after whatever action has completed they provide is invoked. What do you think?

I think it looks good. I will thing about the name.

egil · 2020-01-08T07:18:02Z

Just a quick comment regarding the general use of blocking logic (I'll be back with more later). I do want to keep the api clean for the users. And if they have to write async in front of every other statement in their test, it wouldn't be a great experience. So as long as we are NOT causing deadlocks, I am fine with breaking best practices in a few places.

The code related to the dispatcher and WaitForNextRender is actually taken more or less verbatim from Steve Sanderson prototype (https://github.com/SteveSandersonMS/BlazorUnitTestingPrototype).

Since each test runs in its own xunit sync contexts, and each test also has it's own renderer with it's dispatcher with sync context, we might not have deadlock issues at all.

egil · 2020-01-08T13:11:28Z

OK, so I've been reading up on Synchronization Contexts and have a better understanding now, thanks for the push to do so :-)

My understanding is that the Dispatchers context is where the TestRenderer executes components life-cycle methods and generally does its work, and if there is a Wait in that context, it does not affect the context that tests are running in (a Xunit.Sdk.AsyncTestSyncContext).
The current blocking implementation of WaitForNextRender will only block until the timeout is reached. The timeout prevents deadlocks in this case, unless users set the timeout to -1 millisecond obviously.
I do agree that for the purpose of the TestContextTest test, it is easier to verify that the nextRender task has not finished. However, the user of the library is unlikely to want to verify that. They just want to do something, such as setting the result of a mock service a component under test awaits, and then wait for the component to rerender with those values, so they can verify that the values the service has provided to the component was used correctly.

In conclusion, I really appreciate your efforts with this, but I think we should take a step back and reevaluate the changes to WaitForNextRender before continuing.
Your PR has made me rethink a bunch of things about it, such as its name, and whether or not it should be associated with a IRenderedComponent instead of the test context. So before we make a change to the API, let's make sure it's for the better.

I do think it is useful to have SetupAsync, TestAsync, and TestsAsync on Fixture, so that part we can go forward with.

Ps. this modified version of your deadlocking tests now just throws a TimeoutException:

[Fact(DisplayName = "WaitForNextRender is not blocked in Dispatcher context")]
public async Task Test008()
{
    using (var waitHandle = new ManualResetEvent(false))
    {
        string GetTestData()
        {
            var result = waitHandle.WaitOne(TimeSpan.FromSeconds(1));
            Thread.Sleep(100);
            return result ? "Test" : string.Empty;
        }

        var asyncTestDepMock = new Mock<IAsyncTestDep>();
        asyncTestDepMock.Setup(o => o.GetData()).Returns(Task.Run(GetTestData));
        Services.AddService<IAsyncTestDep>(asyncTestDepMock.Object);

        var cut = RenderComponent<SimpleWithAyncDeps>();

        await Renderer.Dispatcher.InvokeAsync(() =>
        {
            WaitForNextRender(() => waitHandle.Set());
        });

        cut.Find("p").TextContent.ShouldBe("Test");
    }
}

egil · 2020-01-08T13:13:58Z

Shall I create new issue to start the discussion about it?

Yes. Please do. Let's have a look at the async/sync code and see where we can make changes to benefit the users and the performance of the library.

duracellko · 2020-01-08T13:23:36Z

Ok, I will rewrite the branch just with changes on Fixture. And I will create issue to continue discussion about async part.

egil · 2020-01-08T13:38:54Z

Ok, I will rewrite the branch just with changes on Fixture. And I will create issue to continue discussion about async part.

Awesome. Appreciate it!

Co-Authored-By: Egil Hansen <[email protected]>

duracellko · 2020-01-11T05:15:56Z

Sorry, for late response. Last few days were busy.

I updated the branch and created issue #31.

egil · 2020-01-13T08:56:01Z

Sorry, for late response. Last few days were busy.

No worries at all. This is not the primary occupation for none of us.

I will review and merge on Wednesday. Busy preparing for .net conf tomorrow.

* Fixed spelling mistake in comment * Reordered parameters to top of TestRenderer * Add support for asynchronous Razor tests (#27) * Add support for asyncrhonous Razor tests * Rename Fixture.AsyncTest to TestAsync * Update description of Fixture Co-Authored-By: Egil Hansen <[email protected]> * Dotnetconf samples (#33) * Basic example tests * Add missing MarkupMatches overload * Added docs to MockHttp extensions * dotnet conf samples * Added unit tests of CompareTo, Generic and Collection assert extensions. * Added tests for general events and touch events dispatch extensions * Added tests for anglesharp extensions, JsRuntimeInvocation and ComponentParameter * Removed assert helpers that conflict with Shoudly * Tests of JsRuntimeAsserts * Reorganized test library * Suppressing warnings in sample * Changed ITestContext to have a CreateNodes method instead of HtmlParser property * Removed empty test * Added missing code documentation * Moved MockJsRuntime to its own namespace * Pulled sample from main solution into own solution * Update main.yml * Change GetNodes and GetMarkup to Nodes and Markup properties in IRenderedFragment (#34) * Add SetupVoid and SetVoidResult capabilities to JsRuntime mock (#35) * Moved MockJsRuntime to its own namespace * Add SetupVoid() and SetVoidResult() to PlannedInvocation and Mock * Updates to template and sample * Add default JsRuntime (#32) * Add default JsRuntime * Update src/Mocking/JSInterop/DefaultJsRuntime.cs Co-Authored-By: Egil Hansen <[email protected]> * Response to review. Add custom exception and code cleanup * Remove unneded method * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <[email protected]> Co-authored-by: Egil Hansen <[email protected]> * Add .vscode to gitignore * TestServiceProvider now explictly implements IServiceCOlelction (#40) * TestServiceProvider now explictly implements IServiceCOlelction * Tweaks to namespaces * Added async setup method to snapshot test * Added test of SnapshotTests use of setup methods * Update to readme * Removed duplicated MarkupMatches method * Removed PR trigger Co-authored-by: Rastislav Novotný <[email protected]> Co-authored-by: Michael J Conrad <[email protected]>

egil self-requested a review January 6, 2020 12:38

egil added the enhancement New feature or request label Jan 6, 2020

duracellko changed the base branch from master to dev January 7, 2020 01:52

egil requested changes Jan 7, 2020

View reviewed changes

egil self-requested a review January 7, 2020 20:37

egil reviewed Jan 7, 2020

View reviewed changes

src/Components/Fixture.cs Outdated Show resolved Hide resolved

egil reviewed Jan 7, 2020

View reviewed changes

egil requested changes Jan 7, 2020

View reviewed changes

duracellko and others added 3 commits January 11, 2020 12:06

Add support for asyncrhonous Razor tests

d73b90d

Rename Fixture.AsyncTest to TestAsync

cc85baf

Update description of Fixture

838b0ee

Co-Authored-By: Egil Hansen <[email protected]>

duracellko mentioned this pull request Jan 11, 2020

Waiting operations and multi-threading #31

Closed

egil approved these changes Jan 15, 2020

View reviewed changes

egil merged commit 457409c into bUnit-dev:dev Jan 15, 2020

	public class TestContextTest
	public class TestContextTest : ComponentTestFixture

	var renderTask = target.WaitForNextRender(NoOperation);
	var renderTask = WaitForNextRender(NoOperation);

	target.RenderComponent<Simple1>();
	RenderComponent<Simple1>();

Make WaitForNextRender method asynchronous #27

Make WaitForNextRender method asynchronous #27

Conversation

duracellko commented Jan 5, 2020

egil commented Jan 5, 2020

egil commented Jan 6, 2020 • edited Loading

duracellko commented Jan 7, 2020

egil commented Jan 7, 2020 • edited Loading

egil left a comment

Choose a reason for hiding this comment

egil commented Jan 7, 2020

duracellko commented Jan 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duracellko commented Jan 8, 2020

duracellko commented Jan 8, 2020

egil commented Jan 8, 2020 • edited Loading

egil commented Jan 8, 2020

egil commented Jan 8, 2020

duracellko commented Jan 8, 2020

egil commented Jan 8, 2020

duracellko commented Jan 11, 2020

egil commented Jan 13, 2020

egil commented Jan 6, 2020 •

edited

Loading

egil commented Jan 7, 2020 •

edited

Loading

egil commented Jan 8, 2020 •

edited

Loading