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

Revamp the scenario_app/../README.md to encourage self-sustenance #51196

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

matanlurey
Copy link
Contributor

Feedback welcome, though if it is non-blocking and ambiguous consider a follow-up PR instead 😸

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM I read the whole thing

Copy link
Member

@gmackall gmackall 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 a couple typo nits

Or for a specific, build, such as `android_debug_unopt_arm64`:
By default when run locally, the test runner:

- Uses the engine-wide efault graphics backend for Android.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Uses the engine-wide efault graphics backend for Android.
- Uses the engine-wide default graphics backend for Android.

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!

dart ./testing/scenario_app/bin/run_android_tests.dart --help
```

Frequency used options include:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Frequency used options include:
Frequently used options include:

Copy link
Member

Choose a reason for hiding this comment

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

fyi: there is a vscode plugin that will do spell checking, super handy.

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.

hoo boy, looks good. I just have some minor nits about what information should be at the top and generally there are a couple of cases where external links could be added or made more contextual.

Comment on lines 19 to 38
This test suite was [originally written in 2019](https://github.com/flutter/engine/pull/10007)
with a goal of:

> \[being\] suitable for embedders to do integration testing with - it has no
> dependencies on the flutter_tools or framework, and so will not fail/flake
> based on variances in those downstream.

Unfortunately, the Android side of the test suite was never fully operational,
and the tests, even if failing, were accidentally be reported as passing on CI.
In 2024, as the team got closer to shipping our new graphics backend,
[Impeller](https://docs.flutter.dev/perf/impeller) on Android, it was clear that
we needed a reliable test suite for the engine on Android, particularly for
visual tests around external textures and platform views.

So, this package was revived and updated to be a (more) reliable test suite for
the engine on Android. It's by no means complete
([contributions welcome](#contributing)), but it did successfully catch at least
one bug that would not have been detected automatically otherwise.

_Go forth and test the engine on Android!_
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this history-in-prose should get top billing. I think a Timeline section lower down would make more sense. We can keep the guiding principles.

Something like:

## Timeline
- 2019 Originally implemented: lorem ipsom
- 2024 Revamped to work for impeller.

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!

### Scope of Testing

The tests in this package are end-to-end tests that _specifically_ exercise the
engine and Android-specific embedding code. They are not unit tests for the
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add a link after "unit tests" to the link provided below. Maybe using something like [1]

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!

width="300"
/>

_The top picture is the Flutter app, and the bottom picture is just Android._
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a link to https://github.com/flutter/engine/tree/main/tools/compare_goldens here, maybe a "see also" note.

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!

dart ./testing/scenario_app/bin/run_android_tests.dart --help
```

Frequency used options include:
Copy link
Member

Choose a reason for hiding this comment

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

fyi: there is a vscode plugin that will do spell checking, super handy.


See also:

- [`DrawSolidBlueScreenTest.java`](./app/src/androidTest/java/dev/flutter/scenariosui/DrawSolidBlueScreenTest.java), an example of a JUnit test.
Copy link
Member

Choose a reason for hiding this comment

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

I'd put these links above to make them more contextual.

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!

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 6, 2024
@auto-submit auto-submit bot merged commit d374c78 into flutter:main Mar 6, 2024
26 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 6, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 6, 2024
…144659)

flutter/engine@effcf97...d374c78

2024-03-06 matanlurey@users.noreply.github.com Revamp the `scenario_app/../README.md` to encourage self-sustenance (flutter/engine#51196)
2024-03-06 jonahwilliams@google.com [Impeller] disable blending in gaussian intermediate steps. (flutter/engine#51118)
2024-03-06 bdero@google.com [Impeller] Enable depth buffer clipping & Stencil-then-Cover path rendering. (flutter/engine#51219)
2024-03-06 skia-flutter-autoroll@skia.org Roll Skia from 37947aec8c5c to 1d1fd7fe1a89 (1 revision) (flutter/engine#51216)
2024-03-06 bdero@google.com [Impeller] Fix path winding when bridging from contours with an odd number of points. (flutter/engine#51218)

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 rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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