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

Update BlazorWebView Device Tests to have longer timesouts and more retries #21318

Merged
merged 5 commits into from Mar 22, 2024

Conversation

Eilon
Copy link
Member

@Eilon Eilon commented Mar 19, 2024

This change was motivated by some flakiness in BlazorWebView tests. The tests fail sometimes, but pass on rerun. And there are no customer reports of these particular errors either.

Example failure: https://dev.azure.com/xamarin/public/_build/results?buildId=111379&view=ms.vss-test-web.build-test-results-tab&runId=2634804&resultId=100000&paneView=debug

With this test output:

System.Exception : Waited 7500ms but couldn't get CoreWebView2.DOMContentLoaded to complete.
   at Microsoft.Maui.MauiBlazorWebView.DeviceTests.WebViewHelpers.WaitForWebViewReady(WebView2 wv2) in D:\a\1\s\src\BlazorWebView\tests\MauiDeviceTests\WebViewHelpers.Windows.cs:line 53
   at Microsoft.Maui.MauiBlazorWebView.DeviceTests.Elements.BlazorWebViewTests.<>c__DisplayClass2_0.<<BlazorWebViewUsesStartPath>b__1>d.MoveNext() in D:\a\1\s\src\BlazorWebView\tests\MauiDeviceTests\Elements\BlazorWebViewTests.cs:line 131
--- End of stack trace from previous location ---
   at Microsoft.Maui.Dispatching.DispatcherExtensions.<>c__DisplayClass3_0.<<DispatchAsync>b__0>d.MoveNext() in D:\a\1\s\src\Core\src\Dispatching\DispatcherExtensions.cs:line 88
--- End of stack trace from previous location ---
   at Microsoft.Maui.Dispatching.DispatcherExtensions.<>c__DisplayClass2_0`1.<<DispatchAsync>b__0>d.MoveNext() in D:\a\1\s\src\Core\src\Dispatching\DispatcherExtensions.cs:line 67
--- End of stack trace from previous location ---
   at Microsoft.Maui.MauiBlazorWebView.DeviceTests.Elements.BlazorWebViewTests.BlazorWebViewUsesStartPath() in D:\a\1\s\src\BlazorWebView\tests\MauiDeviceTests\Elements\BlazorWebViewTests.cs:line 127
--- End of stack trace from previous location ---

There are a few parts to this change:

  1. Increase retries and timeouts of all BlazorWebView tests.
  2. Fix place where there was no retry at all
  3. Change the BlazorWebView tests to use the same HandleTestBase classes used in other control tests. For some reason the tests would run fine on CI, but fail on my machine due to some service not being registered. No idea how that's possible. But the base test classes used in these tests got very out-of-date compared to other tests, so this brings them inline with other tests.
    • Why not use the actual same base classes? For now that's a lot more work because the Blazor test project can't reference the Controls test project without causing problems (it's not a regular ClassLib). And moving the classes to a new shared library is a lot more work. And the Blazor tests don't need much to begin with (I deleted a lot of unused code from the copy).
  4. Small change to the test base classes to be compatible with the async disposal needed by BlazorWebView tests. This is a superset of what was being done before (async disposal also calls sync disposal)

@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Mar 19, 2024
@Eilon Eilon marked this pull request as ready for review March 21, 2024 20:17
@Eilon Eilon requested review from a team as code owners March 21, 2024 20:17
@Eilon Eilon changed the title WIP: Update BlazorWebView Device Tests to use newer handle test base classes Update BlazorWebView Device Tests to have longer timesouts and more retries Mar 21, 2024
public static partial class WebViewHelpers
{
const int MaxWaitTimes = 30;
const int WaitTimeInMS = 1000;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retries are now consistently 30 retries * 1000ms.


if (testHtmlLoadedAttributeValue != "true")
await Retry(async () =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code here never had a retry (despite the exception message saying it did!). And this is where the test failure that motivated this change occured - so hopefully adding retries mitigates that particular flakiness.

@@ -19,7 +19,7 @@

namespace Microsoft.Maui.DeviceTests
{
public class HandlerTestBasement : TestBase, IDisposable
public class HandlerTestBasement : TestBase, IAsyncDisposable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAUI team reviewers: This is the part of the change that can affect other tests: I changed this to be IAsyncDisposable (which the BlazorWebView tests always did). In theory this is a good change anyway because MauiApp is IAsyncDisposable so should really always be disposed that way.

@@ -4,7 +4,7 @@

namespace Microsoft.Maui.DeviceTests
{
public abstract class CoreHandlerTestBase : HandlerTestBase, IDisposable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This was never needed because the base class already declared its disposableness.)

@@ -0,0 +1,20 @@
using Microsoft.Maui.Controls;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file and the next few files (AppStub, ControlHandlerTestBase, etc.) are copied from the MAUI Controls DeviceTests, but I then removed anything that wasn't needed by the BlazorWebView tests (it was too hard to get it to compile, and too hard to directly reuse the types).

@Eilon
Copy link
Member Author

Eilon commented Mar 22, 2024

@ruiminhu / @PureWeen - can one of you do a tiny review for the small changes to the DeviceTests base handler types? (I put comments in the PR to explain everything).

@dotnet/dotnet-maui-blazor-eng - anyone available to review my BlazorWebView test changes? There's a lot of code shuffling but the core change is simply to add more retries to some test segments that have failed on CI (but always pass on rerun 😠 ).

@PureWeen PureWeen enabled auto-merge (squash) March 22, 2024 20:37
@PureWeen PureWeen merged commit 8482028 into main Mar 22, 2024
47 checks passed
@PureWeen PureWeen deleted the eilon/blazorwebviewtest-flakiness branch March 22, 2024 22:36
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants