Add Visual Tree Dump Testing to E2E Test App on Fabric #12322
Conversation
| @@ -1499,6 +1499,9 @@ void CompositionViewComponentView::updateProps( | |||
| if (oldViewProps.opacity != newViewProps.opacity) { | |||
| m_visual.Opacity(newViewProps.opacity); | |||
| } | |||
| if (oldViewProps.testId != newViewProps.testId) { | |||
| m_visual.Comment(newViewProps.testId == "" ? L"View" : winrt::to_hstring(newViewProps.testId)); | |||
There was a problem hiding this comment.
Not sure that we want to be adding the overhead of setting a comment on every View. - And as it is this wont set it to View unless the user sets it to something other than empty, then sets it to empty.
I think its fine to just keep them empty by default.
| auto nodeChildren = containerNode.Children(); | ||
| winrt::Windows::Data::Json::JsonArray children; | ||
| for (auto childVisual : nodeChildren) { | ||
| if (!targetNodeHit && childVisual.Comment() == accessibilityId) { |
There was a problem hiding this comment.
Do we have to save the id in the Comment? Can we use another (or create) more unique and descriptive property? This feels like how we used the Tag property in XAML for RN tags, which was an annoying move for consumers who use that public property for other things.
There was a problem hiding this comment.
So at present I'm iterating through the Microsoft::UI::Composition::Visual types which only have a small set of properties defined Comment being one of them. If we wanted to define a unique property to save the testId to then I believe I would have to change the algorithm to iterate through the Microsoft::ReactNative::IVisual type which allows us to define new properties. Something to note it looks like all the properties do just get passed into the corresponding composition properties. So it would be a distinct design choice to have the IVisual itself store property data. It should be possible for me to rewrite the tests in the way just depends on which design choice we want and if we think customers will want to override the Comment property to something other than the testId. @acoates-ms any thoughts?
There was a problem hiding this comment.
Yes the E2E tests are not going to be able to read anything from the MS.RN.Visuals. It can only access the underlying composition visuals. You could store the testid as a property within the Visual.Properties() object.
There was a problem hiding this comment.
@acoates-ms Yea that was another option I was looking at! But it doesn't look like Properties() supports storing string data (https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.composition.compositionpropertyset?view=windows-app-sdk-1.4). There seems to only be the option to store booleans, colors, and floats/vectors of floats.
There was a problem hiding this comment.
Hmm. maybe comment is the right mapping for this then. -- To @jonthysell's comment, unlike how we were using Tag, this would only be for components that have set a testId - which is more explicit that the user of the JS wants something to be set on the native side.
There was a problem hiding this comment.
Cool makes sense to me! @jonthysell sound okay to you?
| result.Insert(L"Opacity", winrt::Windows::Data::Json::JsonValue::CreateNumberValue(node.Opacity())); | ||
| auto spriteVisual = node.try_as<winrt::Microsoft::UI::Composition::SpriteVisual>(); | ||
| if (spriteVisual) { | ||
| result.Insert(L"Visual Type", winrt::Windows::Data::Json::JsonValue::CreateStringValue(L"SpriteVisual")); |
There was a problem hiding this comment.
It looks like everything in the tree is a SpriteVisual?
| @@ -63,6 +63,7 @@ namespace Microsoft.ReactNative.Composition | |||
| void Offset(Windows.Foundation.Numerics.Vector3 offset, Windows.Foundation.Numerics.Vector3 relativeAdjustment); | |||
| void RelativeSizeWithOffset(Windows.Foundation.Numerics.Vector2 size, Windows.Foundation.Numerics.Vector2 relativeSizeAdjustment); | |||
| BackfaceVisibility BackfaceVisibility{ get; set; }; | |||
| String Comment { get; set; }; | |||
There was a problem hiding this comment.
Can we just name this TestID?
Description
Type of Change
Why
Increase automated validation of the fabric architecture to reduce regressions.
#11422
What
Adds visual tree dump testing to e2e-test-app-fabric. Tree dumps include size, offset, opacity, and color brush information. Additional properties can be added later to increase property coverage.
Note: When running locally, make sure to put display into 100% scaling (Windows usually defaults to 150%). This way the scaling lines up with CI. At some point I will add code to avoid having to do this manual step.
Changelog
Should this change be included in the release notes: No
Microsoft Reviewers: Open in CodeFlow