Fix Disabling some image tests after updating net9.0 with main #25770

devanathan-vaithiyanathan · 2024-11-11T14:26:11Z

Description of Change

After merging the net9 branch into main, two UI tests were failing:
1. Android Test: ImageDoesntDisappearWhenNavigatingBack

After merging the net9 branch into main, this test began passing successfully without additional modifications.

2. Windows Test: Issue31366PushingWithModalStackCausesIncorrectStackOrder

To resolve this test, I made adjustments in the HostApp sample, which contains two buttons. Initially, after clicking the first button, the sample displayed an empty white screen, blocking access to the second button. This issue caused the test to fail with a WaitForElement exception, as the second button was not visible.
I added a delay, ensuring the second button becomes visible and clickable, which resolved the issue and allowed the test to pass as expected.

Issues Fixed

Fixes #24243

Output Screenshot

Android:

Windows

dotnet-policy-service · 2024-11-11T14:26:30Z

Hey there @devanathan-vaithiyanathan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

jsuarezruiz · 2024-11-11T15:26:10Z

/azp run

azure-pipelines · 2024-11-11T15:26:30Z

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz · 2024-11-15T08:32:18Z

/azp run

azure-pipelines · 2024-11-15T08:32:39Z

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho · 2024-11-18T03:01:42Z

/rebase

jsuarezruiz · 2024-11-18T12:27:22Z

/azp run

azure-pipelines · 2024-11-18T12:27:44Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen

I added a delay, ensuring the second button becomes visible and clickable, which resolved the issue and allowed the test to pass as expected.

Can we add a WaitForElement on the second button opposed to just adding delays?

jsuarezruiz · 2024-11-21T12:55:25Z

src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31366.cs

