-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Testing] Fix for MacCatalyst flaky tests in CI which fails due window position below the dock layer #27279
Conversation
Hey there @anandhan-rajagopal! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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.
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue6784.cs: Evaluated as low risk
- src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/CollectionViewUITests.CollectionViewTabbedPage.cs: Evaluated as low risk
- src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue12652.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/Controls/tests/TestCases.Shared.Tests/UITest.cs:288
- The newly added EnterFullScreen method should be covered by a test case to ensure it functions as expected.
App.EnterFullScreen();
// In CI the window goes to left bottom corner in Catalyst, use full screen mode to prevent dock overlap with UI elements. | ||
App.EnterFullScreen(); | ||
// Wait a little bit to complete the system animation moving the App Window to FullScreen. | ||
Thread.Sleep(500); |
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.
Using Thread.Sleep for waiting can lead to flaky tests. Consider using a more reliable synchronization method.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Something faster (not require to wait to invoke actions, or wait time) would be to just set the Window position in the center of the screen. To do that, can use the CreateWindow method from MauiProgram and set the X
and Y
properties.
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.
@jsuarezruiz We attempted to set X and Y in MauiProgram.cs, but unfortunately, it didn't work and the window did not render at the center on our end. I have attached a video for your reference. As a result, we have decided to run the tests in full-screen mode to address this issue. Additionally, there are a few cases that may fail due to this behavior, particularly those involving bottom tabs. Could you please take a look and share your suggestions? @MartyIX any thoughts on this?
Screen.Recording.2025-01-22.at.10.29.13.PM.mov
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.
@MartyIX any thoughts on this?
I sort of think that window.X and window.Y is set "too soon" after the app starts and Mac Catalyst does not pick it up. I would try to postpone it a bit (say 2 seconds) to verify this idea. It looks like a bug to me.
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.
Yes, good point. I look forward to the feedback!
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.
In .NET MAUI sample gallery, there is: #24001 (comment) where one can test moving a window to the center of one's screen.
I would check it out, if it works there and if it does then it's just a matter of "how to wait properly" (e.g. one can possibly use Dispatcher with a timeout of a few seconds).
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.
I don't know if it is in the scope of your work. Anyway, I would find it very useful to figure this one out.
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.
Thanks @MartyIX, @jsuarezruiz using dispatcher actually works in this case. commited the modified the code changes can you please review and share your concerns.
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.
If we want to ultimately achieve this without the delay, then it is necessary to figure out why it takes time until this method:
is called. I think that the reaon might be that
requires to use
-NSKeyValueObservingOptions.OldNew
+NSKeyValueObservingOptions.Initial | NSKeyValueObservingOptions.OldNew
but it's just a hunch. If we don't want to pursue this, then I believe we should have an issue for this.
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.
@MartyIX & @jsuarezruiz, At present, issue has been created to conduct further investigation and resolve this matter: #27304.
// In CI the window goes to left bottom corner in Catalyst, use full screen mode to prevent dock overlap with UI elements. | ||
App.EnterFullScreen(); | ||
// Wait a little bit to complete the system animation moving the App Window to FullScreen. | ||
Thread.Sleep(500); |
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.
Something faster (not require to wait to invoke actions, or wait time) would be to just set the Window position in the center of the screen. To do that, can use the CreateWindow method from MauiProgram and set the X
and Y
properties.
// Setting X and Y without delay doesn't work on Catalyst | ||
window.Dispatcher.DispatchDelayed(TimeSpan.FromMilliseconds(200), () => | ||
{ | ||
window.X = (screenWidth - desktopWindowWidth) / 2; |
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.
For Mac Catalyst, it should not be needed to set:
window.MaximumWidth = desktopWindowWidth;
window.MinimumWidth = desktopWindowWidth;
window.MaximumHeight = desktopWindowHeight;
window.MinimumHeight = desktopWindowHeight;
you can just set window.Width = desktopWindowWidth
and window.Height = desktopWindowHeight
here in window.Dispatcher.DispatchDelayed
below IMHO.
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.
@MartyIX , Updated the changes in latest commit.
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.
Thank you
/rebase |
b33048f
to
2468680
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
…l/maui into mac_flakyTests
@jsuarezruiz Unfortunately, updating the x and y values in the create window method with a delay can lead to some UI elements disappearing at intial page in CI, although this issue doesn't occur locally. Here attached the images from the attachments for the failed test cases, Could you take a look and share your thoughts on this? ![]() |
…cher seems updating height and width with delay will leads to disaapearing some UI elements in CI, so reverted the latest changes.
Sure, the test to verify is NavigatingBackToAlreadySelectedTopTabDoesntCrash, right? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Not only this specific tests, all the test cases are failed with time out exception for GotoTestCases button which in initial page on Catalyst. you can refer to the previous build result for reference (https://dev.azure.com/xamarin/public/_build/results?buildId=133414&view=ms.vss-test-web.build-test-results-tab) |
Could you try Appium to set the Window Position?
|
@jsuarezruiz It seems that setting the window position in the Catalyst driver is not supported, resulting in a System.NotImplemented exception. However, in the latest commit, I reverted the width and height values in the dispatcher, which resolved the issue with the disappearing UI elements. Could you please review the current changes and let me know your concerns on it. |
Description
This PR addresses an issue with flaky tests in MacCatalyst when running in CI environments. The problem occurs because the application window sometimes positions itself in the left bottom corner, potentially overlapping with the dock and causing UI elements to be obscured or inaccessible. (Most of the cases are specifically for bottom tabs)
Ref: https://dev.azure.com/xamarin/public/_build/results?buildId=133100&view=ms.vss-test-web.build-test-results-tab&runId=3631844&resultId=100106&paneView=attachments
Solution