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

Test crash fixes #14789

Merged
merged 39 commits into from
Jan 24, 2024
Merged

Test crash fixes #14789

merged 39 commits into from
Jan 24, 2024

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Dec 19, 2023

I changed my approach to the webview2 lifecycle issues:
Instead of trying to catch all misuses of webview2 (not disposing during tests in which they are allocated), I added a WebView2 derived class (to better control disposal and give better debugging logs) so that we can let the gc/finalizer clean them safely.

Added unhandled exception handlers - for better logs during testing
Fixed an issue with the Analytics tests - depending on the order of execution, some tests were hanging
Added better logging for Serialization tests

Copy link

UI Smoke Tests

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

@pinzart90 pinzart90 changed the title Test crash logs [WIP]Test crash logs Dec 20, 2023
@pinzart90 pinzart90 added the DNM Do not merge. For PRs. label Dec 20, 2023
@pinzart90 pinzart90 marked this pull request as ready for review December 20, 2023 05:21
Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

github-actions bot commented Jan 8, 2024

UI Smoke Tests

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

Copy link

github-actions bot commented Jan 8, 2024

UI Smoke Tests

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

@@ -786,7 +787,7 @@ private void RestartTestSetup(bool startInTestMode)

//create the view
View = new DynamoView(ViewModel);
SynchronizationContext.SetSynchronizationContext(new SynchronizationContext());
Copy link
Member

@mjkkirschner mjkkirschner Jan 10, 2024

Choose a reason for hiding this comment

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

these methods are only used for a few tests - correct?
RestartTestSetup and RestartTestSetupWithNewSettings I mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes..just for 2 tests

@@ -854,7 +855,7 @@ private void CompareWorkspaceViews(WorkspaceViewComparisonData a, WorkspaceViewC
var valueB = b.NodeViewDataMap[kvp.Key];
Assert.AreEqual(valueA, valueB,
string.Format("Node View Data:{0} value, {1} is not equal to {2}",
a.NodeViewDataMap[kvp.Key].Name, valueA, valueB));
a.NodeViewDataMap[kvp.Key].Name, JsonSerializer.Serialize(valueA), JsonSerializer.Serialize(valueB)));
Copy link
Member

Choose a reason for hiding this comment

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

can this possibly work for all cases? I think it's fragile because we never know what test graphs are going to run.

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 did find it useful for investigating some parallel tests. Ex https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/test_parallel_selfserve/87/testReport/junit/DynamoCoreWpfTests/JSONSerializationTests/SerializationTest__C___Jenkins__workspace__Dynamo__DynamoSelfServe__test_parallel_selfserve__Dynamo__test__core__dsevaluation__regress722_dyn__/
Not sure how much to invest in better visibility here.
I though I would keep it since it does not seem that it would break stuff. (but I can remove it if you think it is dangerous)

@@ -11,9 +12,24 @@ public class Setup
{
private AssemblyHelper assemblyHelper;

private void CurrentDispatcher_UnhandledException(object sender, DispatcherUnhandledExceptionEventArgs e)
{
e.Handled = true;
Copy link
Member

@mjkkirschner mjkkirschner Jan 10, 2024

Choose a reason for hiding this comment

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

so, do all these handlers have the potential for hiding real test failures from us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they do. I will try it again (to see if these actually work during testing - as in runtime). If they do not do anything useful..I will remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed them since they do not seem to add any value to the testing context

[SetUp]
public void SetUp()
{
TestUtilities.WebView2Stamp = TestContext.CurrentContext.Test.Name;
Copy link
Member

Choose a reason for hiding this comment

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

yeah I guess it would just be nice if instead of sprinkling this logic all over for any UI test it was just handled in the DynamoWebView2 wrapper.

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 could try it. But it would mean to have more testing logic in runtime code. It would probably involve reflection and asembly checking..

Copy link
Member

Choose a reason for hiding this comment

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

let's leave that potential refactor for later, for now it's just one static string.

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

a couple questions.

Copy link

UI Smoke Tests

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

@pinzart90
Copy link
Contributor Author

@pinzart90 pinzart90 removed the DNM Do not merge. For PRs. label Jan 10, 2024
Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

@pinzart90
Copy link
Contributor Author


clientMoq.Verify(c => c.CreateCommandEvent("TestCommand", "", null), times);
e = Analytics.TrackTaskCommandEvent("TestCommand", "TestCommand description", null, new Dictionary<string, object>() { } ).Result;
clientMoq.Verify(c => c.CreateTaskCommandEvent("TestCommand", "", null, null), times);

e = Analytics.TrackFileOperationEvent(this.TempFolder, Actions.Read, 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using TrackTaskFileOperationEvent here?

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 did not intend to convert all obsolete APIs to the new ones with this PR.

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

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

Some minor comments, then LGTM.

Copy link

UI Smoke Tests

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

Copy link
Contributor

@sm6srw sm6srw left a comment

Choose a reason for hiding this comment

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

LGTM!

@pinzart90 pinzart90 merged commit 2a6152a into master Jan 24, 2024
22 checks passed
@pinzart90 pinzart90 deleted the test_crash_logs branch January 24, 2024 19:28
QilongTang pushed a commit that referenced this pull request Jan 27, 2024
* update

* Update NodeModelAssemblyLoader.cs

* update

* update

* update

* update

* Update AnalyticsTests.cs

* Update SerializationTests.cs

* Update Setup.cs

* update

* update

* Update SplashScreen.xaml.cs

* Update PublishPackageViewModelTests.cs

* Update PublishPackageViewModelTests.cs

* update

* Update AssemblyInfo.cs

* Update TestUtilities.cs

* update

* upate

* update

* update

* update

* Update AnalyticsTests.cs

* Update WebView2Utilities.cs

* update

* update

* Update Setup.cs

* update

* update

* Update PeriodicEvaluationTests.cs

---------

Co-authored-by: pinzart <[email protected]>
QilongTang added a commit that referenced this pull request Jan 27, 2024
* update

* Update NodeModelAssemblyLoader.cs

* update

* update

* update

* update

* Update AnalyticsTests.cs

* Update SerializationTests.cs

* Update Setup.cs

* update

* update

* Update SplashScreen.xaml.cs

* Update PublishPackageViewModelTests.cs

* Update PublishPackageViewModelTests.cs

* update

* Update AssemblyInfo.cs

* Update TestUtilities.cs

* update

* upate

* update

* update

* update

* Update AnalyticsTests.cs

* Update WebView2Utilities.cs

* update

* update

* Update Setup.cs

* update

* update

* Update PeriodicEvaluationTests.cs

---------

Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: pinzart <[email protected]>
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.

5 participants