[release/11.0.1xx-preview7] De-flake DragEvents UI test assertions - #37045
[release/11.0.1xx-preview7] De-flake DragEvents UI test assertions#37045kubaflo wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9be49656-7117-4235-9d96-404779ab6b16
|
/azp run |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37045Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37045" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR aims to de-flake the DragEvents UI test by replacing immediate text reads/assertions with a helper that waits for each drag/drop event label to update before asserting exact text.
Changes:
- Refactors per-event assertions in
DragEvents()into a sharedAssertEventTexthelper. - Introduces a wait step prior to asserting each event label’s final text value.
…sitive AssertEventText waited with a Contains-based substring match, but each event label's placeholder already contains the expected value (e.g. "DragOverEvents: " contains "DragOver", "DropEvents: " contains "Drop"), so the wait passed immediately on the placeholder and the following equality assertion could still flap. Add a WaitForTextEqualToElement helper that waits for the text to become exactly the expected value (ordinal) and use it here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/TestUtils/src/UITest.Appium/HelperExtensions.cs:1114
WaitForTextEqualToElementusesretryFrequency.Millisecondswhen delaying between retries.TimeSpan.Millisecondsis only the milliseconds component (0-999), so this becomes incorrect if the retry frequency is ever changed to >= 1s (or if this code is copy/pasted elsewhere). Use theTask.Delay(TimeSpan)overload to avoid this pitfall.
Task.Delay(retryFrequency.Milliseconds).Wait();
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:52
AssertEventTextalready waits until the label text is exactly the expected value (and the helper uses the same underlyinggetTextcommand). The follow-upGetText()call is redundant and can still introduce flakiness if Appium briefly errors or the accessibility snapshot changes again between the wait and the assertion. Consider relying on the wait assertion alone.
Assert.That(
App.WaitForTextEqualToElement(automationId, expectedText),
Is.True,
$"Timed out waiting for {automationId} to become '{expectedText}'.");
Assert.That(App.FindElement(automationId).GetText(), Is.EqualTo(expectedText));
- Use the Task.Delay(TimeSpan) overload in WaitForTextEqualToElement instead of TimeSpan.Milliseconds (which is only the 0-999 component). - Drop the redundant GetText re-read in AssertEventText; the exact-text wait already asserts the value, and the extra read only re-opened a window for transient Appium flakiness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
Addressed the two suppressed review nits in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/TestUtils/src/UITest.Appium/HelperExtensions.cs:1115
WaitForTextEqualToElementusesDateTime.Nowto measure elapsed time andTask.Delay(...).Wait()in a tight polling loop. Using wall-clock time can hang if the system clock moves backwards (NTP/time sync), andTask.Delay().Wait()allocates aTaskeach iteration. Consider switching toStopwatchfor elapsed time andThread.Sleepfor the polling delay (consistent with other wait helpers in this file).
timeout ??= DefaultTimeout;
TimeSpan retryFrequency = TimeSpan.FromMilliseconds(500);
DateTime start = DateTime.Now;
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:48
- The comment says the wait "already asserts" the exact text, but
WaitForTextEqualToElementreturns abool(it checks equality; theAssert.That(...)is what asserts). Tweaking the wording avoids confusion when someone reads this test later.
// Wait for the label's text to become EXACTLY the expected value. A substring wait is
// unreliable here because each label's placeholder (e.g. "DragOverEvents: ") already
// contains the expected event name (e.g. "DragOver"), so a Contains-based wait would pass
// immediately on the placeholder. The wait already asserts the exact text, so no separate
// GetText re-read is needed (it would only re-open a window for transient Appium flakiness).
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review
Three findings survived consensus: two testing-reliability warnings and one maintainability suggestion. The exact-text comparison itself is correct: the host pages replace each placeholder with the expected bare event name, and each poll re-queries the accessibility element rather than caching it.
Methodology: 3 independent reviewers with adversarial consensus + repo domain specialist.
Test coverage: The changed DragEvents path directly exercises the new helper, but the equivalent sibling drag/drop test retains the same racy immediate-read pattern.
Prior review status: Earlier feedback about substring matching, TimeSpan.Milliseconds, and the redundant text re-read is addressed in the current head.
|
/azp run maui-pr-uitests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
… test Addresses reviewer feedback on the DragEvents wait helper: - Extract a shared predicate-based WaitForText core so WaitForTextToBePresentInElement (contains) and WaitForTextEqualToElement (exact) can no longer drift apart; this also fixes the older helper's Task.Delay(int) millisecond overload to use the TimeSpan overload. - Surface the last observed text (and expected value) in the timeout diagnostic so a placeholder-stuck/stalled label is distinguishable from a text-read failure. - Reuse the exact-text wait for the four event labels in DragAndDropBetweenLayouts, which previously waited only for element existence and could observe the placeholder on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen thanks for the thorough pass — addressed all three points in ae51188:
Ready for another look 🙏 |
|
/azp run maui-pr-uitests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:75
- After waiting with WaitForTextEqualToElement, the test re-queries the element text and asserts again. That extra GetText can reintroduce the same transient Appium/accessibility-tree flakiness this PR is trying to remove, and it’s redundant now that AssertEventText already handles the wait + assertion with a clear timeout message. Consider replacing this whole block with AssertEventText(...).
This issue also appears in the following locations of the same file:
- line 83
- line 94
- line 116
App.WaitForTextEqualToElement("DragStartEventsLabel", "DragStarting");
var textAfterDragStart = App.FindElement("DragStartEventsLabel").GetText();
if (string.IsNullOrEmpty(textAfterDragStart))
{
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:87
- Same pattern as above: WaitForTextEqualToElement already proves the label text is exactly the expected value. Re-reading with GetText and re-asserting is redundant and can reintroduce flakiness. Replace this block with AssertEventText(...).
App.WaitForTextEqualToElement("DragOverEventsLabel", "DragOver");
var textAfterDragOver = App.FindElement("DragOverEventsLabel").GetText();
if (string.IsNullOrEmpty(textAfterDragOver))
{
Assert.Fail("Text was expected: Drag over event");
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:98
- Same issue: the wait already guarantees the expected text; the follow-up GetText/if/else is redundant and can cause flakiness if the second read returns a stale value. Replace the block with AssertEventText(...).
App.WaitForTextEqualToElement("DragCompletedEventsLabel", "DropCompleted");
var textAfterDragComplete = App.FindElement("DragCompletedEventsLabel").GetText();
if (string.IsNullOrEmpty(textAfterDragComplete))
{
Assert.Fail("Text was expected: Drag complete event");
src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs:120
- Same pattern: once WaitForTextEqualToElement is used, re-fetching the label text is redundant and risks a transient mismatch. Replace with AssertEventText(...).
App.WaitForTextEqualToElement("DropEventsLabel", "Drop");
var textAfterDrop = App.FindElement("DropEventsLabel").GetText();
if (string.IsNullOrEmpty(textAfterDrop))
{
Assert.Fail("Text was expected: Drop event");
|
@PureWeen this is now proven at the current head |
|
Consolidated into #37081, which is now the single rebased test-only stabilization PR for Preview 7. |
<!-- 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 Bundles independently valid Preview 7 test and test-infrastructure stabilizations into one merge path. Changes are cherry-picked or extracted from their source PRs without modifying MAUI runtime or product behavior. The aggregate currently covers: - exact DragEvents label polling and shared Appium text-wait behavior; - waiting for the iOS Entry keyboard before its snapshot; - adaptive ListView and CollectionView screenshot stabilization; - RefreshView gesture retry and SwipeView callback-result waiting; - stable initial SafeArea layout measurement; - deterministic iOS Picker-to-Entry keyboard transition handling; - a bounded Android screenshot tolerance for stable post-rotation antialiasing variance; - independent iOS and MacCatalyst Graphics image-scaling device coverage; - a bounded UIKit cleanup wait for the CarouselView leak assertion; - Android-only scoping for the Android modal-animation leak regression test; - an exact detached-page transition with a synchronous alert-request return signal; and - resilient Windows test-machine resolution setup, with focused Pester coverage for each fallback and failure path. Eligible UI tests, device tests, unit tests, test infrastructure, and screenshot baselines can all be included here when they are independently valid without a corresponding product-code change. ### Source PRs The standalone source PRs below are closed in favor of this aggregate. For #37057, only the independently valid test subset is included; its product-code rewrite remains excluded. | PR | Included test-side fix | |---|---| | #37044 | Windows screen-resolution test infrastructure and Pester coverage | | #37045 | DragEvents exact-text waits and Appium helper | | #37053 | iOS Entry keyboard readiness | | #37055 | ListView screenshot retry window | | #37057 | Graphics image-scaling device tests that pass without the runtime rewrite | | #37058 | RefreshView pull-to-refresh retry | | #37059 | SwipeView result-label wait | | #37066 | SafeArea initial-layout retry | | #37069 | Picker keyboard transition stabilization | | #37074 | Empty CollectionView footer screenshot retry/tolerance | | #37086 | iOS/MacCatalyst CarouselView leak-test cleanup wait | | #37088 | Detached-page alert request synchronization | | #37091 | Android modal-animation leak-test platform scope | | #37092 | Android Issue22306 post-rotation screenshot tolerance | ### Scope exclusions Mixed runtime/test fixes are included only when their test-side changes pass independently against the unmodified product code. The current device tests in #37052, the four nonpositive-size cases in #37057, and the device test plus screenshots in #37062 remain excluded because they expose or describe behavior that requires those PRs' functional fixes. #37070 has no test-side changes. Closed ineffective or unsafe fixes are also excluded. ### Validation - All 25 directly reusable source commits were cherry-picked in their original order. - The #37057 device-test subset was extracted into one additional test-only commit after proving it against the unmodified product implementation. - Every directly cherry-picked aggregate file matches the corresponding included source PR head. - The aggregate diff contains no MAUI runtime or product files. - Targeted Release builds pass for `UITest.Appium`, `Controls.TestCases.iOS.Tests`, `Controls.TestCases.Mac.Tests`, and `Controls.TestCases.Android.Tests`. - The focused screen-resolution Pester suite passes 14/14. - Exact local xUnit XML reports 46/46 Graphics device tests passing on both iOS 26 and MacCatalyst, including all 13 extracted cases. - The Graphics deadlock regression now asserts the bounded five-second completion result before awaiting the scaling task. - The CarouselView category passes 6 consecutive MacCatalyst runs at 4/4 each and passes 4/4 on iOS 26 with the final 100 ms cleanup wait. - The Android modal-animation leak test passes in both discovered Android variants, while the iOS device-test assembly excludes that Android-specific regression test. - The Issue22306 Android failure's three retries were byte-for-byte identical at a 1.85% visual difference, below the new Android-only 2% tolerance. - The unloaded-page alert probe now signals immediately after `DisplayAlertAsync` returns, avoiding a teardown-order dependency on the detached page task. - Rebased the aggregate onto release head `dee83edd121`; the resulting diff remains limited to tests and test infrastructure. - The Pester workflow now triggers when `eng/scripts/Set-ScreenResolution.ps1` changes. - A standalone `/azp run` was posted for aggregate head `83da465424b`; exact merge `bad9e4e9a57` builds `1539860`, `1539861`, and `1539862` are required before merge. ### Issues Fixed Contributes to stabilizing the .NET 11 Preview 7 test branch. --------- Co-authored-by: Vally Fixture <vally-fixture@example.invalid> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: kubaflo <34349119+kubaflo@users.noreply.github.com> Copilot-Session: 9be49656-7117-4235-9d96-404779ab6b16 Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
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
The Windows
DragEventsUI test can read event-label text before queued drag/drop event updates are reflected in the accessibility tree. Build 1537359 observedDragOverEvents:instead ofDragOver, while the retry passed.Wait for each expected event value before asserting its exact text. This preserves coverage for all four drag/drop events without adding sleeps or broad test retries.
Validation
dotnet format Microsoft.Maui.sln --no-restore --include src/Controls/tests/TestCases.Shared.Tests/Tests/DragAndDropUITests.cs --exclude Templates/src --exclude-diagnostics CA1822git diff --check