Refactor tests by hassan20990 · Pull Request #7702 · microsoft/PTVS

hassan20990 · 2023-08-23T12:57:05Z

No description provided.

sonarqubecloud · 2023-08-23T12:59:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

rchiodo · 2026-07-07T17:05:30Z

🔒 Automated review in progress — @rchiodo is auto-reviewing this PR.

Copilot

Pull request overview

This PR refactors parts of the UI test utilities to make Solution Explorer interactions and project loading more robust, primarily by adjusting solution-node identification, adding UI-thread helpers for IVs* interactions, and hardening TreeView node-name comparisons.

Changes:

Simplifies how the solution root node text is computed for Solution Explorer interactions.
Updates VisualStudioApp.OpenProject to route more IVs* API calls through new UI-thread helper methods.
Sanitizes AutomationElement.NameProperty values during Solution Explorer tree traversal to handle non-printable characters.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
Common/Tests/Utilities.UI/UI/VisualStudioInstance.cs	Changes solution root node label construction used for Solution Explorer lookups.
Common/Tests/Utilities.UI/UI/VisualStudioApp.cs	Refactors project/solution open flow and adds UI-thread helper methods for IVs* calls.
Common/Tests/Utilities.UI/UI/TreeView.cs	Sanitizes node names (regex) before comparing path segments.
Common/Tests/Utilities.UI/TestUtilities.UI.csproj	Adds a project reference to `VSCommon`.
.vscode/settings.json	Adds a VS Code workspace setting for a specific extension.

+                var solutionName = Path.GetFileNameWithoutExtension(_solution.Filename);
+                return $"Solution '{solutionName}'";


+                var name = (node.GetCurrentPropertyValue(AutomationElement.NameProperty) as string);
+
+                // Sometimes AutomationElement.NameProperty contains non-printable characters that mess up the
+                // string compare, so get rid of those.
+                // See https://stackoverflow.com/questions/40564692/c-sharp-regex-to-remove-non-printable-characters-and-control-characters-in-a
+                name = Regex.Replace(name, @"\p{C}+", string.Empty).Trim();


 using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Microsoft.VisualStudio.Threading;
+using Microsoft.VisualStudioTools;
 using IOleServiceProvider = Microsoft.VisualStudio.OLE.Interop.IServiceProvider;


+                t = CreateTaskOnUIThread(async () =>
+                {
                    ErrorHandler.ThrowOnFailure(solution.OpenSolutionFile((uint)0, fullPath));
                });


+                t = CreateTaskOnUIThread(async () =>
+                {
                    Guid guidNull = Guid.Empty;
                    Guid iidUnknown = Guid.Empty;
                    IntPtr projPtr;


            object o;
            ErrorHandler.ThrowOnFailure(vsProject.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID.VSHPROPID_ExtObject, out o));
            var project = (Project)o;


+{
+    "marquee.widgets.npm-stats.packageNames": [
+        "ptvs"
+    ]
+}


bschnurr · 2026-07-07T17:11:57Z

@copilot resolve the merge conflicts in this pull request

rchiodo · 2026-07-07T17:13:38Z

-                        while (!t.Wait(1000, cts.Token)) {
+                        while (!t.Wait(1000, cts.Token))
+                        {
                            ErrorHandler.ThrowOnFailure(uiShell.GetDialogOwnerHwnd(out hwnd));


The threading fix is incomplete: the new comment states "everything from the IVs* APIs need to run on the UI thread," but IVsUIShell.GetDialogOwnerHwnd here — and vsProject.GetProperty at ~1057 — still run on the calling background thread while their neighbors (GetSolutionInfo, EnumerateLoadedProjects, GetGuidOfProject, ReloadProject) were wrapped in RunOnUIThread. Off-thread IVs* calls can throw RPC_E_WRONG_THREAD or silently COM-marshal. Either wrap these in RunOnUIThread too, or narrow the "everything" wording to match reality.

rchiodo · 2026-07-07T17:13:40Z

+            {
+                try
+                {
+                    if (!t.Wait(1000, cts.Token))


CreateTaskOnUIThread returns the raw .Task from a JoinableTask, and the caller blocks with t.Wait(1000, cts.Token). Extracting and blocking on JoinableTask.Task bypasses JTF's joinable-collection deadlock mitigation. It's safe today only because OpenProject runs off the UI thread and the lambda bodies are synchronous; if OpenProject is ever invoked on the UI thread, the scheduled main-thread continuation can't run while Wait() blocks that thread, deadlocking until the 30s CTS fires. Prefer returning the JoinableTask and driving the dialog-poll loop via Join()/JoinAsync.

rchiodo · 2026-07-07T17:13:43Z

+    "marquee.widgets.npm-stats.packageNames": [
+        "ptvs"
+    ]
+}


This file appears to be unrelated scope creep in a "Refactor tests" PR — it only sets a personal Marquee npm-stats widget config and is unrelated to test reliability. Consider dropping it from the change. (It is also missing a trailing newline.)

rchiodo · 2026-07-07T17:13:46Z

+            else
+            {
+                t = CreateTaskOnUIThread(async () =>
+                {


These async () => { ... } lambdas contain no await, producing CS1998 ("runs synchronously"). Harmless at runtime since the body executes on the UI thread after SwitchToMainThreadAsync() inside the helper, but if the harness project sets TreatWarningsAsErrors the build breaks. Drop the async keyword (return Task.CompletedTask) to silence the warning.

rchiodo · 2026-07-07T17:13:49Z

Solid test-reliability refactor. A few non-blocking notes: the UI-thread marshaling is applied inconsistently (a couple of IVs* calls remain off-thread), the JTF .Task/Wait pattern is a latent deadlock footgun if OpenProject is ever called on the UI thread, and the .vscode/settings.json addition looks unrelated to this PR.

rchiodo

Approved via Review Center.

heejaechang

Approved via Review Center.

rchiodo · 2026-07-10T15:12:41Z

-
+                var solutionName = Path.GetFileNameWithoutExtension(_solution.Filename);
+                return $"Solution '{solutionName}'";
            }


Collapsing SolutionNodeText to $"Solution '{solutionName}'" drops the (N of N projects) suffix and no longer consults _solution.Projects. This may be correct if a newer VS version removed the count from the solution root node, but it's an unexplained behavioral change. A one-line comment citing the VS-version behavior that motivated it would help; any consumer/FindNode match still expecting the old count text would silently mismatch.

rchiodo · 2026-07-10T15:12:44Z

Overall this looks fine to merge. A couple of non-blocking notes: the UI-thread wrapping is applied inconsistently across the IVs* calls, and there's an unrelated .vscode/settings.json change that should probably be dropped from a test-refactor PR.

rchiodo

Approved via Review Center.

AdamYoblick and others added 2 commits June 16, 2022 15:06

Fix the BuildSolution integration test

e3c34c2

Create settings.json

f8cf469

hassan20990 requested a review from a team as a code owner August 23, 2023 12:57

bschnurr requested a review from Copilot July 7, 2026 17:04

Copilot started reviewing on behalf of bschnurr July 7, 2026 17:04 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

rchiodo reviewed Jul 7, 2026

View reviewed changes

rchiodo approved these changes Jul 7, 2026

View reviewed changes

heejaechang approved these changes Jul 8, 2026

View reviewed changes

rchiodo reviewed Jul 10, 2026

View reviewed changes

rchiodo approved these changes Jul 10, 2026

View reviewed changes

		var solutionName = Path.GetFileNameWithoutExtension(_solution.Filename);
		return $"Solution '{solutionName}'";

Uh oh!

Conversation

hassan20990 commented Aug 23, 2023

Uh oh!

sonarqubecloud Bot commented Aug 23, 2023

Uh oh!

rchiodo commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bschnurr commented Jul 7, 2026

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Jul 7, 2026

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

heejaechang left a comment

Choose a reason for hiding this comment

Uh oh!

rchiodo Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Jul 10, 2026

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rchiodo commented Jul 7, 2026 •

edited

Loading