Homepage tests refactor #15078

dnenov · 2024-04-02T14:28:01Z

Purpose

HomePage tests refactor. So far, async tests have been causing timeout when ran on master-15. This PR attempts to address the potential resource lock that might be causing the issue. The assumption is that the lock happens when multiple tests attempt to initialize webView2 at the same time, which points to the same user data folder. Here, we are introducing unique data folders each time the HomePage is initialized. The unique data folder is deleted during the Dispose routine.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

tests refactor
introducing unique user data folder for webView2

Reviewers

@QilongTang
@mjkkirschner
@RobertGlobant20

FYIs

@Amoursol

- refactored tests to simplify the async interaction - all tests are now async Task instead of Void - introduced a state monitoring for the HomePage similar to the NotificationExtension

…factor

- eliminating possible causes - current assumption is that the concurrent initialization of webView2 from multiple test instances leads to lock

…factor

- attempt to resolve potential resource lock by introducing unique user data folder for webView2

mjkkirschner · 2024-04-02T15:42:55Z

I'd like to understand some of the assumptions made here:

address the potential resource lock that might be causing the issue - do you have some evidence or past experience that this is the issue? Because the tests run fine on master-5 ( is that the case?) my experience hints at a memory leak issue, but of course it could also be a timing issue etc.
Have you logged into the master-15 test runners and run the tests locally there or watched them run? Looked at any event viewer logs on those machines?
why change the void tests to async?
Have you verified that more than one test is actually running at the same time? It's possible that the webView is still initialized from a previous test especially given the interaction with dispatcher not being flushed in some cases. That does not mean two tests are running in parallel.

@pinzart90 had discovered many issues with the existing tests that used webview2 - @pinzart90 can you share some invariants or best practices that need to be followed to keep these tests from affecting each other?

pinzart90 · 2024-04-02T15:55:30Z

src/DynamoCoreWpf/Views/HomePage/HomePage.xaml.cs

+                        var webViewProcess = Process.GetProcessById(webViewProcessId);
+
+                        this.dynWebView.Dispose();
+                        webViewProcess.WaitForExit(3000);


you might be able to use this event dynWebView.CoreWebView2.Environment.BrowserProcessExited

Oh, great, I can definitely do that. Thank you for the suggestion.

probably have to clean it up too..

pinzart90 · 2024-04-02T15:58:34Z

src/DynamoCoreWpf/Views/HomePage/HomePage.xaml.cs

@@ -143,14 +145,17 @@ private async void UserControl_Loaded(object sender, System.Windows.RoutedEventA
            PathHelper.CreateFolderIfNotExist(userDataDir.ToString());
            var webBrowserUserDataFolder = userDataDir.Exists ? userDataDir : null;

+            var userDataFolder = CreateUniqueUserDataFolder(webBrowserUserDataFolder.FullName);


this has not been a problem with other vebview2 tests...
Are you certain this solves your test issues ?
Also maybe we should just create and cleanup these unique folders only during testing ? either use the testing flag...or make these APIs accessible to the test project and call them directly from there.

For the time being, testing things locally, I didn't run on any sporadic test failures as before.

I was also thinking that maybe this is only necessary under test environment, so if the build completes correctly, I can make the necessary changes.

dnenov · 2024-04-02T16:04:15Z

I'd like to understand some of the assumptions made here:

address the potential resource lock that might be causing the issue - do you have some evidence or past experience that this is the issue? Because the tests run fine on master-5 ( is that the case?) my experience hints at a memory leak issue, but of course it could also be a timing issue etc.

Have you logged into the master-15 test runners and run the tests locally there or watched them run? Looked at any event viewer logs on those machines?

why change the void tests to async?

Have you verified that more than one test is actually running at the same time? It's possible that the webView is still initialized from a previous test especially given the interaction with dispatcher not being flushed in some cases. That does not mean two tests are running in parallel.

@pinzart90 had discovered many issues with the existing tests that used webview2 - @pinzart90 can you share some invariants or best practices that need to be followed to keep these tests from affecting each other?

Hi @mjkkirschner - I am indeed making a few assumptions here without any more evidence but the previous timing out on master-15.

The tests were always working locally, or on master-5. The only place they were not working was master-15, with the issue being a timeout instead of a crash
Because it wasn't a direct test failure, it made the issue more difficult to identify, at least to me
I couldn't identify existing tests around webView2 initialization in bulk - meaning, many individual tests each initializing webView2 instance
The assumption that it may be a resource lock was at least partly inspired by @pinzart90
I am currently running a build on master-15, but I have not been very good at triggering the build correctly before :/ it might be that I need assistance to progress this issue further
I refactored the tests to async to reflect their asynchronous nature - we are awaiting the initialization of the webView2 component, which is async, and we rely on that resource before we commence with the test logic. Previously, I was using a more complicated setup to allow the initialization, but making the tests async just cleaned up the code and made it easier to read. Since nunit supports async tests, I decided to go with that decision, but if we think this is not good practice, I will revert to the previous setup

I will wait for master-15 build to finish or timeout and at least we will have some more data to work with.

- dynWebView.CoreWebView2.Environment.UserDataFolder was failing

…factor

github-actions · 2024-04-02T19:15:59Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

pinzart90 · 2024-04-02T19:45:17Z

I also see here that you did not implement a standard Dispose pattern.
The webview2 component is not disposed when the HomePage is disposed. Might not be a problem, but it means that the webview2 component will be cleaned up whenever the finalizer kicks in.

ALso not sure why we have this font file stuff here https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Views/HomePage/HomePage.xaml.cs#L482
It does not seem to be set anywhere.

Also I am not sure if we should overly complicate the homepage with unique userData folders if we do not need to.

As for the tests, I think it is a good idea to wait for the webview2 control to be initialized before you further interact with it (like scripting)

Not sure why you need CloseViewAndCleanup in each test. The HomePageTest class is derived from DynamoTestUIBase which already closes the DynamoView after each test.

Also you seem to allocate the HomePageViewModel (oddly called StartPageViewModel) inside each test and then leave it to the view.Close events to clean up the HomePageView which references the HomePageViewModel in its Dispose. You should assume that the StartPageViewModel might be already destroyed when using it inside the HomePageView

Also not sure about the TestHook system. If you only need to test the interaction between dynamo UI and web, then why not just override the "scriptObject" in each test ? ex homePage.dynWebView.CoreWebView2.AddHostObjectToScript("scriptObject", lambdas ...) in your tests ?

dnenov · 2024-04-02T21:05:05Z

I also see here that you did not implement a standard Dispose pattern. The webview2 component is not disposed when the HomePage is disposed. Might not be a problem, but it means that the webview2 component will be cleaned up whenever the finalizer kicks in.

ALso not sure why we have this font file stuff here https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Views/HomePage/HomePage.xaml.cs#L482 It does not seem to be set anywhere.

Also I am not sure if we should overly complicate the homepage with unique userData folders if we do not need to.

As for the tests, I think it is a good idea to wait for the webview2 control to be initialized before you further interact with it (like scripting)

Not sure why you need CloseViewAndCleanup in each test. The HomePageTest class is derived from DynamoTestUIBase which already closes the DynamoView after each test.

Also you seem to allocate the HomePageViewModel (oddly called StartPageViewModel) inside each test and then leave it to the view.Close events to clean up the HomePageView which references the HomePageViewModel in its Dispose. You should assume that the StartPageViewModel might be already destroyed when using it inside the HomePageView

Also not sure about the TestHook system. If you only need to test the interaction between dynamo UI and web, then why not just override the "scriptObject" in each test ? ex homePage.dynWebView.CoreWebView2.AddHostObjectToScript("scriptObject", lambdas ...) in your tests ?

Thank you for the very detailed review and response @pinzart90, I really do appreciate it! Let me mull over all the pointers you left, and if it's OK I can ping offline to elaborate if something is not clear.

- ignoring tests again to see if allowing the HomePage to be initialized at all during Test mode is causing the time out - remove old code

- revert allowing HomePage initialization under test mode

…factor

QilongTang · 2024-05-17T17:08:56Z

What is the state of this PR? @dnenov Is it now more ready for review?

dnenov added 5 commits March 18, 2024 18:18

simplify tests

cf6971c

- refactored tests to simplify the async interaction - all tests are now async Task instead of Void - introduced a state monitoring for the HomePage similar to the NotificationExtension

Merge remote-tracking branch 'upstream/master' into homepage-tests-re…

aef2603

…factor

leave all but 1 test

fb3bcec

- eliminating possible causes - current assumption is that the concurrent initialization of webView2 from multiple test instances leads to lock

Merge remote-tracking branch 'upstream/master' into homepage-tests-re…

732394e

…factor

unique user data folder

915bf96

- attempt to resolve potential resource lock by introducing unique user data folder for webView2

pinzart90 reviewed Apr 2, 2024

View reviewed changes

dnenov added 2 commits April 2, 2024 20:00

dynWebView.CoreWebView2.Environment.UserDataFolder failing

dbe49f9

- dynWebView.CoreWebView2.Environment.UserDataFolder was failing

Merge remote-tracking branch 'upstream/master' into homepage-tests-re…

6cd83a1

…factor

dnenov added 3 commits April 3, 2024 14:57

ignoring tests again, remove old code

84d10cd

- ignoring tests again to see if allowing the HomePage to be initialized at all during Test mode is causing the time out - remove old code

revert homePage under test mode

3168b22

- revert allowing HomePage initialization under test mode

Merge remote-tracking branch 'upstream/master' into homepage-tests-re…

24ca262

…factor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homepage tests refactor #15078

Homepage tests refactor #15078

dnenov commented Apr 2, 2024

mjkkirschner commented Apr 2, 2024 •

edited

Loading

pinzart90 Apr 2, 2024

dnenov Apr 2, 2024

pinzart90 Apr 2, 2024

pinzart90 Apr 2, 2024

dnenov Apr 2, 2024

dnenov commented Apr 2, 2024

github-actions bot commented Apr 2, 2024 •

edited

Loading

pinzart90 commented Apr 2, 2024 •

edited

Loading

dnenov commented Apr 2, 2024

QilongTang commented May 17, 2024

Homepage tests refactor #15078

Are you sure you want to change the base?

Homepage tests refactor #15078

Conversation

dnenov commented Apr 2, 2024

Purpose

Declarations

Release Notes

Reviewers

FYIs

mjkkirschner commented Apr 2, 2024 • edited Loading

pinzart90 Apr 2, 2024

Choose a reason for hiding this comment

dnenov Apr 2, 2024

Choose a reason for hiding this comment

pinzart90 Apr 2, 2024

Choose a reason for hiding this comment

pinzart90 Apr 2, 2024

Choose a reason for hiding this comment

dnenov Apr 2, 2024

Choose a reason for hiding this comment

dnenov commented Apr 2, 2024

github-actions bot commented Apr 2, 2024 • edited Loading

UI Smoke Tests

pinzart90 commented Apr 2, 2024 • edited Loading

dnenov commented Apr 2, 2024

QilongTang commented May 17, 2024

mjkkirschner commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

pinzart90 commented Apr 2, 2024 •

edited

Loading