1.19.14 dotnet test crashes when running multiple tests #1064
Comments
|
I can reproduce with multiple versions. It appears this is linked to upgrading my projects to .NET 7. It happens more frequently the more tests I run at once. |
|
Hi @groogiam, Thanks for reaching out. Can you share the component under test and the test itself, so we can have a look and better help you. |
|
It appears returning Task.CompletedTask is causing this. Our framework has a Action bar component which allows actions (buttons) to be configured which float at the top of a component. This component then provides and executing and executed event callback. We have code gen that scaffolds these out to: private Task ActionExecuting()
{
IsLoading = true;
return Task.CompletedTask;
}
private async Task ActionExecuted()
{
IsLoading = false;
await Task.CompletedTask;
}For some reason E.g. Calling The strange thing is that the test that is actually failing is not the one that is calling Click. That test passes and another unrelated test that has This is the test that seems to be triggering the exception [Fact]
public void PrimaryOwner_Lookup_Filter_Is_Reset_When_In_Add_New_Clicked()
{
using var ctx = TestContextWithMockHttp.CreateDefault();
ctx.JSInterop.Mode = JSRuntimeMode.Loose;
const string entityName = nameof(Asset);
// setting up the entity
var entity = new Asset
{
Id = 25,
Name = "Asset 1",
TypeDdName = AppPotion.Bd.DropDowns.AssetTypeDd.District.Name,
TypeDdVal = AppPotion.Bd.DropDowns.AssetTypeDd.District.Value,
StatusDdName = AppPotion.Bd.DropDowns.AssetStatusDd.Active.Name,
StatusDdVal = AppPotion.Bd.DropDowns.AssetStatusDd.Active.Value,
CountryDdName = AppPotion.Bd.DropDowns.CountryDd.USA.Name,
CountryDdVal = AppPotion.Bd.DropDowns.CountryDd.USA.Value,
CreatedBy = "System",
CreatedDate = DateTime.Now,
LastModifiedBy = "System",
LastModifiedDate = DateTime.Now
};
// mocking the http calls which happen while initially loading the component
ctx.MockHttp.AddEmptyDocumentManagementResponses();
ctx.MockHttp.AddEmptyEntityTagResponses();
ctx.MockHttp.When($"http://localhost/api/{entityName}/EntityPermission/25").RespondJson(new EntityPermissionDescriptor
{
EntitySystemName = entityName,
EntityId = 25,
Create = true,
Delete = true,
Read = true,
Update = true
});
ctx.MockHttp.When($"http://localhost/api/{entityName}/EntityPermission/0").RespondJson(new EntityPermissionDescriptor
{
EntitySystemName = entityName,
EntityId = 0,
Create = true,
Delete = true,
Read = true,
Update = true
});
ctx.MockHttp.When($"http://localhost/odata/{nameof(Asset)}({entity.Id})").RespondOData(entity);
// inject service for setting the application in Admin User context
ctx.SetLogin(BlazorTestHelpers.CreateAdminUserContext());
var appState = ctx.Services.GetRequiredService<AppState>();
appState.SetPrivateProperty(nameof(AppState.ViewPortSize), ViewPortSize.Large); //viewport size must be large so the lookup component renders as a grid.
var parameters = new List<ComponentParameter>
{
ComponentParameter.CreateParameter(nameof(Ui_Asset_AssetEditor.EntityId),
entity.Id)
};
var cut = ctx.RenderComponent<Ui_Asset_AssetEditor>(parameters.ToArray());
//wait for web requests to complete
cut.WaitForState(() => cut.Instance.EntityPermission != null);
//Asserts
// Click Add New Button
cut.Find(
$"#Asset-Actions-Bar-{cut.Instance.ComponentInstanceId} [title*='Reset form to allow creation of a new entity.']").Click();
cut.Render();
//Asserts
}This is the test where the exception is thrown. Thanks for your help on this. |
|
This is a curious case. The In your test, do you have a very deep component tree (many nested components)? |
|
The components go 2 maybe 3 deep. The more curious thing is that this only manifests when running tests in parallel. Running a single test via visual studio or resharper test window does not result in the exception. It updated all our tests Replacing the Click() calls with awaited calls ClickAsync and this does seem to resolve the issue. |
|
That Infinite loop could be the result of an inconsistent state in Blazors internal render tree which that method that seems to overflow is walking down. Why that happens is not clear. Are each test totally independent? That is, no shared state between them? |
|
As far as I can tell there is not. I spent some time today and yesterday looking through the tests and infrastructure. There are some private members declared in some of the tests used by multiple cases. My understanding of XUnit is that each of the tests method would spin up a new class instance so different instances would not be accessing those members. All the containers and mocks are defined locally in the methods and the services do not seem to have any static properties that would allow state to be shared across tests. |
|
That is correct. Xunit creates a new instance of a test class every time, which means all instance fields are recreated. |
|
I do not see anything in the tests you have shared that should cause this. One small suggestion would be to wrap your HttpClient in an abstraction, e.g. |
|
Hello, I have also the same problem on a new project I'm working on with I joined this project, mid way so there were already alot of tests in place and it's hard to track what's causing it, but by enabling parallelize I get the same behavior as OP transiently, more frequently, so sometimes it would fail, sometimes not... One thing of note is that like OP, we are also using the Also recently it was suddently happening very frequently even on sequential(non parallel) run, by removing the These kind of problems are always hard to diagnose, troubleshoot, if I figure out a reproducible I'll post it here. |
|
Thanks for jumping in @David-Moreira. I am pretty sure that things are still being cleaned up without disposing of the TestContext. Just want to confirm. Are you not seeing the issue at all when you do not dispose of the TestContext after the test is done running? |
|
So, I don't see anything too fancy on your disposal code, but maybe the unawaited async disposal could have some side effect... Anyway this might be or not be the same apparent cause as OP. For The Parallel runs: |
|
Thanks, @David-Moreira, it is certainly giving us something to investigate. The stack overflow that you and @groogiam are seeing indicates that the Blazor renderer's render tree is in an inconsistent state. That should not be something that could happen and something that happens outside of bUnits control. It could be that it happens when we are disposing of the Blazor renderer and we are not guarding against accessing the render tree at that point. cc. @linkdotnet. |
|
Hey @groogiam and @David-Moreira, I am pushing a preview release to Nuget right now. It contains some code that may fix the issues you are experiencing. If you can, please give it a try in your code base. It is available here: https://www.nuget.org/packages/bunit/1.20.7-preview You can also try to extend the default "wait for" timeout. The default is 1 second, which may sometimes be too short if running on slow hardware. Just set the static property Another thing you can try to aid debugging is to enable logging of the renderer logic. See https://bunit.dev/docs/misc-test-tips.html#capturing-logs-from-ilogger-in-test-output for more. |
egil
added
the
investigate
|
@egil Appreciated the fast response. |
|
Forgot to mention that I was on 1.18.4 and not on 1.19
I don't know if any of these help. And they're kinda unrelated to the original overflow problem... I will try to keep posting feedback but as you know sometimes time is limited to be allocating to test troubleshooting. |
We changed things in 1.19 that did forced some of the race conditions more into the light, so if you have more tests failing between the 1.18 and 1.19 that is not surprising.
bUnit has never waited for components that perform async operations. If you have anything that is not completely synchronous (returning Task.CompletedTask counts), then you need to use one of the WaitFor methods. E.g. if you have a Task.Delay or similar.
My experience with our own tests is that with the latest preview they are completely stable, but I did have to increase the default wait for timeout when running on CI or slow hardware. My AMD 5950x has no problem ever. 5 year old laptop and GitHub actions, especially GitHub actions on windows and osx, has. |
|
@egil The change does not seem to help the original issue (non async click calls) on my local machine or CI. However it does seem stable on the updated code that utilizes async calls for actions. Setting TestContext.DefaultWaitTimeout to 30 seconds does seem to significantly slow down the test execution though. It seems like quite a few of my tests wait the entire timeout for some reason. Thanks. |
Are you disposing the TestContext at the end of the test method? Doing so will stop the renderer and prevent any further renders from happening. Also, try enabling logging as mentioned previously. If you have tests that sometimes completes successfully and sometimes not, you can compare the log output between the two and get a good idea of what is going on. If you share the log statements here, we call also help parse them.
If that is the case, then you are probably not writing the right assertions. The "wait for" methods will attempt the assertion/check you pass to them synchronously after each render is completed (but before OnAfterRender runs). If you have tests that block/waits for the entire timeout period, then that very likely means that the stat/elements/assertions you are waiting for never happens. The reason why it works to extend the (default) timeout is that there are cases where the machine running the tests are simply so slow that none or not enough renders cycles happens which takes the component under test to the desired state, before the timeout is reached. |
|
I might be going off thread here...But are you guys using I'm thinking most of my problems might just straight up be to do with Thread Safety, I'm not sure how MSTest runs the tests that are inside a single The new tests that were failing because of some concurrent update (what the hell), I decided to refactor them into xUnit, put the Anyway since removing the |
We use xUnit. But I do not think MSTest should be a problem. Haven't heard anybody say so.
I would love an example of a test that works on xUnit and not in MSTest. bUnit is supposed to work with all major test frameworks.
I explicitly fixed the dispose problem in the latest 1.20 preview release, so it should be safe to clean up and tell the renderer that you are done with the test. |
I forgot a very important note, the SF Dictionary concurrent issue, did not seem to happen at all running test locally, only on pipeline everytime. In this case it's in BitBucket. And well I was not seeing any reason why some concurrent access would be done, so I decided to try a diff testing framework and it worked... Not saying it's the testing framework fault, there might be more at play here... but that was what immediately worked in this case. Anyway I'll see if I get around to it. The original issue does not seem a problem anymore, at least for me as I'm even fine without the explicit Dispose. That's something that was already in the project I am now working on and I don't think it's needed. Thanks. |
|
Appreciate that. As far as I know, MSTest does not have a synchronization context, that could be the culprit. NUnit and xUnit have that. Also, with xUnit, you have to try to have shared state between tests, e.g. a shared TestContext. I think that is an easier trap to fall into with MSTest and NUnit. |
|
@groogiam what's the status from you? Still seeing the original issue with the stack overflow? If not, I would like to close this issue. |
|
@egil Yes, I still see the stack overflow on code that does not use the Async overloads even though each unit tests disposes it's context. Everything works fine if I use the async overloads. Still seeing intermittent failures with WaitForX but I'm not sure that is related to this issue. |
Please bare with me. Too many threads in this issue for me to keep track of things. Can you simplify what you say here to a basic example of when this happens vs. when it doesn't. It sounds like you are changing between a sync vs async version where it works in one but not the others.
In general, if you have anything in a component that causes an async re-render (StateHasChanged) of the component, e.g. a timer or delay, that re-render may happen before or after the "WaitFor" call in your test, since the test code is running in one thread and the re-render happens on another thread. If a trigger of a StateHasChanged can be controlled from the test, then you can make your tests deterministic. E.g. if a re-render is triggered by a timer, consider something like https://www.nuget.org/packages/TimeProviderExtensions to control time during testing. |
This still causes a stack overflow exception cut.Find(
$"#Actions-Bar-{cut.Instance.ComponentInstanceId} [title*='Reset form to allow creation of a new entity.']").Click();where this does not await cut.Find(
$"#Actions-Bar-{cut.Instance.ComponentInstanceId} [title*='Reset form to allow creation of a new entity.']").ClickAsync(new MouseEventArgs());I've already modified my tests to use the async calls and that seems to works just fine with the updated version. So if you want to call it good and close the issue I would not have any problem with that. But this is essentially the thing that caused me to open the issue in the first place.
The issue seems to be timing with the async network calls in the OnInitializedAsync. I have code that essentially waits for a property that get set by one of these network calls directly after the initial render is called, but when under load things will just randomly hang for whatever the wait timeout is and fail. We can table this for now. I can submit another issue when I can finally work out a repro. It could very well be something silly I am doing. Sorry to muddy the waters. |
|
Let's keep the issue open for now, but focus on the overflow. Can you share the code that is in the "onclick" handler that is being invoked? It may give me a hint at what I should look for. Just to round off the other issue: You are probably able to make the tests deterministic by not injecting HttpClient directly into your components. Instead create an abstraction, e.g.: public interface IDataService
{
Task<EntityPermissionDescriptor> GetPermissionDescriptor(string entityName, int id);
Task<Entity> GetEntity(string entityName, int id);
}
// use in production
public class DataService : IDataService
{
private readonly HttpClient httpClient;
public DataService(HttpClient httpClient)
{
this.httpClient = httpClient;
}
public async Task<EntityPermissionDescriptor> GetPermissionDescriptor(string entityName, int id)
{
// copy httpclient call code from components in here
}
public Task<Entity> GetEntity(string entityName, int id)
{
// copy httpclient call code from components in here
}
}
// In your blazor app, in register the service
// via Services.AddScoped/AddSingleton<IDataService, DataService>()
// use this version in testing
public class FakeDataService : IDataService
{
private readonly List<EntityPermissionDescriptor> descriptors = new();
public void SetPermissionDescriptor(EntityPermissionDescriptor descriptor)
{
descriptors.Add(descriptor);
}
public Task<EntityPermissionDescriptor> GetPermissionDescriptor(string entityName, int id)
=> Task.FromResult(descriptors.Single(x => x.EntitySystemName == entityName && x.EntityId == id));
// todo add other overloads
}Then in your tests, you can do something like this: [Fact]
public void PrimaryOwner_Lookup_Is_Filtered_For_Companies_With_AssetOwnerMap_When_Entity_In_Add_New_Mode()
{
using var ctx = TestContextWithMockHttp.CreateDefault();
var dataService = new FakeDataService();
cts.Services.AddSingleton<IDataService>(dataService);
ctx.JSInterop.Mode = JSRuntimeMode.Loose;
const string entityName = nameof(Asset);
// mocking the http calls which happen while initially loading the component
dataService.SetPermissionDescriptor(new EntityPermissionDescriptor
{
EntitySystemName = entityName,
EntityId = 0,
Create = true,
Delete = true,
Read = true,
Update = true
});
// inject service for setting the application in Admin User context
ctx.SetLogin(BlazorTestHelpers.CreateAdminUserContext());
var appState = ctx.Services.GetRequiredService<AppState>();
appState.SetPrivateProperty(nameof(AppState.ViewPortSize), ViewPortSize.Large); //viewport size must be large so the lookup component renders as a grid.
var cut = ctx.RenderComponent<Ui_Asset_AssetEditor>();
//wait for web requests to complete
cut.WaitForState(() => cut.Instance.EntityPermission != null);
//Asserts
}Because the Having the IDataService abstraction also makes it possible to add caching or similar in the production version that will be shared by all components that may want to call the API, so there are definitely some benefits to having an abstraction like that besides making testing easier. |
private async Task Refresh()
{
if (!await ConfirmOverwriteUnsavedChanges())
{
return;
}
IsLoading = true;
try
{
await LoadEntityInternal(); //makes odata http request
await LoadEntityPermissionAsync(); //checks singleton service for cache entry otherwise makes http request for the permission.
}
finally
{
IsLoading = false;
}
await Ctx.MessageBus.PublishAsync(new EntityRefreshedMessage<TEntity>
{
Sender = this,
Entity = Entity
});
await Ctx.MessageBus.PublishAsync(new EntityRefreshedMessage
{
Sender = this,
EntityTypeName = EntityType.Name,
Entity = Entity
});
Ctx.Notifier.SystemNotifySuccess("Refresh Complete");
} |
|
A few questions:
|
This returns false in the tests.
The message bus publishes don't do anything in the tests. In a live app other editor or list components might handle them but here nothing is wired to them. The LoadEntityInternal and LoadEntityPermissionAsync may have hooks attached which update properties on the component. In most cases none of these will explicitly called StateHasChanged as it is assumed the component will re-render after the method has completed. The tests in most cases actually verify properties have been set on the instance as the affected markup would be in a sub component usually a lookup component. Our assertions on these properties pass without issue but the background execution seems to continue even after the context is disposed causing the stack overflow. |
|
Ok. Thank you for the update. The difference between What are you doing in the test after triggering the onclick handler? |
|
In this case we are calling a |
|
Interesting. Why are you calling Calling it just causes the component to rerender. But doing this right after calling Either way, calling Can you help with a stack trace for when this happens in the |
In this case cut.Render seem to be there mostly for timing. As we were not awaiting the click the asserts on the state would randomly fail without the render call. cut.Render();
// Verify Filters are Reset
existingFilter = cut.Instance.PrimaryOwnerIdLookupFilters.OfType<FilterDescriptor>().FirstOrDefault(filter => filter.Member == "OwnedAssets/any(p: {0})|p.AssetId");
Assert.NotNull(existingFilter); //this randomly fails without the render call above
The exception does not seem to be thrown in this test method directly. It is thrown by the test referenced in the original post when run in conjunction with other tests that have this pattern of non awaited click handlers. |
|
Ok. Thanks for clarifying. Calling Perhaps this will work instead: cut.WaitForAssertion(() => cut.Instance.PrimaryOwnerIdLookupFilters.OfType<FilterDescriptor>().First(filter => filter.Member == "OwnedAssets/any(p: {0})|p.AssetId"));Calling click will cause the renderer to complete any async renders as fast as the async task that is being awaited completes. Calling Render will just invoke |
|
I agree it is a happy accident that it works using the extra render. Both awaiting the |
If you are disposing of the TestContext between each test then that should not be the case, but I would need to see the full test setup you have to understand exactly what is going on. Is it possible you can share it in private? |
|
I'd be happy to share greater details of the repository in private. Please let me know you would like to proceed with that. Thanks. |
If you have the code on github, you can give add me to the repo and I can look there. Otherwise, feel free to zip up the relevant parts of the source code and send it to me via email (egil at assimilated.dk). EDIT: include points to the test in question that are failing, and test log from runs that have failed. |
|
Sorry wrong issue :) |
|
@egil Just sent an email with zipped bUnit test project. Let me know if you need anything else. |
Thank you. Ill take a look as soon as possible and report back. |
Describe the bug
After updating my nuget package reference to 1.19.14 dotnet test crashes with a stack overflow exception when running multiple tests.
This appears to happen after rendering when waiting for state. E.g.
Results in this exception
Expected behavior:
Tests should not crash dotnet tests and work as in previous versions of bunit.
Version info:
Additional context:
I'm still trying to create a simple repro but reverting to 1.18.4 seems to resolve the issue.
The text was updated successfully, but these errors were encountered: