Homepage tests refactor

[dnenov]

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

[mjkkirschner]

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

1. 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.
2. 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?
3. why change the void tests to async?
4. 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?

[dnenov]

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

1. 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.
2. 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?
3. why change the void tests to async?
4. 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.

[github-actions[bot]]

UI Smoke Tests

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

[pinzart90]

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]

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.

[QilongTang]

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