Migrate gallery/transitions_perf test to e2e by CareF · Pull Request #62064 · flutter/flutter

CareF · 2020-07-22T20:59:56Z

Description

Migrating gallery/transitions_perf test to e2e, hopefully it can solve the flaky issue for the test.

Related Issues

Tests

This is itself a test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

CareF · 2020-07-29T01:58:24Z

Currently transitions_perf is relying on timeline for the 'Start Transition' event to locate transition:

flutter/dev/integration_tests/flutter_gallery/test_driver/transitions_perf_test.dart

Lines 64 to 74 in 78efd3e

    
           /// Extracts event data from [events] recorded by timeline, validates it, turns 
        
           /// it into a histogram, and saves to a JSON file. 
        
           Future<void> saveDurationsHistogram(List<Map<String, dynamic>> events, String outputPath) async { 
        
             final Map<String, List<int>> durations = <String, List<int>>{}; 
        
             Map<String, dynamic> startEvent; 
        
             int frameStart; 
        
             // Save the duration of the first frame after each 'Start Transition' event. 
        
             for (final Map<String, dynamic> event in events) { 
        
               final String eventName = event['name'] as String; 
        
               if (eventName == 'Start Transition') {

which is defined in the gallery app:

flutter/dev/integration_tests/flutter_gallery/lib/gallery/home.dart

Lines 172 to 180 in 78efd3e

    
           void _launchDemo(BuildContext context) { 
        
             if (demo.routeName != null) { 
        
               Timeline.instantSync('Start Transition', arguments: <String, String>{ 
        
                 'from': '/', 
        
                 'to': demo.routeName, 
        
               }); 
        
               Navigator.pushNamed(context, demo.routeName); 
        
             } 
        
           }

So if we still want to keep avoiding timeline in the solution, we need to modify the app for sending a signal about the transition, or we can just ignore this.
@liyuqian what do you think?

Decided not to track this metric.

CareF · 2020-07-29T19:56:01Z

This PR is ready for review. @liyuqian
And I believe this is a very good example of e2e version being able to avoid #59263

CareF · 2020-07-30T17:12:49Z

removed the transition count because I can't decide which frame is the exact frame after transition, due to different epoch in FrameTiming and in DateTime

liyuqian · 2020-07-30T23:56:36Z

dev/devicelab/manifest.yaml

Great! Once this is proved to work, let's add ios and ios32 versions of this in another PR as those are currently flaky.

dev/integration_tests/flutter_gallery/test_driver/e2e_util.dart

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e.dart

liyuqian · 2020-07-31T01:02:27Z

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e.dart

A lot of the following code is very similar to runDemo in https://github.com/flutter/flutter/blob/master/dev/integration_tests/flutter_gallery/test_driver/transitions_perf_test.dart.

As we plan to preserve both Flutter driver timeline perf tests and e2e FrameTiming perf tests for a while (partially because timeline still has a lot more information that FrameTiming doesn't), it's probably better to use a single piece of code in both places. Maybe now is a good opportunity to let FlutterDriver implements WidgetController so we can share the code?

This would guarantee that driver tests and e2e tests are measuring similar animations, and avoid the risk that someone changes one test and forget to update another.

It's hard. WidgetController has everything depending on a binding (it can be a test binding or a real binding), but FlutterDriver doesn't have a binding. And the finder in WidgetController and that in FlutterDriver don't share base class.

So to do that, the path in my opinion would be to write a mixin separating the controlling part (not the setting up part) of WidgetController and share this with FlutterDriver, and similarly for both finders. Even with that, there are some APIs doing the same thing but are named differently, meaning the result will be a breaking change. If we still consider abandon flutter_driver for driving the test, I don't think it will be worth it.

Hmm... your concerns seems to be very legitimate. In that case, for Flutter driver timeline perf tests, can we just control the test inside transitions_perf.dart which is running on device instead of transitions_perf_test.dart? Then it seems that the host side transitions_perf_test.dart only needs to send a signal to device to start the control sequence (and the host will start capturing the timeline tracing events). The device-side code in transitions_perf.dart should be shareable with the E2E test?

Haven't tried but sounds do-able. If that's the case the issue I mentioned above about using WidgetController and thus #62640 becomes necessary, and the measured result may be different with the original flutter_driver ones because it's almost a new test (a host-driven and self-driven hybrid test). Will try after #62640 lands .

3e83f94 is an implementation of making flutter drive test share the driving code with the e2e test (making it a hybrid of host-driven and self-driven).

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e_test.dart

liyuqian · 2020-07-31T01:41:19Z

dev/devicelab/lib/tasks/gallery.dart

According to our discussion earlier today, it seems that we're not planning to measure transition duration in the e2e test. If so, why do we still do this Firebase specific work as only e2e tests can run on Firebase?

This piece is shared with the original flutter_driver based transition_perf test. I just moved it into an if so in the e2e it doesn't go through this by not providing a transition_duration file name. I don't know why it talks about firebase.

Ah, I didn't realize this code was already there before your PR. It seems to be added by @yjbanov 4 years ago https://github.com/flutter/flutter/blame/30aef0a3b9611763f8e60985e7cca9cb30c1ea6a/dev/devicelab/lib/tasks/gallery.dart#L47.

Yegor: do you still remember why we're doing this as I'm pretty sure we're not using Firebase test lab until very recently? What could happen today if we remove it (the gallery transition test is still not running on Firebase yet)?

I now removed this extra work, but I'll wait to see if there's anything breaking that we didn't realize.

CareF · 2020-08-07T02:57:53Z

@liyuqian Now it's ready for review again. The last commit share the runDemos method between driver test and e2e test, making the former a hybrid test. I'm thinking maybe it is better to create another test case for this and make the previous driver test as is. Though that means there will still be duplicate code.

The commit above (3e83f94) is removed and will be landed in another PR if we decided to use it.

CareF · 2020-08-10T16:30:14Z

@dnfield I removed those duplicate code here, as it's relevant for creating the necessary java code as part of e2e firebase test lab support.

dev/benchmarks/macrobenchmarks/test/util.dart

dev/devicelab/manifest.yaml

liyuqian · 2020-08-10T23:55:07Z

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e.dart

If we plan to use this file both in e2e and flutter_driver-timeline tests, maybe we should rename this file?

Nope, this file is not reused as a whole. For the reused version run_demos function will be in a separate file.

dev/integration_tests/flutter_gallery/test_driver/e2e_utils.dart

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e_test.dart

liyuqian

LGTM

fluttergithubbot · 2020-08-12T06:06:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite web_tests-1-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jul 22, 2020

CareF marked this pull request as draft July 22, 2020 21:00

CareF self-assigned this Jul 22, 2020

googlebot added the cla: yes label Jul 22, 2020

fluttergithubbot added the work in progress; do not review label Jul 22, 2020

CareF mentioned this pull request Jul 22, 2020

flutter_gallery's driver test doesn't show image resources #30641

Closed

CareF changed the title ~~Migrate gallery/transitions_perf test to 2e2~~ Migrate gallery/transitions_perf test to e2e Jul 22, 2020

CareF mentioned this pull request Jul 23, 2020

Add WidgetController.scrollUntilVisible #62097

Merged

9 tasks

CareF marked this pull request as ready for review July 29, 2020 19:53

fluttergithubbot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Jul 29, 2020

CareF requested a review from liyuqian July 29, 2020 19:53

CareF removed the work in progress; do not review label Jul 29, 2020

liyuqian reviewed Jul 31, 2020

View reviewed changes

CareF mentioned this pull request Jul 31, 2020

promote WidgetTester pumpAndSettle #62640

Merged

9 tasks

CareF requested a review from liyuqian July 31, 2020 15:42

CareF marked this pull request as draft August 5, 2020 15:34

CareF marked this pull request as ready for review August 7, 2020 01:55

This was referenced Aug 10, 2020

[team] flutter_gallery_ios32__transition_perf flakes "Original error: Bad state: The client closed with pending request "ext.flutter.driver"" #60559

Closed

Remove the firebase test lab tests #63419

Closed

liyuqian reviewed Aug 10, 2020

View reviewed changes

liyuqian reviewed Aug 11, 2020

View reviewed changes

dev/integration_tests/flutter_gallery/test_driver/e2e_utils.dart Outdated Show resolved Hide resolved

dev/integration_tests/flutter_gallery/test_driver/transitions_perf_e2e_test.dart Outdated Show resolved Hide resolved

CareF requested a review from liyuqian August 11, 2020 23:40

liyuqian approved these changes Aug 12, 2020

View reviewed changes

CareF added the waiting for tree to go green label Aug 12, 2020

add an e2e test

a340658

CareF added 19 commits August 12, 2020 00:57

add to device lab

9dba080

remove transition

559e394

move utils to a package

9c4cfaf

move debug print to end_perf

74f86f4

using end_perf e2e_driver

daeef36

add flaky flag

e0b6840

remove duplicate from new package

30b7c2e

end_perf -> e2e for macrobenchmarks

b960ba8

widgetTester => widgetController

65a98e4

make compatible with new e2e

b93aa35

update to e2e 6.3

76d495a

update pubspec checksum

b82106d

remove old instrument and support firebase

4ee8cc5

remove firebase related key transform

16e4843

formatting

a77644b

revert end_perf_to_e2e

8b4033a

remove summary code

66a6281

modify according to liyuqian@

3423ca6

update pubspec

b7ef43d

fluttergithubbot removed the waiting for tree to go green label Aug 12, 2020

CareF added the waiting for tree to go green label Aug 12, 2020

fluttergithubbot merged commit 2384376 into flutter:master Aug 12, 2020

CareF deleted the gallery_e2e branch August 12, 2020 20:38

This was referenced Aug 12, 2020

Clean up flutter_gallery__transition_perf #63609

Merged

add flutter_gallery__transition_perf_e2e to ios tests #63797

Merged

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Migrate gallery/transitions_perf test to e2e (flutter#62064)

315f726

This was referenced Jul 21, 2021

Devicelab task tests not running anywhere #86802

Closed

Remove unused flutter_gallery_instrumentation_test #86812

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021

Conversation

CareF commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

CareF commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CareF commented Jul 29, 2020

Uh oh!

CareF commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CareF commented Aug 10, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CareF Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CareF commented Jul 22, 2020 •

edited

Loading

CareF commented Jul 29, 2020 •

edited

Loading

CareF commented Jul 30, 2020 •

edited

Loading

CareF Jul 31, 2020 •

edited

Loading

CareF Jul 31, 2020 •

edited

Loading

CareF Jul 31, 2020 •

edited

Loading

CareF Jul 31, 2020 •

edited

Loading

CareF commented Aug 7, 2020 •

edited

Loading

CareF Aug 11, 2020 •

edited

Loading