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

Tests: Add support for connected component analysis #16770

Merged
merged 9 commits into from
Aug 22, 2023

Conversation

dotMorten
Copy link
Contributor

@dotMorten dotMorten commented Aug 15, 2023

Description of Change

This introduces the ability to analyze pixels as a set of connected pixels, allowing you to measure the size and location of UI elements, independent of device, screen size and DPI.
It will help ensure consistent layout across all supported .NET MAUI platforms.

A quick introduction to the algorithm used can be found here: https://www.youtube.com/watch?v=ticZclUYy88

Issues Fixed

While this specific issue doesn't actually fix any issue, the two included example tests does verify regressions and fixes.
The border test specifically is currently failing on Windows due to #16667 and on Android it finds the border to be too thin (it was made to validate #15339 and shows the result is closer to correct, but still needs tweak -cc @jstedfast ).

This is the initial PR submitted, as discussed in a recent meeting with @BretJohnson @PureWeen @mattleibow and @jstedfast

Please let me know what you'd like to change/improve/remove and I'd be more than happy to work with you on getting it to a point where you can start using it.

A few things that could be improved:

  • Replace RawBitmap with the code already there for grabbing screenshots of VisualElements (will need support for getting the entire pixel buffer in a consistent byte order)
  • One of the unit tests exposes an issue on iOS where the grid renders under the safe-area, but doesn't adjust rows for it, however I wasn't able to get the specific test UI elements to scale to the entire screen, so hard to hardcode a fixed height or the test would fail on all platforms (unspecified width/height not supported)
  • Disable the two tests for now, since they don't actually pass on all platforms just yet due to other bugs.

Fails on Windows due to #16667:
image

Fails on Android because border is too thin:
image

iOS passed (but second would fail if we could expand the view to the full screen, since safe area affects outcome):
image

@dotMorten dotMorten requested a review from a team as a code owner August 15, 2023 23:44
@ghost ghost added the community ✨ Community Contribution label Aug 15, 2023
@ghost
Copy link

ghost commented Aug 15, 2023

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

@samhouts
Copy link
Member

Ooooooh!

@samhouts samhouts added the area-testing Unit tests, device tests label Aug 15, 2023
@jfversluis
Copy link
Member

/azp run MAUI-UITests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho
Copy link
Member

/azp run MAUI-DeviceTests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jstedfast
jstedfast previously approved these changes Aug 16, 2023
@jstedfast
Copy link
Member

BorderAndStrokeIsCorrectSize() is failing on Android API 30 and 23:

Assert.Equal() Failure: Values are not within tolerance 2\nExpected: 180\nActual: 189.95238095238093

Issue15330() is failing on iOS:

Assert.Equal() Failure: Values are not within tolerance 2\nExpected: 166.66666666666666\nActual: 152.66666666666666

@dotMorten
Copy link
Contributor Author

@jstedfast Yup first one those failures are as reported: Android's border is half the thickness it needs to be. For the second one, not quite sure - I see it passing on iOS, and unfortunately with xunit you can't attach which assert it is failing to the assert failures like mstest can.

@jstedfast
Copy link
Member

and unfortunately with xunit you can't attach which assert it is failing to the assert failures

Yea, it's something I find very frustrating about xunit as well.

@dotMorten
Copy link
Contributor Author

dotMorten commented Aug 16, 2023

Yea, it's something I find very frustrating about xunit as well.

@jstedfast It's a "tenant" of xunit. I don't quite get it either. However you could just reference <PackageReference Include="MSTest.TestFramework" Version="3.1.1" /> and get access to a different set of Assert methods that gives you the option to add a string parameter while continuing to use xunit as the test framework. Or just lift the asserts out of the framework: https://github.com/microsoft/testfx/tree/cc713270b44c8881341ccef7179feb7d09473bef/src/TestFramework/TestFramework/Assertions

@jfversluis
Copy link
Member

I assume the license headers are added intentionally? I personally don't have a problem with that, but I want to make sure that this is something that wouldn't cause any issues in agreement with the CLA and such.

Do you know of any other places where this happens? I have seen some examples but those are mostly from people working at Xamarin (or variations of) and I guess that is a bit of a different situation.

@jfversluis jfversluis added the do-not-merge Don't merge this PR label Aug 16, 2023
@jstedfast
Copy link
Member

@jfversluis FWIW, @BretJohnson @mattleibow and I suggested he do that as one way we could give him attribution for this code.

@jfversluis
Copy link
Member

@jstedfast I'm all good with that! Just want to make sure that it won't cause any issues. Anyway we can know that? Or we might run that by?

@richlander
Copy link
Member

richlander commented Aug 17, 2023

Here's our policy on licensing ...

  • All files must start with our two-line declaration (example below).
  • Files copied from other repos can (and should) keep their existing declaration. Ours goes on top. An accompanying entry must also be added to the THIRD-PARTY-NOTICES.TXT file.
  • New files shouldn't have an additional declaration. It doesn't solve any problems.

https://github.com/dotnet/runtime/blob/374b1116d14a2912f8b36ffbd9523001cceb8316/src/libraries/Microsoft.Bcl.AsyncInterfaces/src/System/Runtime/CompilerServices/AsyncIteratorMethodBuilder.cs#L1-L2

Also described at https://github.com/dotnet/runtime/blob/main/docs/project/copyright.md

Couple observations:

  • I see that the Maui repo doesn't follow this policy. This should be fixed in a separate PR. Also please ensure a Microsoft employee does this. Licensing changes are so much clearer when someone who likely has authority to make the change does so. Optics matter. Yes, I get asked strange and unfun questions from partners and customers when our repos look weird. I'd prefer not to do that.
  • I see that these files are in tests. My care factor on tests is much lower. If folks want to add additional attributive information in tests, fine with me. I consider it be decoration as opposed to having any important legal meaning.
  • The policy I outlined for product code is absolute. Any exceptions will be for very specific reasons.

@dotMorten
Copy link
Contributor Author

@richlander thanks Rich. Code is based on this: https://github.com/dotMorten/UniversalWPF/blob/main/src/UnitTests/Framework/ImageAnalysis.cs
However heavily cleaned up and adapted for MAUI and the test framework here. So should it reference the original repo?

@richlander
Copy link
Member

richlander commented Aug 17, 2023

Yes. We'd be honored to have an entry from your repo.

Please follow the pattern used at dotnet/runtime for third party notices for tests. Just do file search for "THIRD-PARTY" and you'll see all the test-specific TPNs.

The whole idea is that the root TPN describes the license burden for product code. Tests don't contribute to that. However, it still important to record the third party contributions for tests. Our test code might contribute to someone else's product.

OSS is a gift of sorts. A tidy (licensing) house enables unwrapping that gift w/confidence.

Related: dotnet/runtime#89305

@BretJohnson
Copy link
Member

Thanks @richlander for the clear and specific guidance here.

This is really a Grid test
@PureWeen PureWeen removed the do-not-merge Don't merge this PR label Aug 22, 2023
@PureWeen
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) August 22, 2023 14:31
@PureWeen PureWeen merged commit 302474a into dotnet:main Aug 22, 2023
34 checks passed
@dotMorten dotMorten deleted the connected_component_analysis_tests branch August 22, 2023 21:18
@riverar
Copy link

riverar commented Aug 23, 2023

Am confused, did this PR merge in a bunch of unused code? Is anything using ConnectedComponentAnalysis?

@dotMorten
Copy link
Contributor Author

dotMorten commented Aug 23, 2023

@riverar The tests are currently failing - they'll be turned on as those bugs gets fixed.
Example: https://github.com/dotnet/maui/pull/15856/files#diff-f98252bd202e3be880086c3d6a5c11916e9c0d45a39643626b4f630092b02ac0L117

@riverar
Copy link

riverar commented Aug 23, 2023

@dotMorten Thanks for the clarification. I'm a bit skeptical of the value/hygiene of merging in a bunch of unused code, and announcing this from the mountain tops as some sort of success, but will try to remain optimistic here.

@BretJohnson
Copy link
Member

This is an enabler, which we'll start to use for some tests. We were eager to get it merged so we can start to use it. Thanks again to @dotMorten for making this happen.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
@samhouts samhouts added the fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-testing Unit tests, device tests community ✨ Community Contribution fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants