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

Refactor, update, and move around testing/scenario_app/README.md #50659

Merged
merged 8 commits into from
Feb 14, 2024

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Feb 14, 2024

I'm not sure about long-term direction re: separating the Android and iOS code, but other than lib/*.dart, all of the code in testing/scenario_app/** is platform-specific. Therefore, I've moved the documentation around, and added to it where it made sense, in order to make it easier for someone else to grok.

Index:

This also gives me an obvious place to further document how the Android tests work:

Screenshot 2024-02-14 at 12 35 39 PM

Closes flutter/flutter#143350.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM just some general feedback about percolating the information higher.


## Adding a New Scenario
To run the tests, you will need to build the engine with the appropriate
Copy link
Member

Choose a reason for hiding this comment

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

What is an appropriate configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. I deleted this as I think it's safe to assume if you don't know how to build android or ios engine artifacts this directory is not going to help you.


| API Version | Graphics Backend | Skia Gold | Rationale |
| ----------- | ------------------- | -------------------------------------- | ---------------------------------------------------------- |
| 28 | Skia | [Link][skia-gold-skia-28] | Older Android devices (without `ImageReader`) on Skia. |
Copy link
Member

Choose a reason for hiding this comment

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

nit: give these links apropos names instead of "Link".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,5 @@
# `android_integration_tests` runner
Copy link
Member

Choose a reason for hiding this comment

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

This isn't "android" specific, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, the entire bin folder is only used for the Android integration tests :-/

Comment on lines 24 to 31
/// ## Usage
///
/// ```sh
/// dart bin/android_integration_tests.dart \
/// --adb ../third_party/android_tools/sdk/platform-tools/adb \
/// --out-dir ../out/android_debug_unopt_arm64
/// ```
///
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be in the README.md instead of in the source file in order to minimize where people look for information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

is used to test the engine in isolation from the rest of the Flutter framework
and tooling.

## Adding a New Scenario
Copy link
Member

Choose a reason for hiding this comment

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

This should maybe be at the README.md one level higher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah fair, once I wrote this out it doesn't seem impactful enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Approving since all my feedback is optional, this is an improvement in itself and isn't wrong =)

Co-authored-by: Jenn Magder <magder@google.com>
testing/scenario_app/README.md Outdated Show resolved Hide resolved
Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>
@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2024
@matanlurey matanlurey merged commit 8a16ecb into flutter:main Feb 14, 2024
6 of 26 checks passed
@matanlurey matanlurey deleted the engine-scenario_app-READMEs branch February 14, 2024 21:24
sealesj pushed a commit to sealesj/engine that referenced this pull request Feb 15, 2024
…lutter#50659)

I'm not sure about long-term direction re: separating the Android and
iOS code, but other than `lib/*.dart`, all of the code in
`testing/scenario_app/**` is platform-specific. Therefore, I've moved
the documentation around, and added to it where it made sense, in order
to make it easier for someone else to grok.

Index:
-
[`testing/scenario_app/README.md`](https://github.com/matanlurey/engine/blob/5012e1fd5dabb8a27784ec06c2e6fdbff18c6066/testing/scenario_app/README.md)
-
[`testing/scenario_app/bin/README.md`](https://github.com/matanlurey/engine/blob/5012e1fd5dabb8a27784ec06c2e6fdbff18c6066/testing/scenario_app/bin/README.md)
-
[`testing/scenario_app/android/README.md`](https://github.com/matanlurey/engine/blob/5012e1fd5dabb8a27784ec06c2e6fdbff18c6066/testing/scenario_app/android/README.md)
-
[`testing/scenario_app/ios/README.md`](https://github.com/matanlurey/engine/blob/5012e1fd5dabb8a27784ec06c2e6fdbff18c6066/testing/scenario_app/ios/README.md)

This also gives me an obvious place to further document how the Android
tests work:

<a
href="https://github.com/matanlurey/engine/blob/5012e1fd5dabb8a27784ec06c2e6fdbff18c6066/testing/scenario_app/android/README.md"><img
width="880" alt="Screenshot 2024-02-14 at 12 35 39 PM"
src="https://github.com/flutter/engine/assets/168174/d4663005-d770-4003-a3fa-74ae8bf635c4"></a>

Closes flutter/flutter#143350.

---------

Co-authored-by: Jenn Magder <magder@google.com>
Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 16, 2024
Run engine_tool tests (flutter/engine#50662)
Refactor, update, and move around `testing/scenario_app/README.md` (flutter/engine#50659)
Starts a command line tool for assisting engine dev workflows (flutter/engine#50642)
macOS: add stubs for PlatformView gesture handling (flutter/engine#50630)
[Impeller] Conditionally use A8 or R8 format glyph atlas based on capabilities. (flutter/engine#50534)
Roll Dart SDK from e7cfba13d375 to 322e34dc53f6 (1 revision) (flutter/engine#50651)
Roll Skia from 4235c0421a48 to eae42ea9f7bc (2 revisions) (flutter/engine#50650)
Roll Skia from ea2fd25220cb to 4235c0421a48 (1 revision) (flutter/engine#50648)
Roll Dart SDK from 9982d96cebb0 to e7cfba13d375 (1 revision) (flutter/engine#50646)
Roll Skia from 1e6e2114f15e to ea2fd25220cb (1 revision) (flutter/engine#50643)
Roll Dart SDK from 032323fa534b to 9982d96cebb0 (1 revision) (flutter/engine#50644)
Roll Skia from 2aec75cda46e to 1e6e2114f15e (2 revisions) (flutter/engine#50641)
Roll Skia from b8acfa559db0 to 2aec75cda46e (1 revision) (flutter/engine#50639)
Roll Fuchsia Linux SDK from l6mWjvlO1xJg5ZFKK... to mZP8LxbhYHstUxmxd... (flutter/engine#50638)
Roll Skia from 79ec267090bd to b8acfa559db0 (1 revision) (flutter/engine#50636)
Roll Skia from d650dcaf4b49 to 79ec267090bd (1 revision) (flutter/engine#50634)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
4 participants