[flutter_tools] Add timeout duration to error and handle exceptions for HttpHostValidator. by royarg02 · Pull Request #98290 · flutter/flutter

royarg02 · 2022-02-11T21:37:15Z

Includes the timeout duration, handles exceptions for invalid FLUTTER_DOCTOR_HOST_TIMEOUT, PUB_HOSTED_URL or FLUTTER_STORAGE_BASE_URL environment variables of HttpHostValidator.

Also includes some code hand-formatting/cleanup for readability.

Adds tests to make sure the timeout is being displayed and exceptions are handled accordingly.

Fixes #98286.
Fixes #98328.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

royarg02 · 2022-02-11T21:38:35Z

packages/flutter_tools/lib/src/http_host_validator.dart

Not sure why the .toList was needed here

Why did you condense lines 89-92 into one? I feel like it is more difficult to read as a one-liner.

So should I reintroduce availabilityResultFutures?

If it was just a style refactor, I would prefer having availabilityResultFutures

I agree that .toList() was redundant

flutter-dashboard · 2022-02-12T06:18:44Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

royarg02 · 2022-02-13T06:33:29Z

packages/flutter_tools/lib/src/http_host_validator.dart

We already mention $host before this message, no need to mention it again.

royarg02 · 2022-02-13T06:37:46Z

packages/flutter_tools/lib/src/http_host_validator.dart

e.toString() => e.message

royarg02 · 2022-02-14T12:58:49Z

packages/flutter_tools/test/commands.shard/hermetic/http_host_validator_test.dart

Duplicate test

yikes, good catch

royarg02 · 2022-02-14T12:59:09Z

packages/flutter_tools/test/commands.shard/hermetic/http_host_validator_test.dart

Duplicate test

packages/flutter_tools/lib/src/http_host_validator.dart

christopherfujino · 2022-02-24T22:29:45Z

packages/flutter_tools/lib/src/http_host_validator.dart

can this also be lifted outside of the try block?

I can't, see #98328.

Are you catching FormatException?

Not in this PR, but in the (future)PR that fixes that issue, and I see no point in moving this line when eventually it has to be moved back.

any reason not to catch and toolexit on formatexception in this PR?

Nothing specific, thought it would be better to land separate PRs to fix separate issues, but I can include all in one if you are okay with that.

Sounds good to me as long as it's not a lot more code, which I imagine it wouldn't be?

@christopherfujino I've pushed a commit that handles all possible exceptions, PTAL.

christopherfujino

LGTM

fluttergithubbot · 2022-03-07T17:46:14Z

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

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

christopherfujino · 2022-03-07T18:06:08Z

@Jasguerrero can you give another review on this one?

…ptions for HttpHostValidator. (flutter/flutter#98290)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 11, 2022

royarg02 commented Feb 11, 2022

View reviewed changes

royarg02 marked this pull request as draft February 12, 2022 06:14

royarg02 force-pushed the doctor_host_validator_timeout branch from 69f00bb to fbf784e Compare February 13, 2022 06:31

royarg02 commented Feb 13, 2022

View reviewed changes

packages/flutter_tools/lib/src/http_host_validator.dart Outdated

Copy link

Contributor Author

royarg02 Feb 13, 2022

Choose a reason for hiding this comment

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

e.toString() => e.message

royarg02 marked this pull request as ready for review February 13, 2022 07:32

royarg02 commented Feb 14, 2022

View reviewed changes

packages/flutter_tools/test/commands.shard/hermetic/http_host_validator_test.dart Outdated

Copy link

Contributor Author

royarg02 Feb 14, 2022

Choose a reason for hiding this comment

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

Duplicate test

royarg02 force-pushed the doctor_host_validator_timeout branch 2 times, most recently from 1140d90 to 3e6c6fb Compare February 18, 2022 07:27

christopherfujino reviewed Feb 24, 2022

View reviewed changes

packages/flutter_tools/lib/src/http_host_validator.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Feb 24, 2022

View reviewed changes

royarg02 force-pushed the doctor_host_validator_timeout branch from 3e6c6fb to 2aa5a65 Compare February 26, 2022 05:02

christopherfujino approved these changes Feb 28, 2022

View reviewed changes

royarg02 changed the title ~~[flutter_tools] Include timeout duration in HttpHostValidator error~~ [flutter_tools] Add timeout duration to error and handle exceptions for HttpHostValidator. Feb 28, 2022

royarg02 force-pushed the doctor_host_validator_timeout branch 2 times, most recently from a4061bd to cbde691 Compare March 1, 2022 06:47

royarg02 added 5 commits March 6, 2022 12:43

[flutter_tools] Include timeout duration in HttpHostValidator error

59d24fc

Remove duplicate tests

367d337

Fix review nits

e65a40d

Specific error messages

764c7d3

Un-nest else blocks

6c4cd61

royarg02 force-pushed the doctor_host_validator_timeout branch from cbde691 to 6c4cd61 Compare March 6, 2022 07:14

christopherfujino added the waiting for tree to go green label Mar 7, 2022

fluttergithubbot removed the waiting for tree to go green label Mar 7, 2022

christopherfujino requested a review from Jasguerrero March 7, 2022 18:05

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to f0524d542b2a (111 revisions) flutter/packages#1142

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

cc4844f

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to dbed10bb5920 (112 revisions) flutter/packages#1143

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

cb20720

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 340d1ac40fff (113 revisions) flutter/packages#1144

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

ccfc4b8

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 39f31f0e3035 (114 revisions) flutter/packages#1146

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

d7df7ef

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 39f31f0e3035 (114 revisions) flutter/packages#1148

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

c9054fc

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 39f31f0e3035 (114 revisions) flutter/packages#1149

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

5a02b5e

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to b4abc233ee18 (115 revisions) flutter/packages#1150

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

4e9dbac

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 9cfaabaed09e (116 revisions) flutter/packages#1151

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

2ed6228

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to cff36b946ea8 (117 revisions) flutter/packages#1153

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

cc77ee7

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 035eeb532e29 (118 revisions) flutter/packages#1154

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

9d48633

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 400de160ec37 (119 revisions) flutter/packages#1155

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

241286a

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to e8c7a88914e5 (120 revisions) flutter/packages#1156

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

fcbffe4

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 010910f71cd7 (121 revisions) flutter/packages#1157

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

1aff93d

…ptions for HttpHostValidator. (flutter/flutter#98290)

engine-flutter-autoroll mentioned this pull request Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to e8af40f06b24 (122 revisions) flutter/packages#1158

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 9, 2022

b021507 [flutter_tools] Add timeout duration to error and handle exce…

cf5dc3c

…ptions for HttpHostValidator. (flutter/flutter#98290)

This was referenced Mar 9, 2022

Roll Flutter from 38dbbb17f8ce to 0aab22807c2b (123 revisions) flutter/packages#1159

Closed

Roll Flutter from 38dbbb17f8ce to 291b28641740 (124 revisions) flutter/packages#1160

Closed

Conversation

royarg02 commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

flutter-dashboard bot commented Feb 12, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royarg02 Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royarg02 Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Mar 7, 2022

Uh oh!

christopherfujino commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

royarg02 commented Feb 11, 2022 •

edited

Loading

royarg02 Feb 25, 2022 •

edited

Loading

royarg02 Feb 26, 2022 •

edited

Loading