Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[fuchsia] migrate touch-input integration tests to gfx scene manager test ui stack #41118

Merged
uysalere merged 23 commits into from
Apr 18, 2023
Merged

[fuchsia] migrate touch-input integration tests to gfx scene manager test ui stack #41118

uysalere merged 23 commits into from
Apr 18, 2023

Conversation

@caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Apr 12, 2023 •
edited
Loading

This change migrates touch-input integration tests from the gfx-root-presenter-test-ui-stack UI test realm variant to run parameterized tests of two types: gfx-scene-manager-test-ui-stack and flatland-scene-manager-test-ui-stack. Both are exercised for the generic tap test, and only GFX is exercised for embedded view cases. This will enable fuchsia.git to remove the gfx-root-presenter-test-ui-stack variant, which will no longer be supported.

This change also:

  • removes or updates all remaining references of root presenter, which manifested in the form of inline code comments
  • adds a TODO to update the child view app for the embedded touch input test cases

Note that this change does not modify the embedder integration tests, which use a generic test-ui-stack variant as defined by the fuchsia pkg url: fuchsia-pkg://fuchsia.com/test-ui-stack#meta/test-ui-stack.cm. Since the contents of this package is defined in fuchsia.git, the realm under test can adapt to changes in fuchsia.git so long as they update the test-ui-stack build target (which they do in https://fxrev.dev/831359, the change that relies on the touch changes in this repo described above.)

Fixes https://fxbug.dev/125304

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@caroqliu caroqliu mentioned this pull request Apr 13, 2023
Closed
8 tasks
@chinmaygarde
Copy link
Member

chinmaygarde commented Apr 13, 2023

The presub formatting checks are minor and can be fixed via ./flutter/ci/format.sh --fix

@caroqliu
Copy link
Contributor Author

caroqliu commented Apr 14, 2023

Ok that was a journey. Discovered fxbug.dev/125514 for the failing embedded touch tests on the Flatland test-ui-stack variant, so this change removes that variant for all embedded test cases. I'll update the PR description before submitting but wanted to leave a note here for any confusion in the meantime.

@arbreng
Copy link
Contributor

arbreng commented Apr 17, 2023

so this change removes that variant for all embedded test cases.

Since smart display is now using Flatland, this means we are missing valuable coverage on that product by not having the embedded touch case. This specific use case of embedded taps has been a source of nasty bugs in the past (ask Jaeheon).

What can we do to ensure this test case is re-enabled ASAP? I'm concerned fxbug.dev/125514 might just disappear into the void. It does not look like that much work to fix; anyone on the team could handle it.

@caroqliu
Copy link
Contributor Author

caroqliu commented Apr 17, 2023 •
edited
Loading

so this change removes that variant for all embedded test cases.

Since smart display is now using Flatland, this means we are missing valuable coverage on that product by not having the embedded touch case. This specific use case of embedded taps has been a source of nasty bugs in the past (ask Jaeheon).

What can we do to ensure this test case is re-enabled ASAP? I'm concerned fxbug.dev/125514 might just disappear into the void. It does not look like that much work to fix; anyone on the team could handle it.

Good question! I should rephrase my prior comment -- I've removed the Flatland variant for embedded taps from being introduced in this change. These tests never had coverage from that variant in this repo, so this change still offers more coverage than without. (FWIW these tests do exercise Flatland in its fuchsia.git counterpart.)

I agree we are missing the Flatland coverage for embedded taps and should work to add it ASAP, but that's a separate task from what this PR is trying to solve -- which is to enable deletion of root presenter scene logic code in fuchsia.git by removing dependencies of these tests on gfx-root-presenter-test-ui-stack.

Rest assured that the bug won't disappear into the void because these tests, now running the GFX Scene Manager Test UI stack with this change, will now block GFX code deletions, which will be the next phase of work once Flatland migration is irreversible. Since cleaning up that tech debt is a priority, we will come back to do this -- just maybe in a couple weeks instead of now.

@caroqliu caroqliu changed the title [fuchsia] migrate touch-input integration tests to scene manager test ui stack variants [fuchsia] migrate touch-input integration tests to gfx scene manager test ui stack Apr 17, 2023
@uysalere uysalere added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 18, 2023
@uysalere uysalere merged commit 55bb065 into flutter:main Apr 18, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2023
@engine-flutter-autoroll
55bb065c6 [fuchsia] migrate touch-input integration tests to gfx scen…
9d05632 
…e manager test ui stack (flutter/engine#41118)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2023
@engine-flutter-autoroll
55bb065c6 [fuchsia] migrate touch-input integration tests to gfx scen…
b7044ab 
…e manager test ui stack (flutter/engine#41118)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 18, 2023
@engine-flutter-autoroll
Roll Flutter Engine from d2973619074e to 55bb065c607b (1 revision) (#…
f7245b6 
…125047)

flutter/engine@d297361...55bb065

2023-04-18 [email protected] [fuchsia] migrate touch-input integration tests to gfx scene manager test ui stack (flutter/engine#41118)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@arbreng arbreng arbreng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

platform-fuchsia waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@caroqliu @chinmaygarde @arbreng @uysalere