Add timeout flag to devices command, pipe through discovery #51678

jmagman · 2020-02-29T01:50:43Z

Description

Add a --timeout flag to flutter devices so device discovery can use a potentially longer timeout. Use the timeout for finding iOS and Fuchsia devices. This will be needed so users can discover networked iOS devices that require a longer timeout than the default.

Also improves daemon polling behavior so the polling timeout can be piped down into the tools to override their default timeouts (like device-finder and xcdevice) instead of just killing the process.

Related Issues

Next part of #15072.

Tests

Added xcode_test, daemon_test, device_test, other device subclass tests.
There weren't any device-finder tests I could find. Will test this manually.

Checklist

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

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

jmagman · 2020-03-02T20:49:37Z

packages/flutter_tools/lib/src/fuchsia/fuchsia_dev_finder.dart

$ flutter devices -t 20 -v [ +3 ms] executing: /Users/m/Projects/flutter/bin/cache/artifacts/fuchsia/tools/device-finder list -full -timeout 20000ms [+20357 ms] 2020/03/02 12:48:39 no devices found

packages/flutter_tools/lib/src/commands/devices.dart

packages/flutter_tools/lib/src/fuchsia/fuchsia_dev_finder.dart

packages/flutter_tools/lib/src/commands/devices.dart

zanderso

See emailed comment. Also watch out for long lines. Lines can be longer that 80 iff they improve readability.

packages/flutter_tools/lib/src/commands/devices.dart

jonahwilliams · 2020-03-02T22:00:37Z

Change seems reasonable, but please verify if this requires updates for the google3 tool implementations. Adding a new named parameter to a subclass is not breaking, but adding it to a superclass isn't

jmagman · 2020-03-03T02:09:42Z

packages/flutter_tools/lib/src/device.dart

I needed to add a mechanism to not used the _items cache because in my next commit I'm going to be doing a quick default timeout, then longer when a particular ID is specified but not found. But since you guys pointed out how annoying adding default params is in g3 I'm going to do both in the same PR in case they get split over rolls.

Instead of adding a parameter for whether to use caching, you could also add a new method, like refreshDevices({Duration timeout}). If g3 is doing extends and not implement, adding a method won't be a breaking change.

zanderso

Can you test the behavior of the timeout parameter at some level using FakeAsync?

zanderso · 2020-03-03T16:53:50Z

packages/flutter_tools/lib/src/device.dart

Instead of adding a parameter for whether to use caching, you could also add a new method, like refreshDevices({Duration timeout}). If g3 is doing extends and not implement, adding a method won't be a breaking change.

jmagman · 2020-03-11T20:14:28Z

Can you test the behavior of the timeout parameter at some level using FakeAsync?

There's actually no dart code using this timeout (there was before in await pollingGetDevices().timeout(_pollingTimeout), but that is removed in this change, and it was untested). It's used to create timeout flags passed into fuchsia device-finder list and xcdevice list. I added a xcode_test to assert it's being parsed correctly. I spent a long time trying to add a single device-finder test (it's untested now) but that class was really difficult to test and now I forget why.

jmagman · 2020-03-12T23:44:09Z

Instead of adding a parameter for whether to use caching, you could also add a new method, like refreshDevices({Duration timeout}). If g3 is doing extends and not implement, adding a method won't be a breaking change.

I added this method, though this is already requires an update to the PollingDeviceDiscovery g3 subclasses due to pollingGetDevices signature change. Will coordinate with roller.

zanderso

lgtm w/ nits assuming pollingGetDevices was the only breaking change.

zanderso · 2020-03-13T21:58:28Z

packages/flutter_tools/lib/src/commands/devices.dart

How about:

globals.printStatus('If a connected device was not detected, please run "flutter doctor" to diagnose potential issues.'); if (timeout == null) { globals.printStatus('You may also try increasing the time to wait for connected devices with the --timeout flag.'); } globals.printStatus('visit https://flutter.dev/setup/ for troubleshooting tips.');

or make a StringBuffer to avoid multiple globals.printStatus() calls.

Before:

$ flutter devices No devices detected. Run 'flutter emulators' to list and start any available device emulators. Or, if you expected your device to be detected, please run "flutter doctor" to diagnose potential issues, or visit https://flutter.dev/setup/ for troubleshooting tips.

After:

flutter$ flutter devices No devices detected. Run "flutter emulators" to list and start any available device emulators. If you expected your device to be detected, please run "flutter doctor" to diagnose potential issues. You may also try increasing the time to wait for connected devices with the --timeout flag. Visit https://flutter.dev/setup/ for troubleshooting tips.

packages/flutter_tools/lib/src/device.dart

lock · 2020-04-02T15:41:48Z

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. platform-fuchsia Fuchsia code specifically work in progress; do not review labels Feb 29, 2020

jmagman self-assigned this Feb 29, 2020

googlebot added the cla: yes label Feb 29, 2020

jmagman force-pushed the timeout branch 2 times, most recently from 52d667f to 5ade80d Compare March 2, 2020 20:45

jmagman removed the work in progress; do not review label Mar 2, 2020

jmagman requested a review from zanderso March 2, 2020 20:45

jmagman commented Mar 2, 2020

View reviewed changes

jmagman requested a review from jonahwilliams March 2, 2020 21:28

jonahwilliams reviewed Mar 2, 2020

View reviewed changes

packages/flutter_tools/lib/src/commands/devices.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Mar 2, 2020

View reviewed changes

packages/flutter_tools/lib/src/fuchsia/fuchsia_dev_finder.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Mar 2, 2020

View reviewed changes

packages/flutter_tools/lib/src/commands/devices.dart Outdated Show resolved Hide resolved

zanderso reviewed Mar 2, 2020

View reviewed changes

packages/flutter_tools/lib/src/commands/devices.dart Outdated Show resolved Hide resolved

jmagman commented Mar 3, 2020

View reviewed changes

zanderso reviewed Mar 3, 2020

View reviewed changes

jmagman force-pushed the timeout branch 2 times, most recently from d8954aa to e54a57f Compare March 12, 2020 23:41

jmagman force-pushed the timeout branch from e54a57f to 3736ef6 Compare March 13, 2020 01:19

zanderso approved these changes Mar 13, 2020

View reviewed changes

Add timeout flag to devices command, pipe through discovery

def30ac

jmagman force-pushed the timeout branch from 3736ef6 to def30ac Compare March 16, 2020 18:50

jmagman merged commit 2f216ce into flutter:master Mar 16, 2020

jmagman deleted the timeout branch March 16, 2020 22:28

lock bot locked and limited conversation to collaborators Apr 2, 2020

Add timeout flag to devices command, pipe through discovery #51678

Add timeout flag to devices command, pipe through discovery #51678

Uh oh!

Conversation

jmagman commented Feb 29, 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!

jmagman Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Mar 2, 2020

Uh oh!

jmagman Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

zanderso Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

jmagman commented Mar 11, 2020

Uh oh!

jmagman commented Mar 12, 2020

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

jmagman Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lock bot commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmagman commented Feb 29, 2020 •

edited

Loading