Skip to content

Fix TestExplorer by selecting the grouping button in this form 'Proje…#8338

Open
bschnurr wants to merge 6 commits into
microsoft:mainfrom
bschnurr:add-dynamic-runtimesettings
Open

Fix TestExplorer by selecting the grouping button in this form 'Proje…#8338
bschnurr wants to merge 6 commits into
microsoft:mainfrom
bschnurr:add-dynamic-runtimesettings

Conversation

@bschnurr

@bschnurr bschnurr commented Nov 28, 2025

Copy link
Copy Markdown
Member

try to fix current integration test not running (I think because our build changed from PTVS-BUILD-DEV17 to PTVS-BUILD
converted test stage from azure devops to yml.

trying to convert our azurdevops task group to yml
PTVS-UI-TESTS
https://devdiv.visualstudio.com/DevDiv/_taskgroup/78997fbf-510d-4ae3-9982-edd03e5257b7

@bschnurr bschnurr requested a review from a team as a code owner November 28, 2025 00:11
@sonarqubecloud

Copy link
Copy Markdown

@bschnurr

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

_vs.RunTest(nameof(PythonToolsUITests.TestExplorerTests.DebugUnittestProject));
}

[Ignore]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

test debugging is flaky for some reason. disable for now

Comment thread azure-pipelines-integration-tests.yml Outdated
# All PTVS build artifacts come from this pipeline and are referenced like this:
# $(Pipeline.Workspace)\ComponentBuildUnderTest\<artifactName>\<filePath>
- pipeline: ComponentBuildUnderTest
source: PTVS-Build-Dev17

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

update to latest build name. trying to get this pipeline to run

@@ -0,0 +1,149 @@
parameters:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

converted from json export to yml

failOnStderr: false

- task: VSTest@2
displayName: '[MultiConfiguration] Run UI Test ${{ parameters.UITestNames }}'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The VSTest@2 is the task they provided to tag our tests as part of their global tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure yet.. I think they do support vstest but I think this is all going to change after i check it in

@bschnurr

bschnurr commented Dec 1, 2025

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@bschnurr bschnurr enabled auto-merge (squash) July 7, 2026 17:03
@bschnurr

bschnurr commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

/azp run

@rchiodo

rchiodo commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@sonarqubecloud

sonarqubecloud Bot commented Jul 7, 2026

Copy link
Copy Markdown

{
var walker = TreeWalker.RawViewWalker;
menuItem = walker.GetParent(element);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Utilities.Python/PythonTestExplorer.cs:171
The Skeptic traced this against the UIAutomation contract and the Advocate defended it as intentional; the Skeptic is correct. element is obtained via FindFirst(...) with no active CacheRequest, so reading element.CachedParent throws InvalidOperationException ("requested value was not previously cached") — it does not return null. That means this line crashes on the normal success path (the moment the grouping option is found), and the if (menuItem == null) TreeWalker fallback below is dead code that never executes. The inline comment "CachedParent may be null depending on cache policy" is factually wrong. Fix: drop CachedParent and use TreeWalker.RawViewWalker.GetParent(element) directly.

[verified]

//menuItem.GetInvokePattern().Invoke();
if (menuItem != null)
{
var inv = menuItem.GetInvokePattern() ?? element.GetInvokePattern();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Utilities.Python/PythonTestExplorer.cs:180
Same root misunderstanding as the CachedParent bug, confirmed by source inspection: GetInvokePattern()Pattern<InvokePattern>() (AutomationWrapper.cs:507–515) catches InvalidOperationException and re-throws (throw; at line 513). It can never return null, so menuItem.GetInvokePattern() ?? element.GetInvokePattern() has a dead right-hand side — when menuItem (a container parent) doesn't support InvokePattern, this throws out of the method instead of falling back to the element. Use TryGetCurrentPattern(InvokePattern.Pattern, out var p) on each candidate and invoke whichever supports it.

[verified]

inv.Invoke();
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Utilities.Python/PythonTestExplorer.cs:159
All four reviewers flagged this: when the 5×200ms retry loop leaves element == null (popup not rendered, non-en-US VS, or a VS version with different automation names), the if (element != null) block is skipped and the method proceeds to WaitForTestsGrid() as if it succeeded. Grouping may never have been applied, and the failure surfaces later as a confusing WaitForItem "Could not find …" miss rather than "grouping failed." The method name promises a state it doesn't guarantee. Add Assert.IsNotNull(element, "Could not find 'Project, Namespace, Class' grouping option") (or throw) so failures are diagnosable.

[verified]


WaitForTestsGrid();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Utilities.Python/PythonTestExplorer.cs:137
The Architect (verified against the diff), Skeptic, and Advocate all note this file was reformatted from K&R braces (get {, if (...) {) to Allman (get\n{), plus a using reorder, across every method — while the rest of this PR's C# (Core.UI/TestExplorerTests.cs, PythonToolsUITestsRunner/TestExplorerTests.cs) keeps K&R. The only real behavioral change is in GroupByProjectNamespaceClass, but the whole-file reformat inflates the diff, hides the two bugs above, and leaves this file inconsistent with the codebase. Revert the style-only churn so the diff reflects just the logic change.

[verified]

app.OpenProject(sln);

RunAllTests(app, AllUnittests);
RunAllTests(app, AllUnittests, Path.GetFileNameWithoutExtension(sln));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Core.UI/TestExplorerTests.cs:65
The Skeptic and Architect note that Path.GetFileNameWithoutExtension(sln) assumes the Test Explorer "Project" grouping node label equals the .sln filename. Under "Project, Namespace, Class" grouping the root node is the project name, not the solution name; this only works because TestExplorerUnittest.sln and its project share a base name. [unverified] — the Advocate could not confirm the copied project's root label on a live run. Add a comment documenting the assumption, or derive the root from the project name so it survives a sln/project name divergence.

[verified]

app.OpenProject(sln);

RunAllTests(app, AllUnittests);
RunAllTests(app, AllUnittests, Path.GetFileNameWithoutExtension(sln));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/Core.UI/TestExplorerTests.cs:65
Asymmetric fix (Advocate, Skeptic, Architect all verified): RunAllUnittestProject now passes the sln-name projectRootOverride, but RunAllPytestProject (and DebugPytestProject) still pass no override. If "project grouping shows the project name as root" is the general fix, the pytest project paths should need the same override; if pytest tests legitimately lay out flat under the module, that difference deserves a one-line comment so a future maintainer doesn't "fix" pytest into breakage. Reconcile or document.

[verified]

_vs.RunTest(nameof(PythonToolsUITests.TestExplorerTests.DebugUnittestProject));
}

[Ignore]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Python/Tests/PythonToolsUITestsRunner/TestExplorerTests.cs:78
The two [Ignore] attributes on DebugUnittestProject/DebugUnittestWorkspace drop P0 coverage with no rationale or tracking link (Architect: Medium; Skeptic). Note also that DebugUnittestProject still calls DebugTest without the projectRootOverride that RunAllUnittestProject now requires — so if these are un-ignored they'll hit the same wrong-root path and fail. Add a // reason + issue URL comment so the disablement is intentional and re-enablement is trackable, and state whether it's ignored because of this grouping change.

[verified]

$running = Get-Process -Name $names -ErrorAction SilentlyContinue
}
errorActionPreference: stop
failOnStderr: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📍 Build/azuredevops/test/uitest-integration-stage.yml:130
The terminal Cleanup step uses condition: succeeded() with failOnStderr: true (Skeptic, Architect, Advocate). Two problems: (1) if any earlier step fails (e.g. reg import on a missing .reg, a VSTest infra error), succeeded() skips this teardown and leftover devenv/vshub/python/vstest processes wedge the self-hosted agent for the next run; (2) $running | Stop-Process -Force without -ErrorAction Stop emits a non-terminating error (access-denied / already-exiting) that the surrounding try/catch does not trap — it hits stderr and failOnStderr: true turns a passing run red. Use condition: always() (or succeededOrFailed()) and add -ErrorAction SilentlyContinue to Stop-Process (or set failOnStderr: false for cleanup).

[verified]

@rchiodo

rchiodo commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

The core GroupByProjectNamespaceClass change has two real bugs: both element.CachedParent and menuItem.GetInvokePattern() ?? ... rely on those APIs returning null, but with no active CacheRequest / by AutomationWrapper's implementation they throw instead — so the helper crashes on the success path and the fallbacks are dead code. Please also reconsider the whole-file K&R→Allman reformat, which inflates the diff and diverges from the rest of the PR's C# style.

@bschnurr bschnurr disabled auto-merge July 7, 2026 20:35
@bschnurr bschnurr enabled auto-merge (squash) July 7, 2026 20:36
@rchiodo

rchiodo commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

The main behavioral change in GroupByProjectNamespaceClass has two correctness bugs rooted in the same misunderstanding of the UIAutomation API: CachedParent and GetInvokePattern() throw InvalidOperationException rather than returning null, so the TreeWalker/?? element.GetInvokePattern() fallbacks are dead code and the method crashes on its normal success path. Please fix those first. Separately, the whole-file K&R→Allman reformat inflates the diff and hides the real change — consider reverting the style-only churn.

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.

3 participants