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

Conversation

@blasten
Copy link

@blasten blasten commented May 15, 2022

Fixes the scenario tests on Android.

Issue: flutter/flutter#90401

@chinmaygarde
Copy link
Member

cc @dnfield

@dnfield dnfield self-requested a review May 16, 2022 17:41
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

I'm concerned about adding screenshot tests this way. We need some kind of plan to make sure they won't be flaky and won't be used to cover things that we really need some non-golden test covering.

For emulators running on a x64 host, build `android_debug_unopt_x64` using
`./tools/gn --android --unoptimized --goma --android-cpu=x64`.

Then, launch the emulator, and run `./testing/scenario_app/run_android_tests.sh android_debug_unopt_x64`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't introduce a shell script for this - use python.

Copy link
Contributor

Choose a reason for hiding this comment

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

(or Dart)

Copy link
Author

Choose a reason for hiding this comment

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

this is the existing shell script. It just calls dart with some flags. Is there a concern with this shell script?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'd just make sure it's actually used on CI. I think it's not, but if I'm wrong then leave it. If it's not then we should drop it.

@blasten
Copy link
Author

blasten commented May 16, 2022

I'm concerned about adding screenshot tests this way. We need some kind of plan to make sure they won't be flaky and won't be used to cover things that we really need some non-golden test covering.

@dnfield the idea is fetch the screenshots, and then send them to Skia Gold using https://github.com/flutter/engine/tree/main/web_sdk/web_test_utils. I don't know if it would flake, and if it flakes, I don't know why it would flake. What I'm thinking is to wait and see, and fix issues as they are discovered. I'm trying to be mindful about logging as much info as possible.

@blasten blasten marked this pull request as ready for review May 20, 2022 17:41
@blasten blasten requested a review from dnfield May 20, 2022 17:41
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM with nit about the color text output - we should try to detect if the terminal supports ansi and not output it, or detect if we're on LUCI and not output it.

@blasten blasten 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 May 20, 2022
@fluttergithubbot fluttergithubbot merged commit bdfedde into flutter:main May 20, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 20, 2022
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-android 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

Development

Successfully merging this pull request may close these issues.

4 participants