[Testing] Additional Feature Matrix Event Test Cases for Slider and ScrollView#34352
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34352Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34352" |
|
Hey there @@nivetha-nagalingam! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR extends the Feature Matrix coverage for MAUI controls by adding HostApp plumbing and UI tests that validate Slider.ValueChanged and ScrollView.ScrollToRequested event behavior and arguments.
Changes:
- Added
Slider.ValueChangedevent tracking (raised/not-raised + old/new values) in the Slider Feature Matrix HostApp page and corresponding UI tests. - Added
ScrollView.ScrollToRequestedevent tracking (requested X/Y/position/animate/mode/element) in the ScrollView Feature Matrix HostApp page and corresponding UI tests. - Updated one Android screenshot baseline impacted by the Slider page UI changes.
Reviewed changes
Copilot reviewed 8 out of 251 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/SliderFeatureTests.cs | Adds UI tests validating Slider ValueChanged status and arguments. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ScrollViewFeatureTests.cs | Adds UI tests validating ScrollView ScrollToRequested event and argument values. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/Slider/SliderViewModal.cs | Adds bindable properties for Slider ValueChanged status + old/new values. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/Slider/SliderControlPage.xaml.cs | Hooks Slider ValueChanged to update the view model properties. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/Slider/SliderControlPage.xaml | Adds UI labels/AutomationIds for the new Slider event state/args. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewViewModel.cs | Adds bindable properties for ScrollToRequested status/args. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewControlPage.xaml.cs | Handles ScrollToRequested and adds a pixel-scroll trigger button handler. |
| src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewControlPage.xaml | Adds UI labels/AutomationIds for the new ScrollToRequested status/args and pixel-scroll trigger. |
| src/Controls/tests/TestCases.Android.Tests/snapshots/android/Slider_SetVisibilityToFalse_VerifyVisualState.png | Updates Android screenshot baseline for a Slider visual test. |
src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ScrollViewFeatureTests.cs
Show resolved
Hide resolved
src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ScrollViewFeatureTests.cs
Show resolved
Hide resolved
src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewControlPage.xaml.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewControlPage.xaml.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewControlPage.xaml.cs
Show resolved
Hide resolved
src/Controls/tests/TestCases.HostApp/FeatureMatrix/ScrollView/ScrollViewViewModel.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ScrollViewFeatureTests.cs
Show resolved
Hide resolved
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34352 | Add HostApp event handlers + ViewModel properties + test methods | ⏳ PENDING (Gate) | 8 code files | Original PR |
Issue: None (pure testing PR — no linked issue)
PR: #34352 - [Testing] Additional Feature Matrix Event Test Cases for Slider and ScrollView
Author: nivetha-nagalingam (community / Syncfusion partner)
Platforms Affected: All (test-only PR — new tests run on Android, iOS, macOS)
Files Changed: 8 code files + ~403 PNG snapshot files
Key Findings
- This is a pure testing PR — no production code changes. Adds Feature Matrix event test coverage for
SliderandScrollView. - Slider: Adds
ValueChangedevent handler in HostApp, properties (ValueChangedStatus,OldValue,NewValue) toSliderViewModel, and 4 new test methods verifying event raise/no-raise and argument values. - ScrollView: Adds
ScrollToRequestedevent handler in HostApp, 7 new properties toScrollViewViewModel, a "Scroll To Pixel" button, and 11 new test methods. - Copilot review feedback was addressed (prior review): stray
;removed,OnScrollToPixelClickedmadeasync/awaited,Elementproperty renamed fromobjecttostring(RequestedElementTypeName), unusedusing System.Windows.Input;removed, timing waits added. - Prior Gate (Android):
⚠️ SKIPPED — Android emulator environment failure (ADB broken pipe). Re-running Gate on iOS. - Remaining concerns identified by prior review:
ScrollView_ScrollToRequested_Position_IsCorrectmay be flaky — asserts immediately after tap without polling for label update.ScrollView_ScrollToPixel_*tests may have format ambiguity —double→ string withoutStringFormat.Slider_ValueBelowMinimum_EventNotRaised_WithClampedValuebehavior is platform-specific.- XAML declares
ValueChanged="OnSliderValueChanged"AND code-behind subscribes — but NOT a real double subscription (slider is recreated inReInitializeSlider()).
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34352 | Add HostApp event handlers + ViewModel properties + test methods | ⏳ PENDING (Gate) | 8 code files | Original PR |
Prior Agent Review
- Prior run detected: Yes (on Android — Gate was skipped)
- Prior try-fix: 2 attempts, both passing — found real timing + format issues in new tests
- Prior recommendation: REQUEST CHANGES — fix timing (
WaitForTextToBePresentInElement) and addStringFormatto scroll coordinate labels
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Platform: Android
Mode: Testing PR — Verify tests PASS with PR changes
Note: This is a pure testing PR. No bug baseline exists. Verification: do new tests pass with PR's HostApp changes?
- Tests PASS with PR changes:
⚠️ SKIPPED (environment issue) - Error details: Android emulator deployment failure due to transient ADB0010 broken-pipe errors on API 30 (see below)
Error Summary:
The Gate verification was skipped due to Android emulator environment issues:
-
First Attempt: ADB0010 "Broken pipe" error during app installation
- Error:
Mono.AndroidTools.InstallFailedException: Unexpected install output: cmd: Failure calling service package: Broken pipe (32) - This is a known transient issue on Android API 30
- Error:
-
Second Attempt (Retry): App crashed or test framework timeout
- The app was deployed but failed to locate the test button (
GoToTestButton) - Device logs show repeated failed attempts to find the button via Appium
- App was forcibly stopped after 60+ seconds of button lookup failures
- The app was deployed but failed to locate the test button (
Raw test output (last 30 lines):
03-14 08:12:35.213 18392 18461 W ft.maui.uitest: ClassLoaderContext classpath size mismatch. expected=14, found=2
03-14 08:12:36.255 18392 18461 W ft.maui.uitest: Found duplicated class when checking oat files: 'Landroid/support/v4/graphics/drawable/IconCompatParcelizer;'
03-14 08:12:37.897 11057 11070 I ActivityManager: Force stopping com.microsoft.maui.uitests appid=10154 user=0: from pid 18359
03-14 08:12:37.899 11057 11070 I ActivityManager: Killing 18206:com.microsoft.maui.uitests/u0a154 (adj 0): stop com.microsoft.maui.uitests due to from pid 18359
03-14 08:12:37.910 11057 11070 W ActivityTaskManager: Force removing ActivityRecord{b3c6d38 u0 com.microsoft.maui.uitests/.MainActivity t13 f}}: app died, no saved state
⚠️ Build/deploy failed (attempt 1). ADB0010/broken-pipe errors are transient on API 30 — will retry.
⚠️ Retrying build/deploy (attempt 2 of 2)...
ℹ️ Restarting ADB server...
* daemon not running; starting now at tcp:5037
* daemon started successfully
❌ Tests failed with exit code 1
ℹ️ Review logs at: /home/vsts/work/1/s/CustomAgentLogsTmp/UITests
Recommendation:
This Gate should be re-run in a CI environment with a stable Android emulator. The PR code itself is not responsible for this environmental failure. The new test infrastructure (AutomationIds, test methods) appears to be properly added; verification requires a functioning emulator environment.
Gate Result: ✅ PASSED
Platform: iOS (iPhone 11 Pro simulator)
Mode: Testing PR — Verify new tests PASS with PR's HostApp changes
Note: This is a pure testing PR — no bug baseline exists. Verification confirms new tests pass with PR's changes.
- Tests FAIL without fix: N/A (testing PR — no bug to reproduce)
- Tests PASS with PR changes: ✅
Details:
- New Slider and ScrollView Feature Matrix tests executed successfully on iOS
SliderFeatureTestsandScrollViewFeatureTestsmatched and ran- Build succeeded; tests ran and passed
- Gate verification skill flagged pipeline config files as "fix files" (false positive — those are unrelated pipeline files not part of this testing PR). The actual test code passes correctly.
Conclusion: The new event tests work on iOS. Gate ✅ PASSED.
🔧 Fix — Analysis & Comparison
Gate Result: ⚠️ SKIPPED
Platform: Android
Mode: Testing PR — Verify tests PASS with PR changes
Note: This is a pure testing PR. No bug baseline exists. Verification: do new tests pass with PR's HostApp changes?
- Tests PASS with PR changes:
⚠️ SKIPPED (environment issue) - Error details: Android emulator deployment failure due to transient ADB0010 broken-pipe errors on API 30 (see below)
Error Summary:
The Gate verification was skipped due to Android emulator environment issues:
-
First Attempt: ADB0010 "Broken pipe" error during app installation
- Error:
Mono.AndroidTools.InstallFailedException: Unexpected install output: cmd: Failure calling service package: Broken pipe (32) - This is a known transient issue on Android API 30
- Error:
-
Second Attempt (Retry): App crashed or test framework timeout
- The app was deployed but failed to locate the test button (
GoToTestButton) - Device logs show repeated failed attempts to find the button via Appium
- App was forcibly stopped after 60+ seconds of button lookup failures
- The app was deployed but failed to locate the test button (
Raw test output (last 30 lines):
03-14 08:12:35.213 18392 18461 W ft.maui.uitest: ClassLoaderContext classpath size mismatch. expected=14, found=2
03-14 08:12:36.255 18392 18461 W ft.maui.uitest: Found duplicated class when checking oat files: 'Landroid/support/v4/graphics/drawable/IconCompatParcelizer;'
03-14 08:12:37.897 11057 11070 I ActivityManager: Force stopping com.microsoft.maui.uitests appid=10154 user=0: from pid 18359
03-14 08:12:37.899 11057 11070 I ActivityManager: Killing 18206:com.microsoft.maui.uitests/u0a154 (adj 0): stop com.microsoft.maui.uitests due to from pid 18359
03-14 08:12:37.910 11057 11070 W ActivityTaskManager: Force removing ActivityRecord{b3c6d38 u0 com.microsoft.maui.uitests/.MainActivity t13 f}}: app died, no saved state
⚠️ Build/deploy failed (attempt 1). ADB0010/broken-pipe errors are transient on API 30 — will retry.
⚠️ Retrying build/deploy (attempt 2 of 2)...
ℹ️ Restarting ADB server...
* daemon not running; starting now at tcp:5037
* daemon started successfully
❌ Tests failed with exit code 1
ℹ️ Review logs at: /home/vsts/work/1/s/CustomAgentLogsTmp/UITests
Recommendation:
This Gate should be re-run in a CI environment with a stable Android emulator. The PR code itself is not responsible for this environmental failure. The new test infrastructure (AutomationIds, test methods) appears to be properly added; verification requires a functioning emulator environment.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-sonnet-4.6) | ViewModel int conversion + sync barrier on ScrollToRequestedLabel |
✅ PASS | 3 files | Fixes format at data model + timing with different-element barrier |
| 2 | try-fix (claude-opus-4.6) | Direct label.Text in code-behind (bypass ViewModel binding) |
✅ PASS | 3 files | Fixes both issues at source; also improves Slider test |
| PR | PR #34352 | Add event handlers + VM properties + test methods | ✅ Gate Passed | 8 files | Original PR — has latent format + timing issues |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-sonnet-4.6 | 2 | NO NEW IDEAS | All layers covered: test, XAML, ViewModel, code-behind |
| claude-opus-4.6 | 2 | NO NEW IDEAS | Further approaches would be variants of existing 4 |
Exhausted: Yes — Both models agree problem space is fully explored
Best Fix Selection
Comparing the two ✅ PASS candidates:
| Criterion | Attempt 1 (sonnet) | Attempt 2 (opus) |
|---|---|---|
| Simplicity | ✅ Minimal (3 small changes) | |
| Robustness | ✅ High — int type eliminates format, sync barrier ensures event fired | ✅ High — direct assignment is synchronous |
| Layer | ViewModel (cleanest data contract) | Code-behind (couples UI to logic) |
| Codebase style | ✅ MVVM pattern — ViewModel owns data types | |
| Risk | Low | Medium — removes binding makes maintenance harder |
Selected Fix: Attempt 1 (claude-sonnet-4.6) — int ViewModel conversion + sync barrier
- Simpler (3 files, ~8 lines)
- Cleaner architecture (MVVM pattern preserved)
intproperty type is semantically correct (pixel coordinates are integers)- Sync barrier using
ScrollToRequestedLabelis more elegant than bypassing MVVM entirely
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Testing PR — no linked issue, 8 code files + 403 PNG snapshots |
| Gate | Android emulator environment failure (ADB broken pipe); testing PR — no bug baseline | |
| Try-Fix | ✅ COMPLETE | 2 attempts, 2 passing — both identified real improvements |
| Report | ✅ COMPLETE |
Summary
PR #34352 adds Feature Matrix event test cases for Slider (ValueChanged) and ScrollView (ScrollToRequested). This is a pure testing PR — no production code is changed. The PR is well-structured and addresses feedback from a prior Copilot review (removed stray ;, made OnScrollToPixelClicked async, renamed Element property to RequestedElementTypeName, removed unused using).
However, the try-fix exploration found two real quality issues in the new test code that could cause flaky failures in CI.
Root Cause
Two independent issues in the new test code:
-
Timing issue (
ScrollView_ScrollToRequested_Position_IsCorrect): The test taps a scroll button and immediately assertsRequestedPositionLabelwithout waiting for the asyncScrollToRequestedevent to fire and update the label. SinceScrollToAsyncis async/animated, the label update may lag, causing intermittent assertion failures. -
Format ambiguity (
RequestedScrollX/RequestedScrollYlabels): The ViewModel properties are typed asdouble, but the XAML bindings have noStringFormat. Adoublevalue of150.0may display as"150"or"150.0"depending on platform/runtime, while the tests assert against"150". This is a latent determinism issue.
Fix Quality
The PR's core approach is sound — the HostApp infrastructure (new labels, event handlers, ViewModel properties) is correctly implemented. The Copilot review feedback was addressed. Slider tests are correct and the ValueBelowMinimum behavior is confirmed accurate.
Recommended changes (from try-fix):
Fix 1 — Replace immediate assertion with wait/poll in ScrollView_ScrollToRequested_Position_IsCorrect:
// Before (flaky):
Assert.That(
App.FindElement("RequestedPositionLabel").GetText(),
Is.EqualTo(expectedPosition));
// After (robust):
Assert.That(
App.WaitForTextToBePresentInElement("RequestedPositionLabel", expectedPosition),
Is.True,
$"RequestedPositionLabel did not show '{expectedPosition}' in time");Fix 2 — Add StringFormat to XAML bindings for scroll coordinate labels (ScrollViewControlPage.xaml):
<!-- Before: -->
<Label AutomationId="RequestedScrollXLabel" Text="{Binding RequestedScrollX}"/>
<Label AutomationId="RequestedScrollYLabel" Text="{Binding RequestedScrollY}"/>
<!-- After: -->
<Label AutomationId="RequestedScrollXLabel" Text="{Binding RequestedScrollX, StringFormat='{0:0}'}"/>
<Label AutomationId="RequestedScrollYLabel" Text="{Binding RequestedScrollY, StringFormat='{0:0}'}"/>Fix comparison:
| # | Source | Approach | Test Result | Simplicity | Robustness |
|---|---|---|---|---|---|
| 1 | try-fix (sonnet) | WaitForTextToBePresentInElement in test |
✅ PASS | Simple | High — handles async timing |
| 2 | try-fix (opus) | StringFormat='{0:0}' in XAML binding |
✅ PASS | Very simple | High — eliminates format ambiguity |
| PR | original | Immediate assertion, no StringFormat | Simple | Low — fragile |
Selected Fix: Both Attempt 1 + Attempt 2 — they address independent issues in different files and together make the new tests production-grade.
Additional Notes (No Code Change Required)
- The
Slider_ValueBelowMinimum_EventNotRaised_WithClampedValuetest correctly expects "Not Raised" — when value -10 is clamped to 0 (same as initial),ValueChangeddoes not fire. - The
MaximumEntryentry inSlider_ValueChangedEvent_VerifyArguments_FromOptionsis empty on each Options navigation (fresh page), so entering "5" withoutClearTextis safe. - The apparent "double subscription" (
ValueChangedin XAML +slider.ValueChangedin code-behind) is NOT a real double subscription:ReInitializeSlider()callsSliderGrid.Children.Clear()removing the XAML slider, then adds a fresh programmatic slider. Different objects, no double subscription. - 403 PNG snapshot files are updated — standard for visual regression baseline refresh when HostApp layout changes.
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Testing PR — no linked issue; 8 code files + ~403 PNG snapshots; prior review imported |
| Gate | ✅ PASSED | iOS (iPhone 11 Pro) — new tests pass with PR's changes |
| Try-Fix | ✅ COMPLETE | 2 attempts (+ 2 from prior run), both passing; problem space exhausted |
| Report | ✅ COMPLETE |
Summary
PR #34352 adds Feature Matrix event test cases for Slider (ValueChanged) and ScrollView (ScrollToRequested). This is a pure testing PR — no production code changes. The PR is structurally sound and prior Copilot feedback was addressed. The new tests passed on iOS.
However, the try-fix exploration confirmed two real quality issues that could cause intermittent CI failures:
RequestedScrollX/RequestedScrollYaredoubleproperties withoutStringFormat— tests assert"150"but150.0could render differently on some platformsScrollView_ScrollToRequested_Position_IsCorrectasserts immediately after tap without waiting for the asyncScrollToRequestedevent handler to update the ViewModel
A better fix was found that is simpler and more architecturally correct than the PR's current approach.
Root Cause
Two independent quality issues in new test code:
-
Format ambiguity:
ScrollViewViewModel.RequestedScrollXandRequestedScrollYaredoubleproperties.double150.0 bound withoutStringFormatmay render as"150","150.0", or locale-specific strings depending on platform. The test assertsIs.EqualTo("150"). -
Timing race:
ScrollView_ScrollToRequested_Position_IsCorrecttaps a scroll button and immediately callsApp.FindElement("RequestedPositionLabel").GetText(). SinceScrollToAsyncis animated and theScrollToRequestedevent fires asynchronously, the ViewModel may not be updated yet when the assertion runs.
Fix Quality
Recommended changes (from try-fix Attempt 1 — claude-sonnet-4.6):
Fix 1 — Change RequestedScrollX/RequestedScrollY from double to int in ScrollViewViewModel.cs:
// Before:
private double _requestedScrollX;
public double RequestedScrollX { get => _requestedScrollX; set { ... } }
// After:
private int _requestedScrollX;
public int RequestedScrollX { get => _requestedScrollX; set { ... } }And cast in event handler (ScrollViewControlPage.xaml.cs):
// Before:
vm.RequestedScrollX = e.ScrollX;
vm.RequestedScrollY = e.ScrollY;
// After:
vm.RequestedScrollX = (int)e.ScrollX;
vm.RequestedScrollY = (int)e.ScrollY;Fix 2 — Add sync barrier in ScrollView_ScrollToRequested_Position_IsCorrect (ScrollViewFeatureTests.cs):
// Before:
App.Tap(buttonAutomationId);
Assert.That(App.FindElement("RequestedPositionLabel").GetText(), Is.EqualTo(expectedPosition));
// After:
App.Tap(buttonAutomationId);
App.WaitForTextToBePresentInElement("ScrollToRequestedLabel", "Raised"); // ← sync barrier
Assert.That(App.FindElement("RequestedPositionLabel").GetText(), Is.EqualTo(expectedPosition));Why sync barrier on ScrollToRequestedLabel is better than waiting on RequestedPositionLabel itself:
ScrollToRequestedLabelandRequestedPositionLabelare both set in the sameOnScrollViewScrollToRequestedhandler- Waiting for "Raised" on the event-fired indicator guarantees the position is also set
- Waiting on
RequestedPositionLabeldirectly could false-positive if it already shows the expected value from a prior test run
Fix comparison:
| # | Source | Approach | Result | Simplicity | Style |
|---|---|---|---|---|---|
| 1 | try-fix (sonnet) | int ViewModel + sync barrier |
✅ PASS | ✅ 3 files, ~8 lines | ✅ MVVM preserved |
| 2 | try-fix (opus) | Direct label.Text in code-behind | ✅ PASS | ||
| PR | original | double binding, immediate assertion |
✅ Simple | ✅ MVVM |
Selected Fix: Attempt 1 (claude-sonnet-4.6) — simpler, cleaner, architecturally aligned with MVVM pattern already used in this HostApp.
Additional Observations (No Code Change Required)
Slider_ValueBelowMinimum_EventNotRaised_WithClampedValue: Correctly expects "Not Raised" — when value -10 is clamped to 0 (same as initial),ValueChangeddoes not fire. ✅ Correct behavior.- XAML + code-behind event subscription: Not a double subscription —
ReInitializeSlider()callsSliderGrid.Children.Clear()removing XAML slider before adding programmatic one. SliderViewModal.csfilename: Typo ("Modal" vs "ViewModel") exists but is pre-existing pattern in that directory; minor cleanup.- Indentation in
ScrollView_ScrollToRequested_X_IsNonZero_ForPixelScroll: SecondIs.EqualTo("300")continuation is not indented — minor style issue. - ~403 PNG snapshot files: Updated as expected when HostApp layout changes; standard baseline refresh.
Changes Requested
ScrollViewViewModel.cs— ChangeRequestedScrollX/RequestedScrollYfromdoubletointScrollViewControlPage.xaml.cs— Caste.ScrollX/e.ScrollYtointin event handlerScrollViewFeatureTests.cs— AddApp.WaitForTextToBePresentInElement("ScrollToRequestedLabel", "Raised")inScrollView_ScrollToRequested_Position_IsCorrectbefore assertion
📋 Expand PR Finalization Review
PR #34352 Finalization Review
PR: #34352 — [Testing] Additional Feature Matrix Event Test Cases for Slider and ScrollView
Author: @nivetha-nagalingam
Status: Open · Not Merged · Changes Requested (agent label)
Phase 1: Title & Description
✅ Title: Acceptable — Minor Enhancement Optional
Current: [Testing] Additional Feature Matrix Event Test Cases for Slider and ScrollView
Assessment: Accurate and descriptive. The [Testing] prefix is appropriate since this is a pure testing PR. The title clearly names both controls. No change strictly required.
Optional improvement: [Testing] Feature Matrix: Add event test cases for Slider (ValueChanged) and ScrollView (ScrollToRequested) — more specific about which events.
⚠️ Description: Present but Generic — Enhancements Recommended
Current description:
"This PR enhances the event handling support for multiple MAUI controls by adding comprehensive implementation and validation for control-specific events, along with corresponding test coverage. The update includes the addition of events for Slider and ScrollView controls, ensuring proper event triggering and argument handling across different platforms."
Quality assessment:
- ✅ NOTE block present
- ✅ Covers basic scope
- ❌ Does not name which events were added
- ❌ No breakdown of new test methods or HostApp changes
- ❌ Issues Fixed section empty (acceptable for testing-only PRs)
Suggested description additions:
### Description of Change
Adds Feature Matrix test coverage for events on two controls:
**Slider** (`SliderControlPage`, `SliderFeatureTests`):
- Added `ValueChanged` event tracking to `SliderViewModel` (`ValueChangedStatus`, `OldValue`, `NewValue`)
- Wired `slider.ValueChanged += OnSliderValueChanged` in `ReInitializeSlider()`
- New tests: event not raised initially, arguments verified from Options, clamped-to-maximum args, value-below-minimum not raised
**ScrollView** (`ScrollViewControlPage`, `ScrollViewViewModel`, `ScrollViewFeatureTests`):
- Added `ScrollToRequested` and `Scrolled` event handling
- New `ScrollViewViewModel` properties: `ScrollToRequestedText`, `RequestedScrollX/Y`, `RequestedPosition`, `RequestedAnimate`, `Mode`, `RequestedElementTypeName`
- New tests: event raised, position enum, animated flag, mode (Element vs Position), X/Y coordinates, element type for all content types, pixel-scroll actual vs requested comparison
**Addressed review comments (all 7 resolved):**
- Removed stray `;` in `OnScrollViewScrollToRequested`
- Made `OnScrollToPixelClicked` `async void` and awaited `ScrollToAsync`
- Renamed `Element` property to `RequestedElementTypeName` (typed as `string`)
- Removed unused `using System.Windows.Input` from `ScrollViewViewModel`
- Fixed parameter indentation in `ScrollView_ScrollToRequested_Position_IsCorrect`
- Added `App.WaitForElement` before ScrollToRequested assertions
### Issues Fixed
None — pure test coverage addition.Phase 2: Code Review
🔴 Critical Issues
1. Filename Typo: SliderViewModal.cs should be SliderViewModel.cs
- File:
src/Controls/tests/TestCases.HostApp/FeatureMatrix/Slider/SliderViewModal.cs - Problem: The file is named
SliderViewModal.cs("Modal") but the class inside isSliderViewModel. Every other similar file in the repo uses theViewModelsuffix (e.g.,ScrollViewViewModel.cs). This naming inconsistency makes the file hard to discover and violates naming conventions. - Recommendation: Rename the file to
SliderViewModel.csto match the class name and follow existing conventions.
🟡 Suggestions
2. Missing App.PressEnter() after MaximumEntry in event test
- File:
SliderFeatureTests.cs—Slider_ValueChangedEvent_VerifyArguments_FromOptions - Problem: The test calls
App.EnterText("MaximumEntry", "5")without a subsequentApp.PressEnter(). Later tests (e.g.,Slider_SetMaximumValue_VerifyMinimumLabel) consistently callApp.PressEnter()after entering text. If the OptionsPage uses a keyboard-submit pattern, skippingPressEntercould result in the Maximum not being applied. - Recommendation: Add
App.PressEnter()afterApp.EnterText("MaximumEntry", "5")for consistency and reliability.
3. Test method name mismatch: X test also asserts Y
- File:
ScrollViewFeatureTests.cs—ScrollView_ScrollToRequested_X_IsNonZero_ForPixelScroll - Problem: The test name implies only X is validated, but the test also asserts
RequestedScrollYLabel == "150". Wait — actually it assertsRequestedScrollXLabel == "150"andRequestedScrollYLabel == "300". The method name mentions only X being non-zero, but Y (300) is also asserted. A reader scanning tests would assume this method only checks X. - Recommendation: Rename to
ScrollView_ScrollToRequested_XY_AreNonZero_ForPixelScrollor split into two tests.
4. Inconsistent indentation in pixel scroll test
- File:
ScrollViewFeatureTests.cs—ScrollView_ScrollToRequested_X_IsNonZero_ForPixelScroll - Problem: The second
Assert.That(App.FindElement("RequestedScrollYLabel")...)is indented differently than the first assertion in the same method. - Recommendation: Align both assertions to the same indent level.
5. Missing WaitForElement in horizontal pixel scroll test
- File:
ScrollViewFeatureTests.cs—ScrollView_ScrollToPixel_Horizontal_Updates_RequestedX_And_ScrollX - Problem: The vertical counterpart (
ScrollView_ScrollToPixel_Vertical_Updates_RequestedY_And_ScrollY) correctly callsApp.WaitForElement("ScrollYLabel")before reading scroll values. The horizontal test readsScrollXLabelandScrollYLabelwithout any explicit wait after tapping. Since theScrolledevent is asynchronous, this is inconsistent and potentially flaky. - Recommendation: Add
App.WaitForElement("ScrollXLabel")after tappingScrollToPixelButton, matching the vertical test pattern.
6. App.WaitForElement does not verify label text content
- File:
ScrollViewFeatureTests.cs—ScrollView_ScrollToRequested_Event_IsRaised - Problem:
App.WaitForElement("ScrollToRequestedLabel")was added as the fix for the review comment about flaky assertions. However,WaitForElementonly waits for the element to be present/visible — not for its text to equal "Raised". The element exists before the tap too, so this wait provides minimal timing protection. - Recommendation: Use
App.WaitForTextToBePresentInElement("ScrollToRequestedLabel", "Raised")if available, or poll the text value.
7. Slider_ValueChangedEvent_NotRaised_Initially — test name is slightly misleading
- File:
SliderFeatureTests.cs - Problem: The test navigates to Options, taps Apply, then asserts "Not Raised". The word "Initially" suggests the check happens without user interaction, but the test first visits the Options page (which resets
ValueChangedStatus = "Not Raised") before checking. Technically this tests the reset behavior, not the truly initial state. - Recommendation: Either rename to
Slider_ValueChangedEvent_NotRaised_AfterResetor simplify by checking the label directly without navigating to Options.
✅ Looks Good
- NOTE block present — Required block is at the top of the description.
- Event handler cleanup — Previous review issues resolved: stray
;removed,OnScrollToPixelClickedis nowasync voidwithawait,Elementproperty correctly typed asstring. using System.Windows.InputinSliderViewModal.csis correctly kept —ICommandis used forDragStartedCommandandDragCompletedCommandproperties, so this import is necessary (unlikeScrollViewViewModel.cswhere it was unused).- AutomationId naming — All AutomationIds are clear and consistent (
ScrollToRequestedLabel,RequestedPositionLabel,ValueChangedEventStatus, etc.). - ScrollToRequested tests —
ScrollToRequestedfires synchronously beforeScrollToAsyncanimation begins, so immediate assertions after tap are generally safe for those labels. - Scrolled event tests —
ScrollView_ScrollToPixel_Vertical_Updates_RequestedY_And_ScrollYcorrectly waits forScrollYLabelbefore comparing, handling the async nature of theScrolledevent. [Test, Order(...)]for Slider events — Test ordering makes sense given value state carries across re-renders.- Category attributes — All tests correctly use
[Category(UITestCategories.Slider)]or[Category(UITestCategories.ScrollView)]. - Snapshot updates — Updated ~400 snapshot PNGs indicate the UI changes have been properly baselined.
Summary
| Item | Status |
|---|---|
| NOTE block in description | ✅ Present |
| Title accuracy | ✅ Acceptable |
| Description quality | |
SliderViewModal.cs filename typo |
🔴 Should be SliderViewModel.cs |
Missing PressEnter in Slider event test |
🟡 Potential reliability issue |
| Pixel scroll test name mismatch (X only but asserts Y too) | 🟡 Misleading |
| Missing wait in horizontal pixel scroll test | 🟡 Inconsistent with vertical test |
WaitForElement doesn't check text value |
🟡 Incomplete fix for flakiness |
| Slider events (ValueChanged) — implementation | ✅ Correct |
| ScrollView events (ScrollToRequested) — implementation | ✅ Correct |
| Prior review comments all resolved | ✅ 7/7 resolved |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please resolve conflicts?
6e563dc to
00535e4
Compare
946b3ea to
a1222a8
Compare
@kubaflo , The conflicts have been resolved. |
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
This PR enhances the event handling support for multiple MAUI controls by adding comprehensive implementation and validation for control-specific events, along with corresponding test coverage.
The update includes the addition of events for Slider and ScrollView controls, ensuring proper event triggering and argument handling across different platforms.