Skip to content

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

Merged
jmagman merged 1 commit intoflutter:masterfrom
jmagman:timeout
Mar 16, 2020
Merged

Add timeout flag to devices command, pipe through discovery#51678
jmagman merged 1 commit intoflutter:masterfrom
jmagman:timeout

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Feb 29, 2020 •
edited
Loading

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 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 jmagman self-assigned this Feb 29, 2020
@jmagman jmagman force-pushed the timeout branch 2 times, most recently from 52d667f to 5ade80d Compare March 2, 2020 20:45
@jmagman jmagman requested a review from zanderso March 2, 2020 20:45
jmagman
jmagman commented Mar 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/fuchsia/fuchsia_dev_finder.dart Outdated
Comment on lines 34 to 37
Copy link
Member Author

@jmagman jmagman Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ 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

@jmagman jmagman requested a review from jonahwilliams March 2, 2020 21:28
jonahwilliams
jonahwilliams reviewed Mar 2, 2020
View reviewed changes
jonahwilliams
jonahwilliams reviewed Mar 2, 2020
View reviewed changes
jonahwilliams
jonahwilliams reviewed Mar 2, 2020
View reviewed changes
zanderso
zanderso reviewed Mar 2, 2020
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 2, 2020

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
jmagman commented Mar 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/device.dart Outdated
Comment on lines 287 to 288
Copy link
Member Author

@jmagman jmagman Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@zanderso zanderso Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
zanderso reviewed Mar 3, 2020
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/flutter_tools/lib/src/device.dart Outdated
Comment on lines 287 to 288
Copy link
Member

@zanderso zanderso Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

jmagman commented Mar 11, 2020

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 jmagman force-pushed the timeout branch 2 times, most recently from d8954aa to e54a57f Compare March 12, 2020 23:41
@jmagman
Copy link
Member Author

jmagman commented Mar 12, 2020

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
zanderso approved these changes Mar 13, 2020
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/flutter_tools/lib/src/commands/devices.dart Outdated
Copy link
Member

@zanderso zanderso Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jmagman jmagman Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jmagman jmagman merged commit 2f216ce into flutter:master Mar 16, 2020
@jmagman jmagman deleted the timeout branch March 16, 2020 22:28
@lock
Copy link

lock bot commented Apr 2, 2020

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.

@lock lock bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zanderso zanderso zanderso approved these changes

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

platform-fuchsia Fuchsia code specifically platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmagman @jonahwilliams @zanderso @googlebot