[devicelab] await flutter create call in platform channels benchmark #82762

jonahwilliams · 2021-05-17T22:59:33Z

The flutter create call was not awaited. This could lead to the application not being full created before the benchmark starts.

#82743

flutter-dashboard · 2021-05-17T22:59:36Z

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.

Hixie · 2021-05-17T23:01:24Z

test-exempt: is a test

zanderso · 2021-05-17T23:29:21Z

dev/devicelab/lib/tasks/platform_channels_benchmarks.dart

@@ -31,7 +31,7 @@ TaskFunction runTask(adb.DeviceOperatingSystem operatingSystem) {
        '.'
      ];
      print('\nExecuting: $flutterExe $createArgs $appDir');
-      utils.eval(flutterExe, createArgs);
+      await utils.eval(flutterExe, createArgs);


Why was this not caught by the unawaited Futures lint?

good question, haven't looked into that yet but I suspect that we're either accidentally ignoring part of the repo for lints or we're ticking a lint bug somehow

Looks like this is the cause:

flutter/analysis_options.yaml

Line 198 in 94f9a28

# - unawaited_futures # too many false positives

Pinging @dnfield since that was his PR that disabled that warning. In this case it would have been helpful since I was using a method I didn't author. It was easy to assume the call was synchronous. Is the linter warning not smart enough to catch .then usage? It seems like we'd want to for someone to await or have a .then()

Another way to have avoided this is to have a linter error if you implicitly discard a return value. I always liked languages that disallowed that.

I'm in the blame but it was for cleanup/bulk moving/revert. It was determined to be WONTFIX here: #5793

@gaaclarke See unawaited in the flutter_tools here https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/common.dart#L36

We could turn it on but add an extension method to futures called discard() so that foobar() turns into foobar().discard(). It probably wouldn't be much help without the auto fixer knowing about it.

dart-lang/sdk#46047

The lint doesn't make much sense in Flutter code (a lot of Flutter APIs hand back Futures that are informative but never throw and whose completion does not generally signal a change in API availability and so don't need to be awaited), which is why it's disabled. Enabling it for non-Flutter code (like we have it enabled for the tool) makes sense to me.

FWIW in flutter_test we have runtime verification that await is being called on appropriate APIs, we could do something similar if this is a specific API that's bug-prone in this way. (Look for the async guard API in flutter_test.)

fluttergithubbot · 2021-05-17T23:59:03Z

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

The status or check suite Mac build_aar_module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac gradle_plugin_fat_apk_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot · 2021-05-18T00:19:04Z

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

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

jonahwilliams · 2021-05-18T00:23:31Z

unfortunately the build sadness is going to make this hard to land

fluttergithubbot · 2021-05-18T00:24:05Z

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

The status or check suite Mac module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac build_aar_module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac gradle_plugin_fat_apk_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

jonahwilliams · 2021-05-18T00:48:43Z

these failures seem unrelated but I'm unable to get them to pass by re-running

zanderso · 2021-05-18T02:03:22Z

It looks like the failures are all

[module_test] [STDOUT] ══════════════════════╡ ••• Check files in debug APK ••• ╞══════════════════════
[module_test] [STDOUT] 
[module_test] [STDOUT] 
[module_test] [STDOUT] Executing: /opt/s/w/ir/cache/android/cmdline-tools/latest/bin/apkanalyzer files list /opt/s/w/ir/x/t/flutter_module_test.bz34SN/hello_host_app/app/build/outputs/apk/debug/app-debug.apk in /opt/s/w/ir/x/w/recipe_cleanup/tmpzQkjtx/flutter sdk/dev/devicelab with environment {JAVA_HOME: /opt/s/w/ir/cache/java/contents/Home, BOT: true, LANG: en_US.UTF-8}
[module_test] [STDOUT] stderr: Exception in thread "main" java.lang.IllegalStateException: The tools directory property is not set, please make sure you are executing apkanalyzer. Got /opt/s/w/ir/cache/android/cmdline-tools/.cipd/pkgs/0/4q2dskfQtDcMDu1cxApAyCW1TIE-vpp-CApEoNMH3hoC/latest
[module_test] [STDOUT] stderr: 	at com.android.tools.apk.analyzer.ApkAnalyzerCli.getAaptInvokerFromSdk(ApkAnalyzerCli.java:266)
[module_test] [STDOUT] stderr: 	at com.android.tools.apk.analyzer.ApkAnalyzerCli.main(ApkAnalyzerCli.java:123)
[module_test] [STDOUT] "/opt/s/w/ir/cache/android/cmdline-tools/latest/bin/apkanalyzer" exit code: 1

Which is maybe an issue with the cmdline-tools on the bots?

zanderso · 2021-05-18T16:05:01Z

@jonahwilliams I forgot to ask if there is an issue open for adding the unawaited Futures check in a way that will cover this kind of code. Should I file one?

jonahwilliams · 2021-05-18T16:21:30Z

I did not file one

[devicelab] await flutter create call in platform channels benchmark

b489743

flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label May 17, 2021

jonahwilliams requested a review from gaaclarke May 17, 2021 22:59

google-cla bot added the cla: yes label May 17, 2021

gaaclarke approved these changes May 17, 2021

View reviewed changes

jonahwilliams added the waiting for tree to go green label May 17, 2021

zanderso reviewed May 17, 2021

View reviewed changes

gaaclarke mentioned this pull request May 17, 2021

Mark mac_ios_platform_channels_benchmarks_ios not flaky #82759

Merged

fluttergithubbot removed the waiting for tree to go green label May 17, 2021

jonahwilliams added the waiting for tree to go green label May 18, 2021

fluttergithubbot removed the waiting for tree to go green label May 18, 2021

jonahwilliams added the waiting for tree to go green label May 18, 2021

fluttergithubbot removed the waiting for tree to go green label May 18, 2021

zanderso mentioned this pull request May 18, 2021

Several mac/android presubmits failing on apkanalyzer #82774

Closed

zanderso approved these changes May 18, 2021

View reviewed changes

jonahwilliams merged commit d6fa9bb into flutter:master May 18, 2021

jonahwilliams deleted the fix_platform_channels_bench branch May 18, 2021 15:55

zanderso mentioned this pull request May 18, 2021

Enable the unawaited Futures lint under dev/devicelab #82829

Closed

jmagman mentioned this pull request May 24, 2021

Mark linux_platform_channels_benchmarks unflaky #83295

Merged

[devicelab] await flutter create call in platform channels benchmark #82762

[devicelab] await flutter create call in platform channels benchmark #82762

Uh oh!

Conversation

jonahwilliams commented May 17, 2021

Uh oh!

flutter-dashboard bot commented May 17, 2021

Uh oh!

Hixie commented May 17, 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!

fluttergithubbot commented May 17, 2021

Uh oh!

fluttergithubbot commented May 18, 2021

Uh oh!

jonahwilliams commented May 18, 2021

Uh oh!

fluttergithubbot commented May 18, 2021

Uh oh!

jonahwilliams commented May 18, 2021

Uh oh!

zanderso commented May 18, 2021

Uh oh!

zanderso commented May 18, 2021

Uh oh!

jonahwilliams commented May 18, 2021

Uh oh!

Uh oh!