-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
Multi-threaded test execution appears to cause a race condition when using SetParametersAndRender #1188
Comments
Hey @Skintkingle, could this be fixed by using one of the WaitForX methods, that exists exactly for the purpose of helping with components that do async work during their life cycles? See more here: https://bunit.dev/docs/verification/async-assertion.html If I understand your case correctly, the test would look like this instead: [Fact(DisplayName = "SetParametersAndRender from non-UI thread waits until render completion before returning")]
public async Task Test209()
{
var cut = RenderComponent<SlowComponent>();
cut.RenderCount.ShouldBe(1);
cut.SetParametersAndRender();
cut.WaitForAssertion(() => cut.RenderCount.ShouldBe(2));
} |
Hi @egil Thanks for lending a hand. The components themselves aren't doing anything async, so the tests shouldn't need any async-y waiting. The issue is (I think) to do with the executing environment of the test. When run in parallel (in NUnit), the SynchronizationContext the Dispatcher starts with must be different from the one the test is executed with, so the test goes async when it hits the Dispatcher.InvokeAsync in the SetDirectParameters call. The example test I was playing with is doing Task-y stuff purely to try and replicate a test executing in parallel. I will have a little more of a play today, read up on XUnit and work out how to run tests in parallel. I think Collections? I'll give it a go and see if I can get across what I'm seeing more clearly. I injected some On reflection, it might be an NUnit thing. I'll try a bit today to replicate it using XUnit in the bunit repo. If that fails I'll make a really simple test using NUnit as the test framework, and see what happens. |
The inner workings of the Blazor renderer can be surprising. The runtime will actively attempt not to execute code asynchronously if it can, so when Now, to my knowledge NUnit doesn't set up a synchronization context itself as xUnit does, so there may be something related to that we need to investigate. |
Just to add my two cents: Also to my knowledge NUnit does not have a Just for the slight off-chance: Does your component in question override |
Thanks for the thoughts :). These are similar thoughts I've been having. Especially when looking at the AspNetCore Dispatcher code and seeing its async decision making is based on the SynchronizationContext instance. In our real components there's a couple that seem to exibit the issue more regularly than the others. one of them overrides Otherwise I can't seem to reproduce the issue using XUnit. I've made 2 classes with similar tests in them for simplicity sake, and run them in parallel. It all seems fine so far. I'll try overriding OnParametersSetAsync and see if different decisions are made by the renderer |
I think you might be on to something with the async overrides. And I seem to be able to reproduce it in NUnit now (And curiously, even when NUnit isn't set to run in parallel, too!) but still can't get XUnit to fail reliably. If I run any 1 of the tests with "Run until failure", it never seems to fail. If I select 2 tests, and "Run until failure". Even though I haven't yet turned on parallelisation in NUnit, the 2nd one fails, after only 1 or 2 runs. The XUnit tests sometimes fail and sometimes don't and I really don't understand what causes them to fail or not. I've tried a I have a branch I can push if either of you are interested in seeing it. My test component looks like this:
And each of my tests look almost exactly the same, like this:
NUnit and XUnit tests differ only in the logging. NUnit uses the Consoles output, XUnit I passed it the ITestOutputHelper, otherwise the components are identical. |
Can you share the complete test classes? |
Sure thing. I've made a
The XUnit test classes look like this (again, only providing one because they are basically copy pastes with one saying 1, and the other saying 2. :))
And finally
|
I have actually been thinking if it was possible for us to use source generators to automatically create a copy of all our tests for both NUnit, XUnit, and MSTest. It does require us to only use features available in all frameworks, but should be doable. That way we can verify bUnit with all major test frameworks. |
A general note: there is no shared state/statics between TestContext and other bUnit types, so if there are issues with running things in parallel, the issues are showing up due to the sharing of CPU time/memory/system resources. |
I think the Parallel aspect of my original statement is not quite the whole picture. Parallel execution seems to be needed with XUnit to make the failure happen. But just running the NUnit tests in series causes it to fail. (I never even got to configuring parallelisation in NUnit, and started seeing the issue). |
Sorry, I am trying to understand what the issue is. Is the issue that you are not seeing the expected RenderCount or that there is something in the rendered markup that is not correct? The change in PR you reference in the issue does change the behaviour slightly by allowing the Blazor renderer to batch together multiple synchronous renders into one, just as it happens in production when parameters are updated on a child component. The motivation from our side was to make sure we have like the production runtime, and I do see that it could cause the expected |
The render count I was using in these tests just as a contrived example of inconsistent results. Our tests were actually using assertions that Events were un/subscribed. The test error said it wasn't there, Followed by the output clearly showing that it was there led me to the initial investigation driven by the fact something odd is going on and it felt very racey. (Moq checked the event subscriptions, and it wasn't there. and when it came to logging out all the event subscriptions to help understand what happened, it had actually registered, so it was in that subsequent output that facilitates the failed Assert). It also happened when we upgraded BUnit from 1.20.8 to 1.22.19 so I went to investigate what changed in BUnit between those 2 versions that could have started causing a race condition in our test execution. Any other information from then on has been from generally investigating how bunit operates when tests run in parallel because that's where I thought the issue stems from. All that being said, Unfortunately, I'm not being given masses of time to play with this anymore, even though I want to really understand what's going on here. As the workaround we came up with (Do an |
Interesting. We will have a look. Can you confirm that you have shared the test cases with us that causes the issue when running in parallel? |
Yessir! (Sorry for the late response). The tests provided above were causing the issue in the bunit repo. NUnit/XUnit manifest it in different ways, but both manifest it! |
Hello @egil We have encountered the same issue with the While investigating the issue, we noticed that in the As a test we have adapted the code of bUnit so that the task is awaited in the I hope this helps. Please don't hesitate to ask for further clarifications. Hopefully this fix gets released soon. |
Hey all, Sorry for the radio silence. I have been travelling last week so there has been no time to follow up on this. @Schaeri, thanks for the patch, however, we probably have to resort to calling We missed this behavior change in the release. Originally, we were awaiting the InvokeAsync task like so: var result = renderedComponent.InvokeAsync(() =>
renderedComponent.Instance.SetParametersAsync(parameters));
if (result.IsFaulted && result.Exception is not null)
{
if (result.Exception.InnerExceptions.Count == 1)
{
ExceptionDispatchInfo.Capture(result.Exception.InnerExceptions[0]).Throw();
}
else
{
ExceptionDispatchInfo.Capture(result.Exception).Throw();
}
}
else if (!result.IsCompleted)
{
result.GetAwaiter().GetResult();
} I will adjust our new implementation to match this behavior and that should hopefully fix the issues you are seeing. PR incoming. |
Thanks a lot for your support. |
There should be a 1.23.8-preview of bUnit available on nuget.org in a few minutes. Please take it for a spin and let me know if it fixes your issues. |
This change is almost exactly what I was hoping for. Thank you. Just taking it for a spin now! |
Our tests now pass, with our workaround removed, on the latest 1.23.8-preview. Great stuff, thanks @egil |
Our CIs that run our automated component tests run in a parallel way using NUnit, grouped by component.
I've spent a few hours today trying to sniff out (and replicate) the issue in the BUnit repo without success - so this is a partial bug report partial "can anyone lend a hand trying to replicate it in BUnit". It smells like there's a bug here, but I can't put my finger on it.
Something about the execution environment in a parallel test run causes different threads to be used between intial render and subsequent renders. My suspicion is this recent change and the use of Dispatcher.InvokeAsync. When the SynchronizationContext's don't match it goes async and returns the call. This is what I'm suspicious our parallel test execution is somehow causing.
This is causing inconsistent test failures as assertions we make straight after SetParametersAndRender is called sometimes work, and sometimes don't work.
Further, I can work around the issue we're seeing in our codebase with an extension method like this:
Simply put forces our test to wait for the Dispatcher.InvokeAsync to finish, before our no-op one runs.
The time I have spent trying to write a failing test within the BUnit tests has only given me success. No matter how contrived the test becomes.
I have something that looks like this at the moment (but it passes, so is relatively useless :)).
It's gone through a few iterations, but this is the point where I've stopped trying and thought reaching out for some help from the community to identify the issue may be my best course of action :)
Expected behavior:
SetParametersAndRender, when it branches due to a SynchronizationContext mismatch, to still be blocking call until render completes.
Version info:
The text was updated successfully, but these errors were encountered: