[net11] [iOS][Testing] Extend NavigationPage device tests to run against both NavigationRenderer and NavigationViewHandler - #37802
Conversation
Restore NavigationRenderer coverage for iOS/MacCatalyst device tests that were migrated to NavigationViewHandler. Reuse the existing tests so they run against both NavigationRenderer and NavigationViewHandler without duplicating test bodies.
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37802Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37802" |
GitHub Actions
Inactive
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet-policy-service
Bot
added
the
partner/syncfusion
GitHub Actions
Inactive
GitHub Actions
Inactive
Vignesh-SF3580
changed the title
GitHub Actions
Inactive
github-actions
Bot
added
area-controls-navigationpage
platform/ios
platform/macos
GitHub Actions
Inactive
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines: Successfully started running 2 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.
Expert Review — 7 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Vignesh-SF3580 — new AI review results are available based on commit
751831e.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ INCONCLUSIVE
The trusted test-verification gate could not produce a definitive pass/fail result. Its
detailed display artifact was missing or contradicted this trusted pipeline verdict, so that
agent-writable content was not used in the review summary.
Inspect the trusted Gate task logs for the build, environment, or infrastructure blocker.
📋 Pre-Flight — Context & Validation
PR #37802 Pre-Flight
Context
- Title:
[net11] [iOS] Extend NavigationPage device tests to run against both NavigationRenderer and NavigationViewHandler - State: Open draft PR targeting
net11.0; public PR head751831ebf679522e477806c9f66ec925c5bd8098. - Materialized review commit:
f40aadb13a16aa0882913aea8b263aa5b08948fdonpr-review-37802, with parent4695c95801e0b6764beb83f314c62141ee9c7f2e. - Problem: Existing iOS/MacCatalyst
NavigationPagedevice tests and tests that embed aNavigationPagedo not consistently exercise bothNavigationRendererand the productionNavigationViewHandlerpath. - Gate: Inconclusive because the prior gate could not build/run the tests. This is not evidence that the PR fix fails, and the gate must not be rerun.
- Discussion: There are no human review comments or submitted reviews. PR comments only show pipeline/review automation requests.
Current PR Approach
The PR changes 26 test/test-infrastructure files (+474/-97). It makes handler setup overridable, changes the iOS/MacCatalyst base fixtures to register NavigationRenderer, and adds 10 iOS fixture subclasses that inherit the same facts/theories while overriding registration to use NavigationViewHandler. It applies this pattern to NavigationPage and supporting CollectionView, FlyoutPage, Modal, Shell, TabbedPage, Toolbar, VisualElement, VisualElementTree, and Window tests. It also:
- adds
NavigationViewVarianttrait constants; - prefixes reused xUnit display names with
[NavigationRenderer]or[NavigationViewHandler]; - updates Entry, Memory, and
ControlsPageTypesTestCasessetup paths; - keeps handler-specific disposal/navigation tests pinned to their intended implementation.
The PR description says nine new fixture files and omits the new TabbedPage fixture, while the inspected diff contains 10 new fixture files.
Direct Diff Assessment
The implementation is test-only; it does not change shipping controls code. The central mechanism is inherited xUnit test discovery: a renderer-tagged base fixture and handler-tagged subclasses cause the same test bodies to execute under both registrations. Supporting fixtures generally extract RegisterNavigationPageHandler(...); Modal, TabbedPage, and VisualElementTree instead override a boolean-bearing SetupBuilder(...). xUnitCustomizations.GetReuseVariantPrefix() checks the handler value first because inherited class traits can contain both renderer and handler values.
Bounded Validation
- Platform:
ios - Primary test command:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "Category=NavigationPage" - Mandatory regression tests: none were enumerated in the STEP 5a request. Do not run a full suite or invent broader category runs.
- Applicable repository guidance:
.github/instructions/helix-device-tests.instructions.mdand.github/instructions/threading-async.instructions.md.
Candidate Constraints
Each try-fix invocation must establish and restore the broken baseline only through .github/scripts/EstablishBrokenBaseline.ps1, preserve all pre-existing dirty/untracked paths, perform one implementation/test pass with at most one focused correction/retest, and record Pass, Fail, or Blocked honestly. The current PR adds production-branch test files; if baseline state reports any NewFiles, the try-fix skill requires a Blocked result before editing.
🔬 Code Review — Deep Analysis
Expert PR Evaluation
Verdict: REQUEST CHANGES
The submitted change adds an iOS/MacCatalyst device-test axis that reuses existing test bodies with either NavigationRenderer or NavigationViewHandler. The overall reuse approach is sound, but the handler-variant subclass for TabbedPageTests unconditionally forces NavigationViewHandler and silently overrides two inherited tests that explicitly request NavigationRenderer. One of those tests documents that the renderer-specific navigation path is required to avoid hangs, so the current PR can fail or hang in the new variant.
Actionable findings
- Blocking:
TabbedPageNavigationHandlerTests.iOS.csmust preserve explicitincludeNavigationViewHandler: falserequests, or the affected renderer-only tests must register their intended handler inline. - Warning:
VisualElementTreeNavigationHandlerTests.iOS.cssimilarly discards an explicit renderer request; the test intent must be made unambiguous. - Warning:
NavigationPageTestsapplies the iOS-specificNavigationRenderertrait on Android, producing misleading Android test display names. - Suggestions: Avoid latent ignored-argument traps in the modal variant, retain intentional programmatic-pop coverage if it exercises a distinct path, remove a duplicate VisualElement registration, and correct stale handler-variant filenames in comments.
The trusted Gate was inconclusive because the tests could not be built or run. That is an evidence gap, not a regression-test failure, and does not change the code-level blocking finding.
🛠️ Try-Fix — Analysis & Comparison
PR #37802 — STEP 5a Try-Fix Aggregate
Candidate 1 — Discovery-Time Variant Multiplication
Model: claude-opus-5
Result: Blocked
Detailed report: ../try-fix-1/content.md
Skill artifacts: attempt-1/
Approach
Use custom xUnit [NavigationVariantFact] / [NavigationVariantTheory] discoverers to emit renderer and handler cases from each test method. Each emitted test case would set an AsyncLocal variant consumed when the host builder registers NavigationPage. Variant identity and display naming would live on each discovered case rather than on inherited fixture types.
Prior approach avoided
The PR uses compile-time fixture duplication: a renderer-tagged base class, 10 handler subclasses, per-fixture virtual registration seams, inherited class traits, and a display-name prefix heuristic. Its shared scaling/failure mechanism is that variant identity lives in the CLR type; inherited handler subclasses can carry both renderer and handler trait values, making handler-first trait ordering load-bearing and requiring a separate trait axis.
Mechanism-level difference
Discovery-time multiplication moves identity to independent xUnit test-case objects. A single discoverer would emit both cases and set/clear the ambient value around execution, preventing fixture drift and inherited-trait ambiguity by construction. The discoverer would name each case directly, avoiding the PR's iOS/MacCatalyst change to shared display-name infrastructure and eliminating the 10 new fixture types.
Files changed and diff
None. The baseline guard blocked the attempt before editing, so fix.diff is empty.
Test result
The permitted command was:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "Category=NavigationPage"
It was not run because no candidate implementation could be applied.
Failure analysis
EstablishBrokenBaseline.ps1 rejected the pre-existing dirty tracked worktree before creating .github/.baseline-state.json. The unrelated .github/scripts, .github/skills, and eng/scripts changes must be preserved, while the try-fix contract forbids cleaning, stashing, resetting, or bypassing the baseline script. The attempt therefore had to report Blocked before editing. The PR is also entirely test/test-infrastructure code matching the baseline script's test-path exclusions, so a clean worktree may still produce the separate No fix files detected blocker.
The inline expert self-review found 0 issues against an empty candidate diff. The required restore command ran and reported the expected no-state result (Restored False; no baseline state found); target source paths remained clean and pre-existing worktree changes were untouched.
Candidate 2 — Sequential Dual-Context Execution
Model: gpt-5.6-sol
Result: Blocked
Detailed report: ../try-fix-2/content.md
Skill artifacts: attempt-2/
Approach
Keep each scenario as one normally discovered xUnit case, but execute its NavigationPage scenario twice in sequence. Each invocation would create and dispose a fresh MAUI handler context/window and pass an explicit implementation value to registration: first NavigationRenderer, then NavigationViewHandler.
Prior approaches avoided
This avoids the PR's renderer base fixtures, handler subclasses, virtual registration overrides, inherited traits, and display-name prefixing. It also avoids Candidate 1's custom fact/theory discovery and AsyncLocal ambient variant state. Both earlier designs place variant identity in discovery or fixture identity and propagate it outside the scenario.
Mechanism-level difference
Variant selection would be an ordinary argument scoped to two explicit scenario invocations within one test case. Fresh contexts/windows isolate the executions without fixture inheritance, trait/display-name machinery, custom discovery, or ambient state.
Files changed and diff
None. The baseline guard blocked the attempt before editing, so no candidate diff exists.
Test result
The permitted command was:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "Category=NavigationPage"
It was not run because no baseline state or candidate implementation existed.
Failure analysis
As in Candidate 1, EstablishBrokenBaseline.ps1 rejected the pre-existing dirty tracked worktree and did not create .github/.baseline-state.json. Without a RevertedFiles edit allow-list, the try-fix contract required Blocked before editing; cleaning, stashing, resetting, or bypassing the guard was prohibited.
The inline expert self-review found 0 issues against the empty diff. The required restore command ran and reported the expected no-state result (Restored False; no baseline state found), preserving all pre-existing dirty and untracked paths.
Aggregate Outcome
Two distinct alternatives were designed, but neither could be implemented or tested under the mandatory baseline safety contract. Both results are Blocked, not failures of their proposed mechanisms and not verification of the PR. No candidate source changes remain in the worktree.
📝 PR Finalize — Recommended Title & Description
Assessment: ✏️ Recommend updating — the title is specific, but the description omits the new TabbedPage variant fixture, reports nine instead of ten new fixtures, and overstates that no test logic was rewritten.
Recommended title
[net11] [iOS] NavigationPage device tests: Cover NavigationRenderer and NavigationViewHandler
Recommended description
### Description of Change
#### Problem
`NavigationPage` was migrated from the renderer to the handler architecture on iOS/MacCatalyst. Despite that migration, existing `NavigationPage` device tests and other device tests that embed a `NavigationPage` as a supporting page (Shell, Modal, Window, Toolbar, TabbedPage, CollectionView, FlyoutPage, VisualElement, and VisualElementTree) still hardcoded `NavigationRenderer` on iOS/MacCatalyst. As a result, the new `NavigationViewHandler` path had no equivalent device-test coverage even though it is the path real apps use.
#### Approach
Rather than create a parallel copy of each test suite for `NavigationViewHandler`, this PR makes NavigationPage handler registration overridable and reuses existing test fixtures across both implementations, following the renderer/handler variant pattern already used by Android Shell, Modal, and Window tests.
For each affected fixture:
- `SetupBuilder` (or the equivalent registration path) is made virtual.
- NavigationPage registration is extracted into an overridable hook that defaults to `NavigationRenderer` on iOS/MacCatalyst. Shared registrations are factored into common helpers where appropriate.
- A new `*NavigationHandlerTests.iOS.cs` subclass selects `NavigationViewHandler` while inheriting the existing facts and theories.
- Implementation-specific cases use direct registration or dedicated test methods where required.
`xUnitCustomizations.GetReuseVariantPrefix()` now runs on iOS/MacCatalyst and prefixes reused test display names with `[NavigationRenderer]` or `[NavigationViewHandler]`, matching the existing Android `[Renderer]` / `[Handler]` convention.
#### Files changed
| File | Change |
|---|---|
| `RendererHandlerVariant.cs` | Adds `NavigationViewVariant` trait constants for `NavigationRenderer` and `NavigationViewHandler`. |
| `xUnitCustomizations.cs` | Extends `GetReuseVariantPrefix()` to iOS/MacCatalyst. |
| `NavigationPageTests.cs` / `.iOS.cs` | Adds overridable NavigationPage/common registration and consolidates variant coverage. |
| `ShellTests.cs`, `ModalTests.cs`, `WindowTests.cs`, `ToolbarTests.cs`, `TabbedPageTests.cs`, `FlyoutPageTests.cs`, `CollectionViewTests.cs`, `VisualElementTests.cs`, `VisualElementTreeTests.cs`, `MemoryTests.cs` | Applies the overridable registration pattern wherever a NavigationPage is used as supporting test infrastructure. |
| `ControlsPageTypesTestCases.cs` | Adds an optional parameter selecting `NavigationViewHandler` for the static `ClassData` path. |
| `EntryTests.iOS.cs` | Routes setup through the new handler-registration hook. |
| 10 new `*NavigationHandlerTests.iOS.cs` files | Adds handler-variant fixtures for NavigationPage, Shell, Modal, Window, Toolbar, TabbedPage, FlyoutPage, CollectionView, VisualElement, and VisualElementTree tests. |
This is a test-infrastructure-only change; it does not modify shipping controls code.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winning candidate: pr-plus-reviewer
The submitted PR has a sound fixture-reuse strategy, but the expert review found a concrete handler-selection defect: TabbedPageNavigationHandlerTests forces NavigationViewHandler even when inherited tests explicitly request NavigationRenderer and construct pages for the renderer-specific navigation path. pr-plus-reviewer fixes that mismatch by separating each fixture's default implementation from explicit per-test overrides. It also resolves the analogous VisualElementTree ambiguity, prevents the iOS renderer trait from affecting Android display names, and applies the low-risk registration/comment cleanups from the review.
Comparative ranking
| Rank | Candidate | Implementation | Validation | Assessment |
|---|---|---|---|---|
| 1 | pr-plus-reviewer |
Applied on the required baseline | Pass: 38/38 iOS NavigationPage tests; build had 0 errors |
Best candidate. It preserves the PR's architecture while correcting the blocking test-variant mismatch. |
| 2 | pr |
Submitted fix | Gate inconclusive | The implementation is materially complete, but the unresolved TabbedPage mismatch can fail or hang inherited handler-variant tests. |
| 3 | try-fix-1 |
Not applied | Blocked before testing | Discovery-time multiplication is only a design proposal; it produced no diff or empirical evidence and adds custom discovery plus ambient state complexity. |
| 4 | try-fix-2 |
Not applied | Blocked before testing | Sequential dual-context execution is only a design proposal; it produced no diff or empirical evidence and would combine two independently useful variant results into one test case. |
No candidate failed a regression test. Both try-fix-* candidates were blocked before implementation, and the raw PR Gate was inconclusive rather than failed. pr-plus-reviewer is the only implemented candidate with passing targeted validation.
Validation evidence and uncertainty
The required command ran once from /Users/cloudtest/vss/_work/_temp/pr-37802-pr-plus-reviewer:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "Category=NavigationPage"
All build and test outputs were rooted in the candidate worktree, the build completed with 0 errors, and the test run reported 38 passed and 0 failed. The filter does not directly execute the TabbedPage or VisualElementTree categories, so the passing run proves compilation and focused NavigationPage behavior but does not eliminate all device-runtime uncertainty in those inherited fixtures.
The optional suggestion to retain a separate UIKit programmatic-pop test was not included because the expert review did not establish that it exercises a distinct production path strongly enough to justify expanding the one-shot refinement. This remains a non-blocking coverage discussion, not a reason to prefer the defective raw PR.
Required change: Apply pr-plus-reviewer/reviewer.patch to the submitted PR before approval.
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 355 passed, 0 failed, 8 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
71/72 (1 skipped) ✓ | — |
Label |
90/92 (2 skipped) ✓ | — |
Layout |
194/199 (5 skipped) ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — reviewer changes required
The reviewer-enhanced candidate identified changes that are not yet in the submitted PR.
Why: The reviewer-enhanced PR preserves the submitted fixture-reuse design while fixing the blocking handler-selection mismatch and related variant-label ambiguity. It built successfully in the mandated sandbox and passed all 38 targeted iOS NavigationPage tests.
Address the actionable findings in this review before merging.
MauiBot
added
s/agent-changes-requested
sheiksyedm
changed the title
Added RegisterNavigationPageHandler() to the Modal, TabbedPage, and VisualElementTree test base classes, allowing handler-specific tests to register NavigationViewHandler without overriding SetupBuilder(). Updated tests that require a specific navigation variant to explicitly register the required handler, avoiding silent overrides of the test setup. Restored Handler_NavigatingBackViaProgrammaticPopFiresNavigatedEvent to retain coverage for programmatic PopViewController(animated: false) navigation. Kept back-button navigation coverage for both NavigationRenderer and NavigationViewHandler through the existing test.
I have addressed the suggestions.
|
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
5 participants
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 from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Problem
NavigationPagewas migrated from the renderer to the handler architecture on iOS/MacCatalyst. Despite that migration, every existingNavigationPagedevice test — and every other device test that embeds aNavigationPageas a supporting page (Shell, Modal, Window, Toolbar, CollectionView, FlyoutPage, VisualElement/VisualElementTree) — still hardcodedNavigationRendereron iOS/MacCatalyst. As a result, the newNavigationViewHandlercode path had zero device test coverage, even though it's now the code path real apps run against.Approach
Rather than write a parallel/duplicate set of tests for
NavigationViewHandler, this PR makes the handler registration forNavigationPageoverridable and reuses every existing test body across both variants — the same pattern already used for the Android Shell/Modal/Window renderer/handler axis.For each affected test class:
SetupBuilder(or equivalent) is madevirtual.NavigationPagehandler registration is extracted into a new virtualRegisterNavigationPageHandlers(...)hook, defaulting to the existingNavigationRendererregistration on iOS/MacCatalyst (no behavior change for the base class). Handlers shared by every variant are factored into aRegisterCommonHandlers(...)helper to avoid duplication.*NavigationHandlerTests.iOS.cssubclass overrides that hook to register the realNavigationViewHandlerinstead, inheriting every[Fact]/[Theory]unchanged — so the same test bodies run twice, once per variant.xUnitCustomizations.GetReuseVariantPrefix()was extended to run on iOS/MacCatalyst and tag each test'sDisplayNamewith[NavigationRenderer]/[NavigationViewHandler], mirroring the existing Android[Renderer]/[Handler]convention, so results stay distinguishable in test output.Files changed
RendererHandlerVariant.csNavigationViewVarianttrait constants (NavigationRenderer/NavigationViewHandler)xUnitCustomizations.csGetReuseVariantPrefix()now also runs on iOS/MacCatalystNavigationPageTests.cs/.iOS.csRegisterNavigationPageHandlers+RegisterCommonHandlershooksShellTests.cs,ModalTests.cs,WindowTests.cs,ToolbarTests.cs,FlyoutPageTests.cs,CollectionViewTests.cs,VisualElementTests.cs,VisualElementTreeTests.cs,MemoryTests.csNavigationPageis embedded as a supporting pageControlsPageTypesTestCases.csNavigationViewHandler(staticClassDatahelper, can't be subclassed)EntryTests.iOS.cs*NavigationHandlerTests.iOS.csfilesNavigationPageHandlerTests,ShellNavigationHandlerTests,ModalNavigationHandlerTests,WindowNavigationHandlerTests,ToolbarNavigationHandlerTests,FlyoutPageNavigationHandlerTests,CollectionViewNavigationHandlerTests,VisualElementNavigationHandlerTests,VisualElementTreeNavigationHandlerTests)No existing test logic was duplicated or rewritten — only the handler-registration hook was made overridable.