Fix the firebase scenario app run and assert that it does good things #27295

dnfield · 2021-07-09T21:31:24Z

Currently, the scenario app is failing and we're not bothering to check whether it passed or failed.

GameLoop tests pass as long as the application doesn't completely crash.

Currently, the Dart program fails because it tries to print the property of a null object. I'm also fixing up some things that would emit errors and make the log checking harder.

fixes flutter/flutter#86191

flutter-dashboard · 2021-07-09T21:31:26Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dnfield · 2021-07-09T21:31:35Z

This PR does in fact have tests.

xster · 2021-07-12T16:52:46Z

testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestActivity.java

        new MethodChannel(getFlutterEngine().getDartExecutor(), "driver", JSONMethodCodec.INSTANCE);
    Map<String, Object> test = new HashMap<>(2);
-    test.put("name", launchIntent.getStringExtra("scenario"));
+    test.put("name", "animated_color_square");


is this right? Seems like this is the mechanism to control which test to run no?

The launch intent we get here is an integer, not a string. The method channel used expects a string with the name of the test.

launchIntent.getStringExtra("scenario") should equal to the values defined in

engine/testing/scenario_app/android/app/src/androidTest/java/dev/flutter/scenariosui/PlatformViewUiTests.java

Line 36 in 2b11d09

intent.putExtra("scenario", "platform_view");

We should use a different extra name though - because firebase testlab sets this as an integer.

is "scenario" a reserved word by Firebase?

For gameloops yes.

oh lol. I was wondering why we never caught this. Renaming sg. We need to do it everywhere in this project to keep things working.

Done - renamed the intent extra to scenario_name for these and use it if it's available.

Awesome! thanks

xster · 2021-07-12T16:53:34Z

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

+   * @param viewportMetrics The metrics to send to the Dart application.
+   */
  public void setViewportMetrics(@NonNull ViewportMetrics viewportMetrics) {
+    if (!viewportMetrics.validate()) {


what's the edge case here? Leave a code comment for the next person?

also, should it throw?

We call this method when we get DPR updates, which can happen before we get width/height. The embedding ends up calling this with just the DPR sometimes.

Added a comment

xster · 2021-07-12T16:54:03Z

ci/firebase_testlab.sh

+  exit 1
+fi
+
+result_size=$(gsutil du gs://flutter_firebase_testlab/engine_scenario_test/deadbeef/dnfield1/\*/game_loop_results/results_scenario_0.json | cut -d " " -f1)


did you want to leave these personal paths in here?

blasten · 2021-07-12T17:21:23Z

ci/firebase_testlab.sh

  --results-dir="engine_scenario_test/$GIT_REVISION/$BUILD_ID" \
  --device model=flame,version=29
+
+errors=$(gsutil cat gs://flutter_firebase_testlab/engine_scenario_test/$GIT_REVISION/$BUILD_ID/\*/logcat | grep "[FE]/flutter" | true)


what does FE stand for? is it Flutter engine?

Look for fatal or error warnings, i.e. F/flutter or E/flutter

ah nice! I learned something new about grep

blasten · 2021-07-12T18:10:29Z

testing/scenario_app/lib/main.dart

+  if (data != null) {
+    print('$name = ${utf8.decode(data.buffer.asUint8List())}');
+  } else {
+    print(name);


nit: print('$name = null');

blasten

LGTM

xster · 2021-07-12T18:39:11Z

testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestActivity.java

        new MethodChannel(getFlutterEngine().getDartExecutor(), "driver", JSONMethodCodec.INSTANCE);
    Map<String, Object> test = new HashMap<>(2);
-    test.put("name", launchIntent.getStringExtra("scenario"));
+    test.put("name", "animated_color_square");


oh lol. I was wondering why we never caught this. Renaming sg. We need to do it everywhere in this project to keep things working.

xster

Thanks

…d things (flutter/engine#27295)

…flutter#27295)

Fix the firebase scenario app run and assert that it does good things

4bc14f6

dnfield requested review from blasten, jason-simmons and xster July 9, 2021 21:31

flutter-dashboard bot added platform-android needs tests labels Jul 9, 2021

google-cla bot added the cla: yes label Jul 9, 2021

dnfield mentioned this pull request Jul 10, 2021

Build rules for scenario_app #27302

Merged

xster reviewed Jul 12, 2021

View reviewed changes

dnfield added 2 commits July 12, 2021 10:19

Update firebase_testlab.sh

ae161de

Update FlutterRenderer.java

2b11d09

blasten reviewed Jul 12, 2021

View reviewed changes

blasten approved these changes Jul 12, 2021

View reviewed changes

xster suggested changes Jul 12, 2021

View reviewed changes

dnfield added 4 commits July 12, 2021 15:31

Merge remote-tracking branch 'upstream/master' into fix_ftl

5ca7f89

unused var

abf3902

Merge branch 'fix_ftl' of github.com:dnfield/engine into fix_ftl

a5e16ee

scenario -> scenario_name

9aabf0b

xster approved these changes Jul 12, 2021

View reviewed changes

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 12, 2021

unneded import, forgot to save file before commit

9139f0e

fluttergithubbot merged commit d8ff243 into flutter:master Jul 12, 2021

engine-flutter-autoroll mentioned this pull request Jul 13, 2021

Roll Engine from a984a833baaf to 0bf8bfdf1796 (5 revisions) flutter/flutter#86350

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2021

d8ff243 Fix the firebase scenario app run and assert that it does goo…

85ca53c

…d things (flutter/engine#27295)

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Fix the firebase scenario app run and assert that it does good things (…

fdcf578

…flutter#27295)

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Fix the firebase scenario app run and assert that it does good things (…

f9897bc

…flutter#27295)

Fix the firebase scenario app run and assert that it does good things #27295

Fix the firebase scenario app run and assert that it does good things #27295

Uh oh!

Conversation

dnfield commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jul 9, 2021

Uh oh!

dnfield commented Jul 9, 2021

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Jul 9, 2021 •

edited

Loading