stabilize tests and update libg #15736

pinzart90 · 2024-12-21T00:06:05Z

Purpose

Update libg (more debug logs)
Stabilize tests

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

(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

github-actions · 2024-12-21T00:25:48Z

UI Smoke Tests

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

mjkkirschner · 2025-01-02T14:54:12Z

test/DynamoCoreTests/Logging/AnalyticsServiceTest.cs

-                    var firstASMmodulePath = string.Empty;
-                    foreach (string module in assemblies)
+                var firstASMmodulePath = string.Empty;
+                foreach (ProcessModule module in dynamoCLI.Modules)


I think these are IDisposable.

mjkkirschner · 2025-01-02T14:54:48Z

test/DynamoCoreTests/Logging/AnalyticsServiceTest.cs

+                var firstASMmodulePath = string.Empty;
+                foreach (ProcessModule module in dynamoCLI.Modules)
+                {
+                    if (module.ModuleName.IndexOf("Analytics.dll", StringComparison.OrdinalIgnoreCase) != -1)


Are all of these modules managed modules? Will this property return both managed and unmanaged?

both, managed and unmanaged

mjkkirschner · 2025-01-02T14:56:45Z

test/Libraries/WorkflowTests/DynamoSamples.cs

-            Process[] procs = Process.GetProcessesByName("excel");
-            foreach (Process proc in procs)
-                proc.Kill();
+            foreach (var process in Process.GetProcessesByName("EXCEL"))


is it better to use your new method for this, or that requires having the windows instance of excel?

yeah, we don't have the handle here. We do however identify the process by the title.

aparajit-pratap · 2025-01-06T17:17:28Z

src/DynamoCoreWpf/Utilities/WebView2Utilities.cs

+            if (DynamoModel.IsTestMode && !string.IsNullOrEmpty(CreationProperties?.UserDataFolder))
+            {
+                var newPath = Path.Combine(Directory.GetParent(Assembly.GetExecutingAssembly().Location).FullName, "Webview2Data", $"{Environment.ProcessId}", tag);
+                if (Directory.Exists(newPath))


You mean if directory does not exist?

I removed this part. Will try it out in another PR

aparajit-pratap · 2025-01-06T18:26:02Z

test/DynamoCoreWpfTests/SplashScreenTests.cs

@@ -131,7 +131,6 @@ void WindowClosed(object sender, EventArgs e)

            ss.Closed -= WindowClosed;

-            Assert.IsNull(ss.webView);// Make sure webview2 was disposed


I found that waiting for the ss.webView object to be disposed is not reliable and not critical for this test.
This is an intentional change

aparajit-pratap · 2025-01-06T18:34:26Z

test/Libraries/WorkflowTests/DynamoSamples.cs

-                proc.Kill();
+            foreach (var process in Process.GetProcessesByName("EXCEL"))
+            {
+                if (process.MainWindowTitle.Equals("ImportExport_Data To Excel - Excel"))


What process is it that does not have this title? IOW, what other titles are there? Is this title specific to Dynamo or the tests?

The titles are specific to each dyn. If a test runs a dyn that reads/writes an excel file name "test1", then the excel window will have the title "test1 - Excel". This title is set by the excel interop API (AFAIK)

mjkkirschner · 2025-01-15T22:52:59Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

-                Service.ShutDown();
+            {
+                // Lock shutdown sequence in case other tracking calls might be executing concurently.
+                lock(trackEventLockObj)


so I might be wrong - but in all other cases this lock is held from a worker thread on the thread pool, not from main thread... could this cause a dead lock?

From what I could tell from the logs Looks like there is concurrent access to an analytics resource (a dictionary). The dictionary is changed while it is iterated over in another thread. The Shutdown call is the only one that is not thread safe

mjkkirschner · 2025-01-15T22:53:53Z

src/Libraries/DSOfficeUtilities/Excel.cs

@@ -105,7 +105,7 @@ public static bool ExcelProcessRunning
        {
            get
            {
-                return Process.GetProcessesByName("EXCEL").Length != 0;
+                return GetExcelProcess(_app) != null;


could you briefly explain these changes?

With my change we can check if the Excel process that is associated with the COM object "_app" is alive.
Previously we were checking if any process called "EXCEL" was running. It seemed that could get into situations where multiple Excel processes could get mixed up

mjkkirschner · 2025-01-15T22:54:03Z

test/DynamoCoreTests/DynamoCoreTests.csproj

@@ -139,18 +138,10 @@
  <ItemGroup>
    <Folder Include="NodesJsonDatasets\" />
  </ItemGroup>
-<Target Name="CopyDisableADPTestDeps">


mjkkirschner · 2025-01-15T22:54:56Z

test/DynamoCoreTests/DynamoModelTestBase.cs

@@ -154,10 +154,10 @@ protected void OpenModelInManualMode(string relativeFilePath)
            CurrentDynamoModel.ExecuteCommand(new DynamoModel.OpenFileCommand(openPath, true));
        }

-        protected void OpenSampleModel(string relativeFilePath)
+        protected void OpenSampleModel(string relativeFilePath, bool forceManualExecution = false)


I actually think this is a binary API break for clients who derive from this class for their tests, which is public, I think you need to add a new overload.

You're right, it would break ABI compatibility. Nice catch!

mjkkirschner · 2025-01-15T22:58:45Z

test/Libraries/WorkflowTests/DynamoSamples.cs

-            //Todo Ritesh: Locally passing but failing on CI.
-            //After fixing issue with this test case add Smoke Test Category.
+        //Todo Ritesh: Locally passing but failing on CI.
+        //After fixing issue with this test case add Smoke Test Category.
        public void ImportExport_Excel_to_Dynamo()


does this pass now?

Fails on the master-15 machines.

mjkkirschner

one change request WRT to API - rest was just comments you can accept or ignore.

update libg

6a69090

pinzart90 added 11 commits December 20, 2024 21:15

stabilize test

6482197

Update DispatcherUtil.cs

cbcec81

update

e2eae53

Merge branch 'master' into update_libg231

e02dfd2

update

a780cb2

Update WebView2Utilities.cs

6eb5013

update

58c78fa

Update PreviewBubbleTests.cs

7e45d93

update

a83946e

Update PreviewBubbleTests.cs

213a64c

Update NotificationsExtensionTests.cs

22f1ec2

mjkkirschner reviewed Jan 2, 2025

View reviewed changes

Update AnalyticsServiceTest.cs

28606cd

pinzart90 changed the title ~~update libg~~ stabilize tests and update libg Jan 6, 2025

QilongTang added this to the 3.5 milestone Jan 6, 2025

aparajit-pratap reviewed Jan 6, 2025

View reviewed changes

pinzart90 added 7 commits January 13, 2025 11:39

update

c43bb99

update

1f0d4c6

Merge branch 'master' into update_libg231

ae45d8d

Update DynamoSamples.cs

ae391dc

Update NotificationsExtensionTests.cs

f1dc508

Update DynamoAnalyticsClient.cs

fb776aa

Merge branch 'master' into update_libg231

b591f88

pinzart90 requested a review from mjkkirschner January 15, 2025 20:42

pinzart90 requested a review from aparajit-pratap January 15, 2025 20:42

mjkkirschner reviewed Jan 15, 2025

View reviewed changes

mjkkirschner requested changes Jan 15, 2025

View reviewed changes

update

8824bcb

pinzart90 requested a review from mjkkirschner January 16, 2025 15:10

mjkkirschner approved these changes Jan 16, 2025

View reviewed changes

pinzart90 merged commit 0b11829 into master Jan 16, 2025
23 of 24 checks passed

pinzart90 deleted the update_libg231 branch January 16, 2025 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stabilize tests and update libg #15736

stabilize tests and update libg #15736

pinzart90 commented Dec 21, 2024 •

edited

Loading

github-actions bot commented Dec 21, 2024 •

edited

Loading

mjkkirschner Jan 2, 2025

mjkkirschner Jan 2, 2025

pinzart90 Jan 14, 2025

mjkkirschner Jan 2, 2025

pinzart90 Jan 6, 2025

aparajit-pratap Jan 6, 2025

pinzart90 Jan 14, 2025

aparajit-pratap Jan 6, 2025

pinzart90 Jan 14, 2025

aparajit-pratap Jan 6, 2025

pinzart90 Jan 14, 2025

mjkkirschner Jan 15, 2025

pinzart90 Jan 16, 2025 •

edited

Loading

mjkkirschner Jan 15, 2025

pinzart90 Jan 16, 2025

mjkkirschner Jan 15, 2025

mjkkirschner Jan 15, 2025

pinzart90 Jan 16, 2025

mjkkirschner Jan 15, 2025

pinzart90 Jan 16, 2025

mjkkirschner left a comment

		@@ -131,7 +131,6 @@ void WindowClosed(object sender, EventArgs e)

		ss.Closed -= WindowClosed;

		Assert.IsNull(ss.webView);// Make sure webview2 was disposed

stabilize tests and update libg #15736

stabilize tests and update libg #15736

Conversation

pinzart90 commented Dec 21, 2024 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

github-actions bot commented Dec 21, 2024 • edited Loading

UI Smoke Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

pinzart90 commented Dec 21, 2024 •

edited

Loading

github-actions bot commented Dec 21, 2024 •

edited

Loading

pinzart90 Jan 16, 2025 •

edited

Loading