Re-enable scenario tests on Android #33574
Conversation
blasten
force-pushed
the
ci_yaml
branch
5 times, most recently
from
9910962 to
d18fff3
Compare
blasten
force-pushed
the
ci_yaml
branch
2 times, most recently
from
28295a4 to
74f9c92
Compare
|
Gold has detected about 1 new digest(s) on patchset 15. |
|
Gold has detected about 1 new digest(s) on patchset 16. |
| double width = 500, | ||
| double height = 500, | ||
| }) async { | ||
| final bool usesAndroidHybridComposition = scenarioParams['use_android_view'] as bool; |
There was a problem hiding this comment.
scenarioParams['use_android_view'] is null on iOS and triggers a crash when trying to cast.
|
Gold has detected about 1 new digest(s) on patchset 17. |
|
Gold has detected about 2 new digest(s) on patchset 19. |
|
Gold has detected about 24 new digest(s) on patchset 21. |
|
Gold has detected about 24 new digest(s) on patchset 22. |
testing/scenario_app/lib/main.dart
Outdated
| void _onBeginFrame(Duration duration) { | ||
| currentScenario?.onBeginFrame(duration); | ||
|
|
||
| Future<void> _onBeginFrame(Duration duration) async { |
There was a problem hiding this comment.
Can we avoid making this async? There's nothing getting awaited in here.
| final SceneBuilder builder = SceneBuilder(); | ||
|
|
||
| finishBuilderByAddingPlatformViewAndPicture(builder, id); | ||
| await addPlatformView( |
There was a problem hiding this comment.
We should try to avoid this - instead we should be pumping frames when things are ready. Making this async makes it a bit harder to reason about what actually drives this method getting called and when it will complete vs. the engine lifecycle side.
There was a problem hiding this comment.
I think using async makes it more clear. For instance, the state is manage by the future, and there's no need to keep track of before/after a condition is met.
In this case the condition is that the method should receive an id from the platform before using the SsceneBuilder API.
There was a problem hiding this comment.
But we're calling a future from something that can't handle futures. The state just disappears into the future and it's really not clear to me from reading this how that's interacting with the frame scheduling that's going on in the engine. I'm not sure if what we're testing here resembles the way a real application would use this or not (i.e. one that will have to handle frame scheduling, even if via the framework).
There was a problem hiding this comment.
that's a good point. It's now scheduling frames as the framework does. It would be ideal if we could actually use the framework, and do a proper integration test. I think it's still not great that a lot of what we do in this test is assume the framework does something similar.
|
The golden files look like the emulator is having some error condition happening at startup and is rendering that over the application and platform views. We should fix that before landing this. |
|
|
|
Gold has detected about 24 new digest(s) on patchset 27. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
Code is looking good, there's still a system dialog showing over the UI |
|
Gold has detected about 23 new digest(s) on patchset 30. |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
@dnfield the latest ones look ok: https://flutter-engine-gold.skia.org/search?crs=github&issue=33574&patchsets=30&positive=true. I sent https://flutter-review.googlesource.com/c/recipes/+/30888 as I suspect one issue we are running is that the user data image is growing over time since these builders don't create a new clean environment whenever a new task is executed. PTAL |
|
Also see http://b/234791343 |
blasten
added
the
waiting for tree to go green
…lutter#33813)" This reverts commit 0de2f6a.
…lutter#33813)" This reverts commit 0de2f6a.
Re-enables the scenario tests on Android.
Fixes: flutter/flutter#90401