Repository navigation
Add a few timeouts to FlutterDriver.connect() - #16762
Merged
Merged
Conversation
We're seeing occasional test timeouts trying to call `FlutterDriver.connect()`. Unfortunately, when the test is timed out at the test runner level, you don't get a meaningful test failure or stack trace of where the timeout occurrred. Your test harness also doesn't get to clean up, which can include things like saving the device logs to see what was going on in the device. Thuss, this change adds configurable timeouts in the places where we've most commonly observed hangs during tests.
yjbanov
reviewed
Apr 19, 2018
| _log.info('Connecting to Flutter application at $dartVmServiceUrl'); | ||
| final VMServiceClientConnection connection = await vmServiceConnectFunction(dartVmServiceUrl); | ||
| final VMServiceClientConnection connection = await vmServiceConnectFunction(dartVmServiceUrl) | ||
| .timeout(vmServiceConnectTimeout, onTimeout: () { |
Contributor
There was a problem hiding this comment.
.timeout does not cancel the original Future (futures cannot be cancelled in general). So after it times out the connection function will continue attempting to connect. If you look at the implementation of the default connection function _waitAndConnect it already has a timeout. It's just hard-coded to _kLongTimeout. A more robust strategy might be to customize that timeout value.
Contributor
Author
|
@yjbanov updated - PTAL |
Contributor
|
|
DaveShuckerow
pushed a commit
to DaveShuckerow/flutter
that referenced
this pull request
May 14, 2018
We're seeing occasional test timeouts trying to call `FlutterDriver.connect()`. Unfortunately, when the test is timed out at the test runner level, you don't get a meaningful test failure or stack trace of where the timeout occurrred. Your test harness also doesn't get to clean up, which can include things like saving the device logs to see what was going on in the device. Thus, this change adds timeouts in the places where we've most commonly observed hangs during tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We're seeing occasional test timeouts trying to call
FlutterDriver.connect().Unfortunately, when the test is timed out at the test runner level, you don't
get a meaningful test failure or stack trace of where the timeout occurrred.
Your test harness also doesn't get to clean up, which can include things like
saving the device logs to see what was going on in the device.
Thuss, this change adds configurable timeouts in the places where we've most
commonly observed hangs during tests.