@@ -32,7 +32,7 @@ public MainPage()

 				popOnAppearing.Appearing += async (sender, args) =>
 				{
-					await Task.Yield();
+					await Task.Delay(50);


Could just include a View, like a Label, and wait for it?

In the test case host app, a new page is being pushed and then immediately popped on the page's Appearing event. As a result, the page's UI does not load, making it impossible to validate views like labels. Here, added a slight delay before popping the page in the Appearing event to avoid exception.

What was the exception? Adding a delay to fix the issue may mean something else went wrong.

Comments addressed

PureWeen · 2025-01-06T09:55:34Z

/azp run

azure-pipelines · 2025-01-06T09:55:56Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen · 2025-01-06T09:57:15Z

src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31366.cs

@@ -32,7 +32,7 @@ public MainPage()

 				popOnAppearing.Appearing += async (sender, args) =>
 				{
-					await Task.Yield();
+					await Task.Delay(50);


Could we wire into an event on the page we're popping back to and add a label?

like

push

pop from appearing

and then incremented a counter or inside "OnNavigatedTo" we could add a label indicating it was popped back to?

The cause of the issue is performing the pop in the Page.Appearing event. As an alternative, I tried this approach:

This checks the Modal stack before popping, similar to a push-then-pop pattern. Can we proceed with this approach?

Maybe this should be a different test case? If there is a problem popping from Appearing, do we want to fix that of just say "don't do that"?

We can still use your idea of a label update because the NavigatedTo will fire again on the first/initial page? When you pop the new page, we can increment the counter.

Also, if this is because of animations, then we will have to document that you can't pop in Appearing while pushing.

mattleibow

Adding a delay might be changing the test here. It seems that the test may have been testing the logic of popping in the Appearing? So, we may have to see if that is the main issue and maui has a bug.

Or, maybe this is not a supported operation? We can only pop in some Appeared and/or NavigatedTo. Either way, this is either a bug in maui or just a case of the Windows limitations and we need to document that it is not supported.

I assume it was working until @PureWeen fixed some navigation issue and events. Since we like those changes, we should see if there needs to be some fine tuning or indicate that removing a page while it is still coming in is not really supported.

Or we can rather queue the navigation operation until the page is settled?

mattleibow · 2025-01-10T09:54:22Z

src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31366.cs

@@ -32,7 +32,7 @@ public MainPage()

 				popOnAppearing.Appearing += async (sender, args) =>
 				{
-					await Task.Yield();
+					await Task.Delay(50);


What was the exception? Adding a delay to fix the issue may mean something else went wrong.

mattleibow · 2025-01-10T09:56:10Z

src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31366.cs

@@ -32,7 +32,7 @@ public MainPage()

 				popOnAppearing.Appearing += async (sender, args) =>
 				{
-					await Task.Yield();
+					await Task.Delay(50);


Maybe this should be a different test case? If there is a problem popping from Appearing, do we want to fix that of just say "don't do that"?

We can still use your idea of a label update because the NavigatedTo will fire again on the first/initial page? When you pop the new page, we can increment the counter.

mattleibow · 2025-01-10T09:57:21Z

src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31366.cs

@@ -32,7 +32,7 @@ public MainPage()

 				popOnAppearing.Appearing += async (sender, args) =>
 				{
-					await Task.Yield();
+					await Task.Delay(50);


Also, if this is because of animations, then we will have to document that you can't pop in Appearing while pushing.

mattleibow · 2025-01-10T10:02:16Z

@devanathan-vaithiyanathan if the Windows test is going to take some time, maybe just move the Android fix into a new PR and we can approve faster and then this PR can become a long discussion on the Appearings.

devanathan-vaithiyanathan · 2025-01-20T05:11:15Z

@devanathan-vaithiyanathan if the Windows test is going to take some time, maybe just move the Android fix into a new PR and we can approve faster and then this PR can become a long discussion on the Appearings.

The android fix has been added in the PR and it has been merged.

PureWeen · 2025-01-23T21:36:27Z

Adding a delay might be changing the test here. It seems that the test may have been testing the logic of popping in the Appearing? So, we may have to see if that is the main issue and maui has a bug.

Or, maybe this is not a supported operation? We can only pop in some Appeared and/or NavigatedTo. Either way, this is either a bug in maui or just a case of the Windows limitations and we need to document that it is not supported.

I assume it was working until @PureWeen fixed some navigation issue and events. Since we like those changes, we should see if there needs to be some fine tuning or indicate that removing a page while it is still coming in is not really supported.

Or we can rather queue the navigation operation until the page is settled?

Agreed, the original test is a little interesting because it always had the yield it seems
https://github.com/xamarin/Xamarin.Forms/blob/2f8f4864a4d289dc89a6228e2ca9d6a49993e365/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla31366.cs#L42

so it was never really testing doing the operation from "Appearing" correctly

But we really shouldn't modify the hostapp to make a test pass in this way

devanathan-vaithiyanathan · 2025-01-24T13:42:57Z

Adding a delay might be changing the test here. It seems that the test may have been testing the logic of popping in the Appearing? So, we may have to see if that is the main issue and maui has a bug.
Or, maybe this is not a supported operation? We can only pop in some Appeared and/or NavigatedTo. Either way, this is either a bug in maui or just a case of the Windows limitations and we need to document that it is not supported.
I assume it was working until @PureWeen fixed some navigation issue and events. Since we like those changes, we should see if there needs to be some fine tuning or indicate that removing a page while it is still coming in is not really supported.
Or we can rather queue the navigation operation until the page is settled?

Agreed, the original test is a little interesting because it always had the yield it seems https://github.com/xamarin/Xamarin.Forms/blob/2f8f4864a4d289dc89a6228e2ca9d6a49993e365/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla31366.cs#L42

so it was never really testing doing the operation from "Appearing" correctly

But we really shouldn't modify the hostapp to make a test pass in this way

@PureWeen When using Task.Yield, the page does not pop properly during the appearing event, and a blank screen appears. We have logged a separate issue report for this. Shall we proceed by moving this test case with Task.Delay as a temporary fix, or should we close this PR and move the test case once the reported issue is resolved?

PureWeen · 2025-02-03T22:07:09Z

@PureWeen When using Task.Yield, the page does not pop properly during the appearing event, and a blank screen appears. We have logged a separate issue report for this. Shall we proceed by moving this test case with Task.Delay as a temporary fix, or should we close this PR and move the test case once the reported issue is resolved?

Yea, I don't think adding Bugzilla31366 with the awaits provides any value.
So let's just wait to add that one

devanathan-vaithiyanathan added 3 commits October 25, 2024 17:02

attribute removed

abc472a

conflicts resolved

29e40f7

added delay in windows test failing issue

e623d6c

dotnet-policy-service bot added the community ✨ Community Contribution label Nov 11, 2024

mattleibow added the area-testing Unit tests, device tests label Nov 11, 2024

attributes removed

7b58624

reduce the delay value

14f7c66

devanathan-vaithiyanathan marked this pull request as ready for review November 15, 2024 15:14

devanathan-vaithiyanathan requested a review from a team as a code owner November 15, 2024 15:14

devanathan-vaithiyanathan requested review from mattleibow and PureWeen November 15, 2024 15:15

PureWeen previously requested changes Nov 18, 2024

View reviewed changes

jsuarezruiz requested changes Nov 21, 2024

View reviewed changes

sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024

Merge branch 'main' into fix-24243

225cca6

PureWeen added this to the .NET 9 SR4 milestone Jan 5, 2025

PureWeen requested changes Jan 6, 2025

View reviewed changes

mattleibow requested changes Jan 10, 2025

View reviewed changes

PureWeen modified the milestones: .NET 9 SR4, .NET 9 SR5 Jan 23, 2025

PureWeen modified the milestones: .NET 9 SR5, .NET 9 SR4 Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Disabling some image tests after updating net9.0 with main #25770

Fix Disabling some image tests after updating net9.0 with main #25770

devanathan-vaithiyanathan commented Nov 11, 2024 •

edited

Loading

dotnet-policy-service bot commented Nov 11, 2024

jsuarezruiz commented Nov 11, 2024

azure-pipelines bot commented Nov 11, 2024

jsuarezruiz commented Nov 15, 2024

azure-pipelines bot commented Nov 15, 2024

rmarinho commented Nov 18, 2024

jsuarezruiz commented Nov 18, 2024

azure-pipelines bot commented Nov 18, 2024

PureWeen left a comment

jsuarezruiz Nov 21, 2024

devanathan-vaithiyanathan Nov 27, 2024

mattleibow Jan 10, 2025

PureWeen commented Jan 6, 2025

azure-pipelines bot commented Jan 6, 2025

PureWeen Jan 6, 2025

devanathan-vaithiyanathan Jan 8, 2025

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

mattleibow left a comment

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

mattleibow Jan 10, 2025

mattleibow commented Jan 10, 2025

devanathan-vaithiyanathan commented Jan 20, 2025

PureWeen commented Jan 23, 2025

devanathan-vaithiyanathan commented Jan 24, 2025

PureWeen commented Feb 3, 2025

Fix Disabling some image tests after updating net9.0 with main #25770

Are you sure you want to change the base?

Fix Disabling some image tests after updating net9.0 with main #25770

Conversation

devanathan-vaithiyanathan commented Nov 11, 2024 • edited Loading

Description of Change

Issues Fixed

Output Screenshot

dotnet-policy-service bot commented Nov 11, 2024

jsuarezruiz commented Nov 11, 2024

azure-pipelines bot commented Nov 11, 2024

jsuarezruiz commented Nov 15, 2024

azure-pipelines bot commented Nov 15, 2024

rmarinho commented Nov 18, 2024

jsuarezruiz commented Nov 18, 2024

azure-pipelines bot commented Nov 18, 2024

PureWeen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureWeen commented Jan 6, 2025

azure-pipelines bot commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattleibow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattleibow commented Jan 10, 2025

devanathan-vaithiyanathan commented Jan 20, 2025

PureWeen commented Jan 23, 2025

devanathan-vaithiyanathan commented Jan 24, 2025

PureWeen commented Feb 3, 2025

devanathan-vaithiyanathan commented Nov 11, 2024 •

edited

Loading