Add mock capability to PerformanceOverlayLayer #7537

liyuqian · 2019-01-18T23:41:25Z

So we can do golden test on PerformanceOverlay to avoid regression like flutter/flutter#26387

To avoid regression like flutter#26387 Need flutter/engine#7537 to land first.

liyuqian · 2019-01-23T19:38:43Z

Ping...

chinmaygarde · 2019-01-23T19:40:42Z

I am not comfortable having mock data present in the file itself. Can you make the layer accept data to display (instead of collect it on its own) and have the unittest supply said data?

So we can do golden test on PerformanceOverlay to avoid regression like flutter/flutter#26387

liyuqian · 2019-01-28T19:48:03Z

@chinmaygarde : now the unittest supply the data.

liyuqian · 2019-01-29T18:16:20Z

Ping...

chinmaygarde

I feel like I did not do an adequate job of conveying my objection initially. The code as it reads currently has explicit logic in all build modes to deal with mock data. My expectation to the amendments suggested in previous comments were something closer to having the stopwatch be constructed using mock data and have the stopwatch definition be supplied (by the test) to the overlay layer.

Now, obviously, you cannot create a stopwatch using the test harness you are currently using (as it is an integration test with Dart code). To work around this limitation, you have had to add a lot code to plumb this data from the test specification in Dart all the way to down to the overlay layer. The Dart code does not need or care about this in normal operation. This, I believe, is because of the use of the wrong test harness to test this functionality.

A more explicit suggestion would be:

Allow stopwatches to be created using lap information upfront. Stopwatch(std::vector<fml::TimeDelta> laps);
Create a new unit test in flow_unittests that creates a performance overlay layer and paints the same using a stopwatch with mock data (specified in the test).
Compare the snapshot with a fixture.

liyuqian · 2019-01-30T00:30:17Z

@chinmaygarde now it's a pure C++ unit test without any changes to the framework or flow layers. My current concern is how easy we can port this test to Android/iOS unit test once we have a device lab golden test. I think that we can worry about this later if you find the current approach Ok.

liyuqian · 2019-01-31T01:18:34Z

@chinmaygarde @Hixie golden test the performance overlay will run into font rendering mismatch issues in different Linux versions. My current solution is to use our Docker container which ensures the identical environment as our presubmit checks. Please let me know your comments.

Hixie · 2019-02-03T03:13:15Z

I don't have an opinion on the approach here. FWIW, in other situations we avoid the font issue by using the "Ahem" font, which is carefully designed to be predictable and identical everywhere.

liyuqian · 2019-02-05T00:13:19Z

Ok, I modified PerformanceOverlayLayer so it can take an additional font_path parameter. Now I found that specifying "Roboto-Regular" is sufficient to make sure that pixels match. This is a little better than "Ahem" because we can see more meaningful texts in the golden image. I guess that we don't have to worry too much about the font rendering since this font is just for this specific test only.

liyuqian · 2019-02-06T22:03:48Z

Ping...

liyuqian · 2019-02-08T22:39:31Z

Ping @chinmaygarde ...

dnfield · 2019-02-09T07:52:42Z

This is breaking no post-submit - is there an image missing? see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8922049806835306640/+/steps/Test_Flow/0/stdout or https://build.chromium.org/p/client.flutter/builders/Linux%20Engine/builds/4762/steps/Test%20Flow/logs/stdio

dnfield · 2019-02-09T07:57:28Z

Ahh it looks like the file is there, but the engine recipe has the tests running from the src/out/whatever_debug_unopt/ directory (https://cs.chromium.org/chromium/build/scripts/slave/recipes/flutter/engine.py?q=file:recipes/flutter/engine.py&g=0&l=51)

We need to either let the file come in as a parameter or maybe copy it over as part of a build step, or edit the recipe to run this specific test in the correct directory (but I'd rather avoid modifying the recipe for this).

dnfield · 2019-02-09T07:57:48Z

In the mean time I'm going to revert this to un-red the bots.

This reverts commit 5f3f3bd.

…e#7537)

flutter/engine@7c70240...90569e8 git log 7c70240..90569e8 --no-merges --oneline 90569e8 Provide public api to allow FlutterEngine related context to be destoryed (flutter/engine#7610) 693645e Revert "Add mock capability to PerformanceOverlayLayer (#7537)" (flutter/engine#7765) d130f15 Add x bit to some python scripts (flutter/engine#7764) 2146cde Roll src/third_party/skia 706a7cd1e826..87461aad7285 (1 commits) (flutter/engine#7763) 2a648b9 Roll src/third_party/skia be39f713e530..706a7cd1e826 (4 commits) (flutter/engine#7761) 68396ae Throttle picture raster cache (flutter/engine#7759) a6753b0 Roll src/third_party/dart 52f5e34dbf..174d6fec3d (12 commits) f9252e7 allow specifying out directory root (flutter/engine#7753) 12d0b95 Don't call OnAnimatorNotifyIdle if a frame is scheduled (flutter/engine#7746) 5f3f3bd Add mock capability to PerformanceOverlayLayer (flutter/engine#7537) 7d97afd Roll src/third_party/skia 37064c1739f3..be39f713e530 (7 commits) (flutter/engine#7757) 3bfd265 Add flutter config to macOS targets (flutter/engine#7756) 4d3a112 Document GPUSurfaceGLDelegate methods and move it to its own file. (flutter/engine#7755) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

flutter/engine@7c70240...90569e8 git log 7c70240..90569e8 --no-merges --oneline 90569e8 Provide public api to allow FlutterEngine related context to be destoryed (flutter/engine#7610) 693645e Revert &flutter#34;Add mock capability to PerformanceOverlayLayer (flutter#7537)&flutter#34; (flutter/engine#7765) d130f15 Add x bit to some python scripts (flutter/engine#7764) 2146cde Roll src/third_party/skia 706a7cd1e826..87461aad7285 (1 commits) (flutter/engine#7763) 2a648b9 Roll src/third_party/skia be39f713e530..706a7cd1e826 (4 commits) (flutter/engine#7761) 68396ae Throttle picture raster cache (flutter/engine#7759) a6753b0 Roll src/third_party/dart 52f5e34dbf..174d6fec3d (12 commits) f9252e7 allow specifying out directory root (flutter/engine#7753) 12d0b95 Don&flutter#39;t call OnAnimatorNotifyIdle if a frame is scheduled (flutter/engine#7746) 5f3f3bd Add mock capability to PerformanceOverlayLayer (flutter/engine#7537) 7d97afd Roll src/third_party/skia 37064c1739f3..be39f713e530 (7 commits) (flutter/engine#7757) 3bfd265 Add flutter config to macOS targets (flutter/engine#7756) 4d3a112 Document GPUSurfaceGLDelegate methods and move it to its own file. (flutter/engine#7755) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

…er#7537)" (flutter#7765)" This reverts commit 693645e.

* Revert "Revert "Add mock capability to PerformanceOverlayLayer (#7537)" (#7765)" This reverts commit 693645e. * Add command line args for golden dir and font file

liyuqian requested review from Hixie, chinmaygarde and jason-simmons January 18, 2019 23:41

googlebot added the cla: yes label Jan 18, 2019

liyuqian mentioned this pull request Jan 18, 2019

Golden test for performance overlay flutter/flutter#26781

Closed

liyuqian added a commit to liyuqian/flutter that referenced this pull request Jan 18, 2019

Golden test for performance overlay

0d6fd2e

To avoid regression like flutter#26387 Need flutter/engine#7537 to land first.

liyuqian requested a review from GaryQian January 22, 2019 19:19

liyuqian added 2 commits January 28, 2019 10:14

Add mock capability to PerformanceOverlayLayer

4c1059b

So we can do golden test on PerformanceOverlay to avoid regression like flutter/flutter#26387

Allow the framework to set the mock data

1aaf894

liyuqian force-pushed the mock_performance_overlay branch from bfb78a3 to 1aaf894 Compare January 28, 2019 19:46

Clang-format

3d64aeb

chinmaygarde reviewed Jan 29, 2019

View reviewed changes

liyuqian added 4 commits January 29, 2019 16:21

Make it a google unittest

4ce6bc7

Clang format

c4403b6

Undo dart changes

b1241d1

Undo PerformanceOverlayLayer changes

0409adf

liyuqian added 5 commits January 29, 2019 16:32

Update license

db58f1d

Improve error messages

d732106

Restore license

7816e1c

Update license

2941ccc

Add base64 in the error message for bots

8d35232

liyuqian force-pushed the mock_performance_overlay branch from 67c7541 to 8d35232 Compare January 31, 2019 00:45

Clarify the environment to generate golden images

da686a0

Add container attach/detach comment

0bec17d

liyuqian added 3 commits February 4, 2019 15:30

Allow fonts to be set

8ade68c

Clang-format

305fbbf

Remove obsolete comments

49b22f5

chinmaygarde approved these changes Feb 8, 2019

View reviewed changes

liyuqian merged commit 5f3f3bd into flutter:master Feb 8, 2019

dnfield added a commit that referenced this pull request Feb 9, 2019

Revert "Add mock capability to PerformanceOverlayLayer (#7537)"

c34931b

This reverts commit 5f3f3bd.

dnfield mentioned this pull request Feb 9, 2019

Revert "Add mock capability to PerformanceOverlayLayer" #7765

Merged

dnfield added a commit that referenced this pull request Feb 9, 2019

Revert "Add mock capability to PerformanceOverlayLayer (#7537)" (#7765)

693645e

This reverts commit 5f3f3bd.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 9, 2019

5f3f3bd Add mock capability to PerformanceOverlayLayer (flutter/engin…

de1f6b5

…e#7537)

engine-flutter-autoroll mentioned this pull request Feb 9, 2019

Roll engine 7c702404b820..90569e8416c7 (13 commits) flutter/flutter#27722

Merged

liyuqian added a commit to liyuqian/engine that referenced this pull request Feb 17, 2019

Revert "Revert "Add mock capability to PerformanceOverlayLayer (flutt…

6f15603

…er#7537)" (flutter#7765)" This reverts commit 693645e.

liyuqian mentioned this pull request Feb 17, 2019

Reland PerformanceOverlayLayer golden test #7863

Merged

liyuqian deleted the mock_performance_overlay branch May 23, 2019 18:45

Add mock capability to PerformanceOverlayLayer #7537

Add mock capability to PerformanceOverlayLayer #7537

Uh oh!

Conversation

liyuqian commented Jan 18, 2019

Uh oh!

liyuqian commented Jan 23, 2019

Uh oh!

chinmaygarde commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyuqian commented Jan 28, 2019

Uh oh!

liyuqian commented Jan 29, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jan 30, 2019

Uh oh!

liyuqian commented Jan 31, 2019

Uh oh!

Hixie commented Feb 3, 2019

Uh oh!

liyuqian commented Feb 5, 2019

Uh oh!

liyuqian commented Feb 6, 2019

Uh oh!

liyuqian commented Feb 8, 2019

Uh oh!

dnfield commented Feb 9, 2019

Uh oh!

dnfield commented Feb 9, 2019

Uh oh!

dnfield commented Feb 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chinmaygarde commented Jan 23, 2019 •

edited

Loading