Skip to content

Conversation

@pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Dec 1, 2023

  1. Using the default SyncronizationContext during tests will cause async operations to be continued on the ThreadPool
    ex.
 async DoStuff() {
   await http.Get("www.google.com");
   someUIControl.Visible = Visibility.Hidden;
}
If you call DoStuff() from Thread1, the code with "someUIControl.Visible = Visibility.Hidden;" will run on a different thread (from the ThreadPool).
  • To fix this scenario, I moved all our UI tests on the WPF sync context (DispatcherSyncronizationContext). This ensures that all async/await methods resume execution on the same thread.
  • A note worth mentioning is that Task.Run or Task.Factory.StartNew will, by default, run the task on a ThreadPool thread. We should make sure that we are delegating all UI calls to the UI dispatcher or ensuring the Task.Run calls are executed directly on the UI thread (via argument TaskScheduler.FromCurrentSynchronizationContext())
  1. Some tests will create webview2 controls.
  • Some of the webview 2 controls are visible and initializeAsync is called, but they finish so fast (the thread dos not process the message in time) that Dispose is called before they get a chace to finish initialization (System.ObjectDisposedException might be thrown)

  • Some of the webview 2 controls are created hidden and initializeAsync will be blocked until the control becomes visible. The tests will finish and Dispose will be called. (ComExceptions or ObjectDisposedException).

    Most of these crashes might happen outside of the tests that caused them. Depends on what test actually calls DoEvents (i.e tells the thread to process its message loop)

    To fix these I moved most of the webview2 initialize calls on the Loaded event in this PR. Also I ensured that all tests that create visible webview2 controls are processed and finished before the test continues and eventually disposes of the control (using DispatcherUtil.DoEventsLoop)

@pinzart90 pinzart90 marked this pull request as ready for review December 4, 2023 17:40
// We need to run the async ExecutePackageDownload function in a separate thread so that the Thread.Sleep does not mess up the other awaiting tasks.
//actually perform the download & install operations
pmVmMock.Object.ExecutePackageDownload(id, pkgVer, "");
}).Wait();
Copy link
Contributor Author

@pinzart90 pinzart90 Dec 4, 2023

Choose a reason for hiding this comment

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

Task.Run is a viable option here because there is no UI interaction from ExecutePackageDownload

This could be resolved by running pmVmMock.Object.ExecutePackageDownload(id, pkgVer, ""); on the test thread but adding a DispatcherUtil.DoEvents() after the thread.Sleep bellow (would have been needed if there was UI interaction in ExecutePackageDownload)

Copy link
Member

Choose a reason for hiding this comment

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

could you also make this test async, await the download and get rid of the sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not that easy to await the download. The ExecutePackageDownload would have to be refactored to return a task fo the download and install operations.

@pinzart90
Copy link
Contributor Author

{
AppDomain.CurrentDomain.AssemblyResolve -= assemblyHelper.ResolveAssembly;
}
System.Console.WriteLine("Finished test: " + TestContext.CurrentContext.Test.Name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These do add more logs to the jobs output but they are useful

/// <returns></returns>
private static object ExitFrame(object frame)
/// <param name="check">When check returns true, the even loop is stopped.</param>
[SecurityPermission(SecurityAction.Demand, Flags = SecurityPermissionFlag.UnmanagedCode)]
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I copied it from the DoEvents method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do know that the Dispatcher does call into unmanaged code when it processes its message queue

Copy link
Member

Choose a reason for hiding this comment

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

I feel you can probably get rid of it all, I think security permissions system is removed.

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2023-12-05 at 10 32 07 AM

}

[Test,Category("Failure")]
[Test]
Copy link
Member

Choose a reason for hiding this comment

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

woohoo!

}

// Wait for the DocumentationBrowserView webview2 control to finish initialization
DispatcherUtil.DoEventsLoop(() =>
Copy link
Member

@mjkkirschner mjkkirschner Dec 5, 2023

Choose a reason for hiding this comment

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

so essentially all devs working on tests with webview2 controls need to use this API for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully not all the time. Only if the webview2 control is expected to be visible during the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to add some sort of docs somewhere

Copy link
Member

@mjkkirschner mjkkirschner Dec 5, 2023

Choose a reason for hiding this comment

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

It would be a really nice addition with a custom analyzer using roslyn. We could detect usage of an API that would show the WebView control in a method marked with the test attribute, and add a compiler error if we don't see this method used... anyway, for another day.

@pinzart90 pinzart90 merged commit ff8d3af into webview2_initialization Dec 5, 2023
@pinzart90 pinzart90 deleted the test_webview2 branch December 5, 2023 15:34
pinzart90 added a commit that referenced this pull request Dec 6, 2023
* upate

* Update ViewExtension.cs

* Update DocumentationBrowserView.xaml.cs

* Update NotificationsViewExtension.cs

* update

* update

* Update NotificationCenterController.cs

* Test webview2 (#14670)

* update

* update

* Update DocumentationBrowserView.xaml.cs

* update

* Update DocumentationBrowserView.xaml.cs

* update

* update

---------

Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com>

* Update NotificationCenterController.cs

* Update DispatcherUtil.cs

* Update CS_SDK.props

* Revert "Update CS_SDK.props"

This reverts commit cfaceb2.

---------

Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants