-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Testing] Fix Bugzilla41842 test by replacing raw Page with ContentPage #31862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fixes the previously crashing Bugzilla41842 UI test by replacing invalid base Page() instantiations with ContentPage(), and re-enabling the test. Key adjustments include updating the HostApp sample to use valid ContentPage instances and reactivating the shared test with a platform-specific assertion.
- Replace raw Page() usages with ContentPage() to avoid crashes when assigning Flyout/Detail multiple times.
- Reactivate previously ignored test and add platform conditional assertion.
- Add AutomationIds to validate behavior in UI test.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla41842.cs | Replaces Page() with ContentPage(), adds labeled content and AutomationIds for validation. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Bugzilla/Bugzilla41842.cs | Re-enables test, adds platform-specific WaitForElement logic. |
| #if MACCATALYST | ||
| App.WaitForElement("FlyoutLabel"); | ||
| #else | ||
| App.WaitForElement("Success"); | ||
| #endif |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The platform conditional causes different assertions without any in-code explanation; adding a brief comment (e.g., why MacCatalyst cannot validate the Detail label) would improve clarity for future maintainers.
| #if MACCATALYST | ||
| App.WaitForElement("FlyoutLabel"); | ||
| #else | ||
| App.WaitForElement("Success"); | ||
| #endif |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On MacCatalyst the test only waits for the Flyout label and never validates that the second Detail assignment (the core of the regression scenario) succeeded; this reduces coverage of the original issue on that platform. Consider asserting both that no crash occurred and that the Detail 'Success' label is eventually present (or document why it cannot be).
| { | ||
| } | ||
|
|
||
| public override string Issue => "Set FlyoutPage.Detail = New Page() twice will crash the application when set FlyoutLayoutBehavior = FlyoutLayoutBehavior.Split"; |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Issue description still references 'New Page()' even though the test now uses ContentPage instances; updating the wording to match the current scenario will avoid confusion.
| public override string Issue => "Set FlyoutPage.Detail = New Page() twice will crash the application when set FlyoutLayoutBehavior = FlyoutLayoutBehavior.Split"; | |
| public override string Issue => "Set FlyoutPage.Detail = new ContentPage() twice will crash the application when set FlyoutLayoutBehavior = FlyoutLayoutBehavior.Split"; |
| public void Bugzilla41842Test() | ||
| { | ||
| #if MACCATALYST | ||
| App.WaitForElement("FlyoutLabel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is passing on CI.
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
…ge (#31862) * Modified the test * Modified the test case
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!
Root Cause
The Bugzilla41842 test was using new Page() which is a base class and should not be instantiated directly. This caused crashes.
Description of Change
Fixed the failing Bugzilla41842 test case by replacing raw Page() instantiations with proper ContentPage() objects.
Validated the behavior in the following platforms
Issues Fixed
Fixes #21205
Output ScreenShot
BeforeFix.mov
AfterFix.mov