[Testing] [macOS] Fix screenshot masking and scaling for Retina (high-DPI) displays - #35156
[Testing] [macOS] Fix screenshot masking and scaling for Retina (high-DPI) displays#35156NafeelaNazhir wants to merge 18 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35156Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35156" |
NafeelaNazhir
added
community ✨
dotnet-policy-service
Bot
added
the
partner/syncfusion
|
/azp run maui-pr-uitests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
added
s/agent-changes-requested
54dd6fa to
0a144f4
Compare
The A/B gate reverts committed fix files to build the without-fix baseline, so it first refuses to run if any revertable fix file has uncommitted changes (to avoid clobbering unstaged work). That guard used a plain 'git status --porcelain', which reports an executable-bit-only change (100644->100755) as ' M' — even though the file CONTENT is fully committed and reverts cleanly via 'git checkout HEAD'. On mac agents a setup step chmod +x's committed shell scripts (eng/scripts/*.sh), tripping the guard and aborting all 3 retries with 'Uncommitted changes detected in fix files' -> a false INCONCLUSIVE (observed build 14699093, #35156 catalyst: disable-/enable-notification- center.sh flagged mode-only). Use 'git -c core.fileMode=false status --porcelain' so an exec-bit-only diff is ignored while a genuine content change is STILL caught (verified locally: mode-only -> clean, content change -> flagged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
…s false INCONCLUSIVE on infra-only PRs) The gate overlays TRUSTED review-branch copies of .github/scripts, .github/skills and eng/scripts over the worktree for security (Review-PR.ps1 Restore-TrustedScripts). A PR that itself modifies a file under those paths therefore ALWAYS shows it as an uncommitted worktree change (trusted content != the PR's committed content), so the uncommitted-fix-files guard aborted with a misleading 'Uncommitted changes detected / run git add && commit' error that the caller treats as a missing-report infra failure and retries 3x before a bare INCONCLUSIVE (build 14699515, #35156 catalyst: eng/scripts/{disable,enable}-notification-center.sh). Those files are also force-restored to the SAME trusted version in both the without-fix and with-fix runs, so reverting them changes nothing - they are not A/B-testable. The same holds for pipeline/workflow definitions (eng/pipelines, .github/workflows): the gate runs on an already-checked-out pipeline, so editing those YAMLs in the worktree cannot alter the gate's own execution. Exclude all such paths from the fix-file set BEFORE the uncommitted guard. If real product/test fix files remain, the A/B runs on those; if none remain, the change is CI-infra-only and the gate emits a deterministic, non-retried INCONCLUSIVE (exit 3 + report without the ENV ERROR token) that defers to the Deep UI Tests stage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| // App.Screenshot() returns pixels at the display's native resolution | ||
| // (1x on non-Retina, 2x on Retina displays). | ||
| // Strategy: Compare screenshot pixel dimensions to screen logical dimensions. | ||
| var screenSize = ((AppiumApp)App).Driver.Manage().Window.Size; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[critical] Logic and Correctness — Driver.Manage().Window.Size is the current app window size, while App.Screenshot() is the full desktop screenshot on Mac. For a non-fullscreen Retina window, dividing full-screen pixels by app-window points can infer 3x/4x instead of the display scale, shifting the mask/crop to the wrong pixels. Use an actual display scale/screen logical size source, or derive the scale from matching screenshot/display dimensions rather than the app window.
| @@ -16,7 +16,7 @@ public Issue33769(TestDevice device) : base(device) | |||
| [Category(UITestCategories.Stepper)] | |||
| public void ValidateStepperReachesMinMax() | |||
| { | |||
| App.WaitForElement("Issue33769_StepperStatusLabel"); | |||
| App.WaitForElementTillPageNavigationSettled("Issue33769_StepperStatusLabel"); | |||
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Regression Prevention and Test Coverage — This added wait only settles the initial page load; the decrement half can still pass on stale state. After IncreaseStepper sets the shared status label to Success, a failed/no-op DecreaseStepper leaves the same text in place, so the second assertion does not prove the Stepper reached minimum. Reset the status/value before decrementing, or assert a distinct min-state/value after the decrement.
MauiBot
removed
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
| // App.Screenshot() returns pixels at the display's native resolution | ||
| // (1x on non-Retina, 2x on Retina displays). | ||
| // Strategy: Compare screenshot pixel dimensions to screen logical dimensions. | ||
| var screenSize = ((AppiumApp)App).Driver.Manage().Window.Size; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Logic and Correctness — Driver.Manage().Window.Size is the Appium app-window size, but App.Screenshot() on Mac captures the whole display. The ratio image.Width / screenSize.Width is only the display scale when the app window fills the screen; most screenshot tests do not call EnterFullScreen(). For a normal 800pt-wide window on a 3024px Retina display this computes about 3.8 and rounds to 4 instead of the real 2, so the subsequent mask/composite uses the wrong pxX/pxWidth and produces an incorrectly cropped screenshot. Use the actual display logical bounds/backing scale (or another native display-scale source) rather than the current app window size.
| @@ -33,7 +36,7 @@ public void Issue33356NavigateShouldOccur() | |||
| App.TapBackArrow(Device == TestDevice.Android ? "" : "Cats"); | |||
| } | |||
| App.WaitForElement("Issue33356CatsCollectionView"); | |||
| App.WaitForElement("Abyssinian"); | |||
| App.WaitForElementTillPageNavigationSettled("Abyssinian"); | |||
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Regression Prevention — WaitForElementTillPageNavigationSettled(string) first performs a Catalyst-only WaitForElement(AppiumQuery.ById(elementId)) before falling back to the normal string wait semantics. Abyssinian is item text, not an AutomationId, so on Mac Catalyst this can time out before App.WaitForElement("Abyssinian") gets its text fallback. The same pattern was added for text/native labels such as DropCompleted and More; keep this helper to AutomationId-backed waits or make it use the same id-or-text lookup as WaitForElement(string).
| // so match by identifier instead of relying on FirstOrDefault(). | ||
| var decreaseButton = _appiumApp is AppiumCatalystApp | ||
| ? buttons.FirstOrDefault(b => | ||
| b.GetAttribute<string>("identifier")?.EndsWith("-Decrement", StringComparison.OrdinalIgnoreCase) == true) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Logic and Correctness / Regression Prevention — If Catalyst reports a decrement button whose identifier does not end with -Decrement, this lookup returns null and decreaseButton?.Tap() silently no-ops while Decrease still returns success. Issue33769 already has Success in the status label after the increment assertion, so a missed decrement tap can make the second assertion pass without exercising the minimum-value path. Fail loudly (for example, throw when the Catalyst-specific button lookup is missing, since the helper ignores CommandResponse) instead of treating a missing button as a successful command.
MauiBot
removed
the
s/agent-review-in-progress
|
/azp run maui-pr-uitests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Tests Failure Analysis
Test Failure Review: Not ready - click to expandOverall verdict: Not ready The PR is not CI-ready: 9 distinct failures are deterministic regressions where the same legs were green on all 5 sampled base builds and red on none. Only 2 of 33 distinct failures also appear on the base branch, and those two are still indeterminate because the gathered reason comparison refused a clean pre-existing dismissal. Coverage: 153 checks · 146 passing · 7 failing · 0 pending · 0 inaccessible · 1 unmapped · 11 unexplained build legs · 0 unaccounted failing checks · 0 aborted failing checks · 0 canceled-build checks · 0 device-test unverified · 24 unattributed · 9 regressed-vs-base. Deterministic ceiling: Not ready — reasons: Build Analysis is unmapped; 11 failed build legs produced no extractable failure; 24 failures are unattributed; 9 failures regressed vs base.
Recommended actionInvestigate and fix the macOS UI-test regressions first, especially the TitleBar visual mismatches and Stepper/TableView assertion/null-reference failures that are red on this PR but green across the sampled base builds; separately inspect the 11 unexplained failed legs and the unmapped Build Analysis check before relying on any rerun. Evidence details
Visual failure comparisonsFull-resolution CI baseline, actual, and diff images are embedded below. They supplement the failure classification and do not change the deterministic verdict ceiling.
|
| CI baseline | Fresh PR actual | CI diff |
|---|---|---|
 |
 |
 |
ButtonsLayoutResolveWhenParentSizeChanges - android - Needs human investigation - visual comparison
CI reported 1.85% difference in build 1517753.
Relationship to PR: Needs human investigation - No decisive exact test-and-platform baseline attribution was available.
| CI baseline | Fresh PR actual | CI diff |
|---|---|---|
 |
 |
 |
|
/azp run maui-pr-uitests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@NafeelaNazhir — new AI review results are available based on this last commit:
b4573f4. To request a fresh review after new comments or commits, comment/review rerun.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate: Test Verification
Result:
Platform: CATALYST
This PR only changes CI infrastructure the gate force-restores to trusted versions or cannot toggle at run time:
eng/pipelines/ci-uitests.ymleng/scripts/disable-notification-center.sheng/scripts/enable-notification-center.sh
These paths are identical (trusted) in both the without-fix and with-fix runs, so the gate cannot construct a without-fix baseline and the change is not A/B-verifiable here. Its behaviour is validated end-to-end by the Deep UI Tests stage.
📱 UI Tests — Button,CarouselView,CollectionView,DisplayAlert,DragAndDrop,Entry,FlyoutPage,GraphicsView,ListView,Navigation,Picker,Shell,Stepper,TabbedPage,TableView,TitleView,ViewBaseTests,Window
Detected UI test categories: Button,CarouselView,CollectionView,DisplayAlert,DragAndDrop,Entry,FlyoutPage,GraphicsView,ListView,Navigation,Picker,Shell,Stepper,TabbedPage,TableView,TitleView,ViewBaseTests,Window
❌ Deep UI tests — 807 passed, 4 failed across 16 categories on platform-pool agent (replaces in-process counts above). The deep UI run for 2 categories (CollectionView, Shell) exceeded the per-category time budget (a very long-running category or a slow build/deploy) and was stopped before finishing. This is an infrastructure/timeout issue, not a code problem; re-run the review, and if a category consistently needs more time the per-category budget can be raised (DEEP_UITEST_CATEGORY_CAP_MIN / DEEP_UITEST_HARDSTOP_MIN). See the build-output.log in the drop-deep-uitests artifact.
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
69/70 ✓ | — |
CarouselView |
82/83 ✓ | — |
DisplayAlert |
8/8 ✓ | — |
DragAndDrop |
1/4 (3 ❌) | — |
Entry |
103/104 ✓ | — |
FlyoutPage |
57/57 ✓ | — |
GraphicsView |
45/45 ✓ | — |
ListView |
92/93 ✓ | — |
Navigation |
85/86 ✓ | — |
Picker |
35/35 ✓ | — |
Stepper |
29/29 ✓ | — |
TabbedPage |
43/43 ✓ | — |
TableView |
16/16 ✓ | — |
TitleView |
29/29 ✓ | — |
ViewBaseTests |
101/101 ✓ | — |
Window |
12/13 (1 ❌) | — |
🔍 AI analysis of failures — PR-related vs unrelated
🔍 AI-generated triage (GitHub Copilot CLI) — a heuristic judgement of whether each deep UI test failure is connected to this PR's changes. Verify before relying on it.
Likely unrelated: the failures appear pre-existing, flaky, or infrastructure.
- ● Unrelated — Drag-and-drop UI navigation/setup timeouts (3 tests): the failures all timed out waiting for expected elements before/around the scenario (
DragAndDropWithAnElementThatIsRemovedis representative), which matches a common Appium/Catalyst flakiness pattern and there is no available PR diff tying the changes to drag-and-drop behavior. - ● Unrelated — Catalyst title-bar visual baseline mismatch (1 test): the failure is a snapshot-vs-baseline delta for
TitleBarWithLargeHeightShell, which is most consistent with a baseline/environment rendering mismatch absent evidence that this PR changed Catalyst title-bar rendering or snapshots.
Strongest signal: every failure matches a generic timeout or visual-baseline pattern, and the PR changed files/diff were unavailable to establish a PR-specific connection.
❌ DragAndDrop — 3 failed tests
DragAndDropWithAnElementThatIsRemoved
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2757
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2784
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, IQuery query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 925
at UITest.Appium.HelperExtensions.WaitForElementTillPageNavigationSettled(IApp app, String elementId, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2408
at Microsoft.Maui.TestCases.Tests.Issues.Issue28416.DragAndDropWithAnElementThatIsRemoved
...
AcceptedOperationNoneDisablesDropOperation
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2757
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2784
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 793
at Microsoft.Maui.TestCases.Tests.Issues.Issue12060.AcceptedOperationNoneDisablesDropOperation() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue12060.cs:line 24
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at Sy
...
DropCompletedFiresWhenDroppingOnNonDropTarget
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2757
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2784
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, IQuery query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 925
at UITest.Appium.HelperExtensions.WaitForElementTillPageNavigationSettled(IApp app, String elementId, Nullable`1 timeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2408
at Microsoft.Maui.TestCases.Tests.Issues.Issue17554.DropCompletedFiresWhenDroppingOnNonDr
...
❌ Window — 1 failed test
TitleBarWithLargeHeightShell
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: TitleBarWithLargeHeightShell_LargeHeightRequest.png (2.28% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at Microsoft.Maui.TestCases.Tests.Issues.Issue24489_Shell.TitleBarWithLargeHeightShell() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue24489_Shell.cs:line 77
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)
📋 Pre-Flight — Context & Validation
Issue: N/A - No linked issue found in PR body
PR: #35156 - [Testing] [macOS] Fix screenshot masking and scaling for Retina (high-DPI) displays
Platforms Affected: MacCatalyst / macOS UI tests
Files Changed: 6 implementation/config/test-source files, 1 pipeline file, many Mac snapshot baselines
Key Findings
- PR changes shared MacCatalyst UI-test screenshot infrastructure in
src/Controls/tests/TestCases.Shared.Tests/UITest.cs, macOS notification suppression scripts, HostApp/UI-test stabilization code, and regenerated Mac snapshot baselines. - The core PR fix derives screenshot scale from full-screen screenshot pixel dimensions divided by Appium app-window size; prior review and independent code review agree this is incorrect for non-fullscreen Retina windows.
- Catalyst navigation-settling waits were introduced broadly, but the string overload first waits by AutomationId only on Catalyst, breaking callers that intentionally pass visible text such as
DropCompletedandAbyssinian. - The Stepper regression test can still pass on stale
"Success"text after the increment phase, so it may not verify the decrement-to-minimum path. - Existing gate result is inconclusive because tests could not be built/run; it must not be treated as a PR-fix failure.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 3 | Warnings: 1 | Suggestions: 0
Key code review findings:
- ✗
src/Controls/tests/TestCases.Shared.Tests/UITest.cs:707-713computes Retina scale fromApp.Screenshot()full-desktop pixels divided byDriver.Manage().Window.Sizeapp-window points, which can inflate scale and crop the wrong region. - ✗
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue17554.cs:21andIssue33356.cs:39can time out on Catalyst becauseWaitForElementTillPageNavigationSettled(string)pre-waits withAppiumQuery.ById(...)before the normal id-or-text string lookup. - ✗
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33769.cs:25-28can pass after a no-op decrement because the label already says"Success"from the increment assertion. - ⚠
Issue8529.cs:33andXFIssue/Issue12320.cs:27use hard-coded Catalyst back-button coordinates.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35156 | Scale Mac screenshots by comparing full screenshot pixels to Driver.Manage().Window.Size, add Catalyst wait stabilization, update snapshots and macOS CI setup |
UITest.cs, UI tests, HostApp, pipeline/scripts, snapshots |
Original PR; code review found unresolved correctness issues |
🔬 Code Review — Deep Analysis
Code Review — PR #35156
Independent Assessment
What this changes: Updates MacCatalyst UI-test infrastructure: switches public macOS UITests to an ACES Tahoe pool, suppresses macOS dialogs/notifications, forces HostApp light theme, changes Mac screenshot masking/scaling for Retina, updates many UITests to use settled waits/retry assertions, changes Catalyst stepper/back-button workarounds, and regenerates Mac screenshot baselines.
Inferred motivation: Stabilize MacCatalyst UI tests on newer/high-DPI macOS agents and reduce flaky Appium/screenshot failures.
Reconciliation with PR Narrative
Author claims: The PR fixes Retina screenshot masking/scaling, moves macOS UI tests to the appropriate shared pool, suppresses interfering system UI, stabilizes UITests, and refreshes Mac baselines.
Agreement/disagreement: The code matches that intent, but the core screenshot scale calculation still uses the app window size as if it were the full screen logical size, which undermines the main PR goal. Several test wait conversions also change id-or-text lookup semantics on Catalyst.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
| Mac screenshot scale is computed from full-screen screenshot pixels divided by Appium app-window size | MauiBot inline reviews/top-level changes requested | ❌ Unresolved | Current src/Controls/tests/TestCases.Shared.Tests/UITest.cs:707-713 still uses Driver.Manage().Window.Size for image.Width / screenSize.Width. |
WaitForElementTillPageNavigationSettled(string) breaks text-only waits on Catalyst |
MauiBot inline reviews | ❌ Unresolved | Current Issue17554.cs:21 waits for text DropCompleted; current Issue33356.cs:39 waits for item text Abyssinian; helper first does ById(...) on Catalyst. |
Issue33769 decrement path can pass on stale Success text |
MauiBot inline reviews | ❌ Unresolved | Current Issue33769.cs:25-28 waits for the same "Success" label value that was already set by the increment phase. |
| Hard-coded Catalyst back-button tap coordinates | MauiBot/expert reviewer | Current Issue8529.cs:33 and Issue12320.cs:27 still tap (158, 67). |
|
| Catalyst stepper identifier-specific decrement lookup | MauiBot inline review | 🔄 Obsolete | Current code no longer uses the identifier-suffix lookup described by that comment; it sorts buttons by X position. |
Blast Radius Assessment
- Runs for all instances: Yes, for all MacCatalyst screenshot-based UI tests because
UITest.TakeScreenshot()is shared infrastructure. - Startup impact: Limited to TestCases HostApp/UI-test startup, not product startup.
- Static/shared state: No new product static state, but global macOS defaults/process-kill behavior affects the whole UI-test agent session.
CI Status
- Required-check result:
gh pr checks --requiredcould not run becauseghis unauthenticated. Public GitHub check-runs for headb4573f46581b3effbfca76cfed57f5d02ce160d0show failingBuild Analysisand aggregatemaui-pr-uitests, including multiple failed macOS UITest legs. - Classification: PR-caused or at least PR-relevant failure; the failures are in the MacCatalyst UITest area this PR changes.
- Action taken: Invoked
azdo-build-investigatorcontext; confidence capped low because required-check status could not be obtained viaghand fallback check-runs are red.
Findings
❌ Error — Retina screenshot scale still uses app-window size instead of screen size
src/Controls/tests/TestCases.Shared.Tests/UITest.cs:707-713
App.Screenshot() on Mac captures the full desktop, but Driver.Manage().Window.Size is the current app window size. For a non-fullscreen Retina desktop, image.Width / screenSize.Width can compute display-pixels/app-window-points (for example ~3x or ~4x) instead of the actual backing scale (usually 2x). The subsequent pxX, pxWidth, and mask composite then crop the wrong screen region. This is the central behavior the PR claims to fix, so it should not merge as-is.
❌ Error — New settled waits can time out on visible text-only elements on Catalyst
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue17554.cs:21
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33356.cs:39
WaitForElementTillPageNavigationSettled(string) does a Catalyst-only WaitForElement(AppiumQuery.ById(elementId)) before the normal string wait. The old WaitForElement(string) falls back from id to text; the new pre-wait does not. DropCompleted is status label text, not the StatusLabel automation id, and Abyssinian is collection item text, not an automation id. On Catalyst these can time out before the normal text fallback is reached.
❌ Error — Stepper min/max test can pass without validating decrement
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33769.cs:25-28
After the increment phase, the status label is already "Success". The decrement phase waits for the same "Success" value and then asserts the same text, so a failed/no-op DecreaseStepper can still pass. This weakens the test exactly where the PR is trying to improve stepper reliability.
⚠️ Warning — Catalyst back-button taps are hard-coded to absolute screen coordinates
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue8529.cs:33
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue12320.cs:27
App.TapCoordinates(158, 67) uses macos: click absolute coordinates. The back button location can vary with window placement, toolbar metrics, display scaling, or agent resolution. Prefer a relative tap based on the app/window/back-button frame or a platform navigation command.
Failure-Mode Probing
- Non-fullscreen MacCatalyst window on Retina display: screenshot scale can be derived from desktop pixels divided by app-window points, producing an inflated scale and wrong crop.
- Catalyst wait for visible text with no AutomationId: the new helper times out on
ById(text)before the existing text fallback can run. - Stepper decrement no-ops:
Issue33769still passes because the status label retained"Success"from the increment assertion. - Different CI display/window layout: absolute
(158, 67)back-button taps can miss the app/back button.
Verdict: NEEDS_CHANGES
Confidence: low, capped by red/undetermined required CI and shared UI-test infrastructure blast radius.
Summary: The PR addresses real MacCatalyst UITest problems, but the main screenshot scaling fix is still logically incorrect for full-screen screenshots of non-fullscreen app windows. There are also unresolved test reliability regressions from Catalyst wait semantics and stale success assertions.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | maui-expert-reviewer | Capture the XCUIElementTypeWindow element screenshot and mask in window-local coordinates |
UITest.cs |
Compiled; runtime blocked by MacCatalyst HostApp registration (com.microsoft.maui.uitests not found). Risk: Appium Mac may not support/shape window element screenshots consistently. |
|
| 2 | maui-expert-reviewer | Preserve id-or-visible-text semantics in WaitForElementTillPageNavigationSettled(string) by using the string FindElement path for the Catalyst pre-wait |
HelperExtensions.cs |
Compiled; same HostApp registration blocker. Fixes text-backed waits such as DropCompleted and Abyssinian. |
|
| 3 | maui-expert-reviewer | Keep full-desktop screenshot cropping but derive scale from CoreGraphics display backing scale | UITest.cs |
Compiled; same HostApp registration blocker. Stronger than PR heuristic for Retina scale, but mixed-monitor coordinate spaces remain a risk. | |
| 4 | maui-expert-reviewer | Make Issue33769 Stepper oracle phase-specific (MaximumReached then MinimumReached) |
Issue33769.cs HostApp + test |
Compiled; same HostApp registration blocker. Prevents stale generic success text from satisfying decrement validation. | |
| PR | PR #35156 | Full-desktop screenshot crop with scale inferred from screenshot pixels divided by Appium window size; Catalyst wait/test stabilizations; refreshed Mac baselines | Multiple | Original PR; pre-flight code review found unresolved correctness issues. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| gpt-5.5 / maui-expert-reviewer | 1 | Yes | Window element screenshot avoids desktop/window scale mismatch. |
| gpt-5.5 / maui-expert-reviewer | 2 | Yes | Catalyst pre-wait should preserve string id-or-text semantics. |
| gpt-5.5 / maui-expert-reviewer | 3 | Yes | CoreGraphics backing scale keeps the existing crop model but replaces the bad scale heuristic. |
| gpt-5.5 / maui-expert-reviewer | 4 | Yes | Stepper test should use distinct max/min success states. |
Test Summary
All candidates were tested with targeted Mac test commands. Each candidate compiled successfully through Controls.TestCases.Mac.Tests.dll, but runtime validation was blocked before test bodies executed because Appium could not find/register the MacCatalyst HostApp bundle:
The app representing com.microsoft.maui.uitests could not be found.
This is the same environment blocker across all attempts and is not evidence that any candidate logic failed. The prior gate result was also inconclusive due build/environment failure, so no candidate can be marked passing.
Exhaustion / Selection
Exhausted: Yes — the loop explored the meaningful distinct approaches available from the expert review findings: alternate screenshot source, native display scale, central wait semantics, and stale test oracle correction. Further attempts would be minor variations without runnable MacCatalyst validation in this environment.
Selected Fix: None verified. Candidate #3 is the best screenshot alternative on code merits because it preserves the current full-desktop crop/snapshot model while replacing only the incorrect scale source with the display backing scale. Candidate #2 and #4 should be considered companion fixes for separate correctness issues found in the PR.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current metadata accurately describes the screenshot/pipeline intent but omits the winning fix's Catalyst wait-semantics and Stepper oracle corrections, and its screenshot-scale details should reflect the CoreGraphics backing-scale approach.
Recommended title
[macOS] UI Tests: Fix Retina screenshot masking and Catalyst stability
Recommended description
This pull request updates the macOS UI test pipeline configuration and improves the screenshot masking logic in UI tests to better support high-DPI (Retina) displays. The main changes ensure that screenshots are correctly masked and scaled regardless of display density, that MacCatalyst UI tests keep reliable navigation/wait behavior, and that the pipeline uses the appropriate shared pool for macOS agents.
**Screenshot logic improvements:**
- Updated the `TakeScreenshot()` method in `UITest.cs` to:
- Dismiss interfering macOS system dialogs before taking full-desktop Mac screenshots.
- Determine the display backing scale for the display containing the app window instead of comparing full-desktop screenshot pixels to app-window size.
- Scale all mask and crop coordinates to pixel units, ensuring the rounded rectangle mask aligns correctly on high-DPI screens.
- After masking, normalize the resulting image back to logical dimensions so downstream cropping logic remains consistent regardless of pixel density.
**MacCatalyst UI test stability:**
- Preserve `WaitForElementTillPageNavigationSettled(string)` id-or-visible-text semantics during the Catalyst pre-wait so text-backed waits such as `DropCompleted` and `Abyssinian` do not time out before the normal lookup fallback.
- Strengthen `Issue33769` coverage by using distinct `MaximumReached` and `MinimumReached` states, preventing the decrement phase from passing on stale max-phase success text.
- Force the TestCases HostApp to light theme on MacCatalyst for consistent screenshots on newer macOS agents.
- Add AutomationIds needed by updated MacCatalyst test paths and refresh macOS screenshot baselines.
**macOS CI configuration:**
- Move the public macOS UI test pool to the shared ACES Tahoe image.
- Update notification/dialog suppression scripts to reduce system UI interference during MacCatalyst UI tests.
🏁 Report — Final Recommendation
Comparative Report — PR #35156
Candidate ranking
| Rank | Candidate | Test result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Best candidate. It preserves the PR's broad macOS UI-test infrastructure and baseline updates while applying the three major expert-review fixes: reliable display backing scale, preserved text/id wait semantics, and phase-specific Stepper assertions. | |
| 2 | try-fix-3 |
Best single screenshot alternative. It keeps the PR's full-desktop crop model but replaces the flawed window-size scale heuristic with CoreGraphics backing scale. It does not address the Catalyst text-wait or Stepper oracle issues by itself. | |
| 3 | try-fix-2 |
Correctly fixes the helper semantics regression for text-backed waits such as DropCompleted and Abyssinian, but it does not address the main Retina screenshot scaling issue. |
|
| 4 | try-fix-4 |
Correctly strengthens the Stepper test oracle, but it is limited to test coverage and does not fix screenshot scaling or wait semantics. | |
| 5 | try-fix-1 |
Conceptually aligns screenshot pixels and window-local bounds by using a window element screenshot, but it has higher Appium compatibility/baseline-shape risk than the CoreGraphics scale approach. | |
| 6 | pr |
Lowest-ranked non-failing candidate because the raw PR still has three major correctness issues found by pre-flight and expert review. |
No candidate has a passed regression result, and no candidate has a failed regression result. All attempted runtime validation was blocked or inconclusive by environment/HostApp registration issues, so the required pass/fail ordering rule does not demote any candidate for a regression failure.
Key comparison
The raw PR contains necessary surrounding work: macOS CI pool changes, notification/dialog suppression, forced light mode, refreshed Mac snapshot baselines, and several MacCatalyst UI-test stabilizations. However, its main Retina scaling fix still mixes full-desktop screenshot pixels with app-window logical dimensions, which can crop the wrong pixels for non-fullscreen Retina windows.
try-fix-3 is the strongest standalone screenshot fix because it preserves the existing full-desktop screenshot flow and changes only the scale source to the display backing scale. try-fix-2 and try-fix-4 are companion fixes for separate correctness problems found in the PR, not replacements for the screenshot fix. try-fix-1 is a useful alternate direction but depends on Appium Mac window element screenshots and may alter the screenshot content differently from the existing baseline model.
pr-plus-reviewer wins because it combines the PR's complete implementation context with the best screenshot fix and the two companion correctness fixes. It leaves only the moderate coordinate-tap concern as an unresolved discussion item because the reviewed diff does not expose a safe identifier-backed replacement and App.Back() would change what the affected BackButtonBehavior tests exercise.
Winner
Winning candidate: pr-plus-reviewer
Rationale: It is the only candidate that keeps the PR's full intended scope while resolving all major expert-review findings. Since all candidates are runtime-unverified due the same environment blocker and none failed regression tests, the combined candidate is strongest on code correctness and coverage merits.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 4 findings
See inline comments for details.
| // App.Screenshot() returns pixels at the display's native resolution | ||
| // (1x on non-Retina, 2x on Retina displays). | ||
| // Strategy: Compare screenshot pixel dimensions to screen logical dimensions. | ||
| var screenSize = ((AppiumApp)App).Driver.Manage().Window.Size; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
❌ [major] Logic and Correctness — The Retina scale is derived from the full-desktop screenshot size divided by the Appium app-window size. On a Retina desktop larger than the 1024x768 test window, image.Width / screenSize.Width describes desktop-pixels-per-window-point (for example ~3024/1024) rather than the display backing scale, so pxX/pxY/pxWidth/pxHeight crop the wrong region and the regenerated Mac baselines can still be invalid on non-fullscreen Retina windows. Use a scale source that describes the same coordinate space as windowBounds (or capture the window element directly).
| @@ -18,6 +18,6 @@ public void DropCompletedFiresWhenDroppingOnNonDropTarget() | |||
| App.WaitForElement("NonDropTarget"); | |||
|
|
|||
| App.DragAndDrop("DragSource", "NonDropTarget"); | |||
| App.WaitForElement("DropCompleted"); | |||
| App.WaitForElementTillPageNavigationSettled("DropCompleted"); | |||
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
❌ [major] Logic and Correctness — This replaces the normal string wait with WaitForElementTillPageNavigationSettled, but that helper does a Catalyst-only pre-wait using AppiumQuery.ById(elementId) before falling back to the existing id-or-visible-text lookup. Callers here and in Issue33356.cs:39 pass visible text (DropCompleted, Abyssinian), so Mac Catalyst can time out before reaching the string lookup that would have found the text. Preserve the helper's original string semantics for the pre-wait or use an explicit query/AutomationId.
| Assert.That(result, Is.EqualTo("Success")); | ||
|
|
||
| App.DecreaseStepper("Issue33769_Stepper"); | ||
| result = App.WaitForElement("Issue33769_StepperStatusLabel").GetText(); | ||
| Assert.That(App.WaitForTextToBePresentInElement("Issue33769_StepperStatusLabel", "Success"), Is.True); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
❌ [major] Regression Prevention and Test Coverage — The decrement phase waits for the same generic Success text that was already produced by the increment phase. If DecreaseStepper is a no-op or fails to reach the minimum, the stale label still satisfies this wait and the following equality assertion, so the test does not actually cover the minimum-bound regression. Make the page expose distinct max/min states (or reset to an intermediate value) before asserting the decrement path.
| { | ||
| // On Mac Catalyst, the AutomationId set on FileImageSource inside BackButtonBehavior.IconOverride | ||
| // is not working, so tap the back button by coordinates as a workaround. | ||
| App.TapCoordinates(158, 67); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
MauiBot
removed
the
s/agent-review-in-progress
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Description of Change Automates visual snapshot evidence in `/review tests`. When public AzDO results contain failed screenshot comparisons, the command now emits exactly one test-failure analysis comment containing bounded, expandable baseline/actual/diff panels. Visual evidence remains supplementary: it does not change `gate.verdictCeiling`, deterministic attribution, or the merge-readiness verdict. #### One-comment flow 1. Trusted pre-activation code discovers failed visual results through the public AzDO `resultsbybuild` API, including retry-suffixed attachments such as `Snapshot[1].png` and `Snapshot-diff[1].png`. 2. It resolves baselines from the exact source version tested by AzDO and maps runtime evidence to the correct snapshot directory (`ios-26`, `android-notch-36`, `mac`, or `windows`). 3. It streams and validates bounded PNG assets, then stores them on `review-tests-assets` using immutable commit-pinned `raw.githubusercontent.com` URLs. 4. The Copilot agent emits the normal single `add_comment` analysis payload with a trusted insertion marker. 5. A sealed post-step validates the published asset manifest and injects as many expandable comparison panels as fit into that same comment. Each collapsed panel shows a conservative relationship label: - `Likely PR-caused` for an exact test/platform base regression or directly changed snapshot/test; - `Likely unrelated` for an exact base/known-issue match without direct visual scope; - `Needs human investigation` for unmatched or mixed evidence. 6. Excess comparisons are summarized as omitted instead of creating a companion comment. The local `.github/scripts/Review-Tests.ps1 -PostComment` path uses the same merger. It also recognizes complete reports returned in Copilot's final response, preserving nested evidence code fences without wrapping a second title or badge section. ### Security and Failure Safety - PR text, logs, test names, attachment metadata, changed files, and visual labels remain untrusted input. - The merger script and visual context are copied to a root-owned location before the workflow checks out the untrusted PR branch. - The post-step runs without `COPILOT_GITHUB_TOKEN`, `GH_TOKEN`, or `GITHUB_TOKEN`. - AzDO attachment URLs must match the expected public project and attachment route. - Published assets are size-bounded, signature-checked PNGs with validated dimensions and repository paths. - Raw image URLs must match the exact repository, asset commit, PR directory, and safe filename. - Labels are HTML-escaped and `@` is neutralized before insertion. - Relationship labels use fixed trusted text. Untrusted attribution values are never rendered, and same-named snapshots changed on another platform do not count as PR scope. - The final body is checked panel-by-panel against conservative limits of 45 URLs, 10 mentions, and 60,000 UTF-16 characters, below gh-aw's throwing limits. - The analysis JSON update is atomic (written to a temp file, then renamed over the original). Invalid context, malformed output, missing analysis payloads, limit failures, and dry-run/noop output leave the original analysis unchanged. - The publisher never creates or patches PR comments; only the existing gh-aw `add_comment` payload is mutated. ### What NOT to Do - Do not use the ordinary anonymous AzDO test-runs listing for discovery; it redirects to sign-in. Use the public failed-results endpoint. - Do not resolve baselines from the current PR head; use the source version actually tested by the selected AzDO build. - Do not let the agent construct or trust visual asset URLs. - Do not publish a second companion comment; merge bounded panels into the single analysis payload. ### Validation - 98 focused Pester tests pass. - Changed PowerShell scripts parse successfully. - `gh aw compile copilot-review-tests --approve` completes without errors or warnings. - A real `agent_output.json` from gh-aw run [29674953402](https://github.com/dotnet/maui/actions/runs/29674953402) was replayed through the post-step: - one `add_comment` item remained one item; - five visual panels were inserted; - the final body contained 26 URLs, one mention, and 9,585 characters. ### Live Single-Comment Examples The exact local `/review tests` path from this branch posted or repaired these merged comments after the PRs' `/azp run` pipelines completed: | PR | Single merged result | Included evidence | Relationship labels | Final limits | | --- | --- | --- | --- | --- | | #36413 | [Test-failure analysis with visual panels](#36413 (comment)) | 5 panels / 15 images | 1 PR-caused, 4 investigate | 23 URLs, 13,156 chars | | #36631 | [Test-failure analysis with visual panels](#36631 (comment)) | 6 panels / 18 images | 2 PR-caused, 4 investigate | 31 URLs, 21,367 chars | | #36395 | [Test-failure analysis with visual panels](#36395 (comment)) | 11 panels / 33 images; 19 omitted | 11 investigate | 43 URLs, 19,858 chars | | #36404 | [Test-failure analysis with visual panels](#36404 (comment)) | 14 panels / 40 images; 81 omitted | 14 investigate | 45 URLs, 23,779 chars | | #35846 | [Test-failure analysis with visual panels](#35846 (comment)) | 10 panels / 30 images; 9 omitted | 10 investigate | 43 URLs, 22,496 chars | | #36277 | [Test-failure analysis with visual panels](#36277 (comment)) | 7 panels / 19 images | 3 PR-caused, 4 investigate | 31 URLs, 18,355 chars | | #36170 | [Test-failure analysis with visual panels](#36170 (comment)) | 11 panels / 33 images; 8 omitted | 11 investigate | 44 URLs, 23,180 chars | | #35578 | [Test-failure analysis with visual panels](#35578 (comment)) | 12 panels / 36 images; 50 omitted | 12 investigate | 44 URLs, 25,443 chars | | #36672 | [Test-failure analysis with visual panels](#36672 (comment)) | 14 panels / 40 images; 9 omitted | 14 investigate | 45 URLs, 25,915 chars | | #31755 | [Test-failure analysis with visual panels](#31755 (comment)) | 12 panels / 36 images; 3 omitted | 12 investigate | 44 URLs, 22,728 chars | | #34637 | [Test-failure analysis with visual panels](#34637 (comment)) | 9 panels / 27 images; 81 omitted | 9 investigate | 43 URLs, 22,325 chars | | #35156 | [Test-failure analysis with visual panels](#35156 (comment)) | 2 panels / 6 images | 1 PR-caused, 1 investigate | 19 URLs, 11,911 chars | | #35885 | [Test-failure analysis with no visual failures](#35885 (comment)) | 0 panels / 0 images | No visual failures detected | 8 URLs, 4,239 chars | | #36577 | [Test-failure analysis with visual panels](#36577 (comment)) | 1 panel / 3 images | 1 investigate | 44 URLs, 21,768 chars | | #36212 | [Test-failure analysis with no visual failures](#36212 (comment)) | 0 panels / 0 images | No visual failures detected | 5 URLs, 5,982 chars | Each result contains one `Tests Failure Analysis` title and one merged review marker. Across 114 rendered panels, all 336 embedded image URLs returned HTTP 200. Seven panels were safely classified as likely PR-caused; no panel in this sample had enough exact evidence to be safely classified as likely unrelated, so the remaining 107 stayed at `Needs human investigation`. Another 260 comparisons were omitted safely by the comment limits. The latest eight-example batch was regenerated concurrently, and #36672, #31755, #34637, #35156, #35885, #36577, and #36212 were added afterward. The current [`review-tests-assets` head](f937993) retains the full asset history. The protected `copilot-pat-pool` environment rejects feature-branch `workflow_dispatch` runs before job execution. The live local-runner examples validate comment generation and asset publication, while the real gh-aw output replay validates the workflow post-step mutation without weakening that branch protection. ### Issues Fixed N/A - reviewer workflow enhancement. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: kubaflo <kubaflo@users.noreply.github.com> Copilot-Session: a280b482-e102-4ca0-9ff9-1cfe1946e21f Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d Copilot-Session: 478d195b-20f3-4bc6-aeed-f6aa88b55fda
|
@NafeelaNazhir what is the status of this PR? |
@kubaflo Regarding this PR, the Stepper-related test cases are continuously failing, and each CI run is introducing new failures. I'm currently working on resolving these issues and will update you once they are fixed. |
6 participants
This pull request updates the macOS UI test pipeline configuration and improves the screenshot masking logic in UI tests to better support high-DPI (Retina) displays. The main changes ensure that screenshots are correctly masked and scaled regardless of display density, and that the pipeline uses the appropriate shared pool for macOS agents.
Screenshot logic improvements:
TakeScreenshot()method inUITest.csto: