Skip to content
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

Consolidate handlertestbase #11021

Merged
merged 17 commits into from
Nov 7, 2022
Merged

Consolidate handlertestbase #11021

merged 17 commits into from
Nov 7, 2022

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Oct 31, 2022

Description of Change

  • Currently our FrameRenderer doesn't call any of the initial mapper properties that run through the batch update. All of the tests for this behavior currently exists inside Core.DeviceTest on the base handler. The intent of this PR is to move all of that testing code into a re-usable place so that FrameRenderer (and other handlers inside Controls) can utilize the tests we have in Core.DeviceTest to validate that all the default ViewHandler mappers. This PR also consolidates a bunch of helper methods that were duplicated between Core/Controls

  • I moved all of the code shared between Controls/Core.DeviceTests to Core.DeviceTests.Shared. These don't seem like things we'd ever really package up and distribute. They are a bit specific to our tests. Anything we'd want to package up we should move to TestUtils and AssertionExtensions

  • I tried to rename anything that was a repeated class name. So, now there's only one HandlerTestBase, ContextStubBase, etc..

  • This PR doesn't actually fix Frame yet it's mainly just the reorg part of the tests so that I can fix Frame and add tests here https://github.com/dotnet/maui/pull/10751/files#diff-ad087c312304a6bb9d0b8d53ab57e6d941c1fa937885ad9f10922b1734f2f0d0R118

  • Moved all the ITextStyle tests out of HandlerTestBase and into a specific TextStyleHandlerBase so now instead of just having a bunch of noop tests related to TextStyle they all just run for interfaces that implement ITextStyle

@PureWeen
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Eilon Eilon added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label Nov 1, 2022
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions.

src/Core/src/Handlers/View/ViewHandler.cs Outdated Show resolved Hide resolved
src/Core/src/Handlers/ViewHandlerExtensions.Android.cs Outdated Show resolved Hide resolved
src/Core/src/Handlers/View/ViewHandler.cs Outdated Show resolved Hide resolved
@@ -150,52 +150,7 @@ public async Task FontDoesNotAffectHorizontalTextAlignment(double initialSize, d
nameof(ILabel.Font),
() => label.Font = Font.SystemFontOfSize(newSize));
}
#if !WINDOWS
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved these all into TextStyleHandlerTests since they apply to anything that implements ITextStyle

@PureWeen PureWeen requested a review from hartez November 1, 2022 22:18
@PureWeen PureWeen marked this pull request as ready for review November 1, 2022 22:21
@PureWeen PureWeen requested a review from a team as a code owner November 1, 2022 22:21
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for the BlazorWebView device test changes.

var w = size.Width;
var h = size.Height;

if (double.IsPositiveInfinity(w))
Copy link
Member Author

@PureWeen PureWeen Nov 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartez I added this code because all of the ScrollView tests started failing due to view.Measure returning infinite size.

I'm wondering if this is actually a bug with ScrollViewHandler.

The GetDesiredSize code inside ScrollViewHandler just returns the constraints back if the ScrollView has no content (which makes sense)

https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L226-L229

I wonder if for this scenario the ScrollViewHandler on iOS should return a size of zero if the content is null and the constraints are infinite.

Something like

if (platformView == null || virtualView == null || virtualView.PresentedContent == null)
{
     if (double.IsPositiveInfinity(widthConstraint))
          widthConstraint = 0;
          
     if (double.IsPositiveInfinity(heightConstraint))
          heightConstraint= 0;
          
     return new Size(widthConstraint, heightConstraint);
}

If this does seem like something we should fix on ScrollViewHandler I'll do that in a different PR. I'd like this PR to not touch any actual sdk code just test code.

@hartez hartez merged commit 6dc3d24 into main Nov 7, 2022
@hartez hartez deleted the consolidate_handlertestbase branch November 7, 2022 15:57
@hartez
Copy link
Contributor

hartez commented Nov 17, 2022

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/3489791409

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants