Skip to content

Fix visibility of web server device when Chrome is not available#40757

Merged
DanTup merged 4 commits intoflutter:masterfrom
DanTup:fix-server-visibility
Sep 30, 2019
Merged

Fix visibility of web server device when Chrome is not available#40757
DanTup merged 4 commits intoflutter:masterfrom
DanTup:fix-server-visibility

Conversation

@DanTup
Copy link
Contributor

@DanTup DanTup commented Sep 18, 2019 •
edited
Loading

This removes the requirement of finding Chrome for all web devices, but does check it before providing the Chrome device.

Fixes #40814.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 18, 2019
@DanTup
Copy link
Contributor Author

DanTup commented Sep 18, 2019

I pushed some changes to the workflow test to remove the assumption that it can't list devices if there's no Chrome - however I'm not sure if this is the best way to change it (it's not clear if returning true for some flags when appliesToHostPlatform=false is valid - it seems like others do, but it means appliesToHostPlatform always needs checking before making assumptions about the other capabilities).

@codecov
Copy link

codecov bot commented Sep 18, 2019 •
edited
Loading

Codecov Report

Merging #40757 into master will decrease coverage by 0.46%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #40757      +/-   ##
==========================================
- Coverage   59.56%    59.1%   -0.47%     
==========================================
  Files         192      192              
  Lines       18683    18683              
==========================================
- Hits        11129    11042      -87     
- Misses       7554     7641      +87
Flag Coverage Δ
#flutter_tool 59.1% <90%> (-0.47%) ⬇️
Impacted Files Coverage Δ
...kages/flutter_tools/lib/src/web/web_validator.dart 75% <100%> (ø) ⬆️
packages/flutter_tools/lib/src/web/web_device.dart 51.28% <100%> (+5.12%) ⬆️
packages/flutter_tools/lib/src/web/workflow.dart 88.88% <100%> (+4.27%) ⬆️
packages/flutter_tools/lib/src/web/chrome.dart 50.74% <75%> (+1.53%) ⬆️
...lutter_tools/lib/src/android/android_workflow.dart 33.96% <0%> (-29.56%) ⬇️
...ackages/flutter_tools/lib/src/commands/attach.dart 46.8% <0%> (-27.66%) ⬇️
...ter_tools/lib/src/build_system/targets/assets.dart 70.9% <0%> (-16.37%) ⬇️
...ages/flutter_tools/lib/src/base/user_messages.dart 45.94% <0%> (-5.41%) ⬇️
packages/flutter_tools/lib/src/template.dart 92.06% <0%> (-4.77%) ⬇️
...ackages/flutter_tools/lib/src/commands/format.dart 75% <0%> (-4.17%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4c7e03...2c31a79. Read the comment docs.

jonahwilliams
jonahwilliams approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

The flags on Workflow are a bit weird, I agree. This seems correct though, its similar to an Android device discoverer having a valid SDK but no connected device.

LGTM

packages/flutter_tools/lib/src/web/web_device.dart
Future<List<Device>> pollingGetDevices() async {
return <Device>[
_webDevice,
if (_chromeIsAvailable)
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 18, 2019

Choose a reason for hiding this comment

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

This seems reasonable

@jonahwilliams jonahwilliams mentioned this pull request Sep 19, 2019
Closed
@DanTup
Copy link
Contributor Author

DanTup commented Sep 19, 2019

@jonahwilliams test failures were due to the tests relying on Chrome actually being on the underlying machine (because there were two checks - that we can find it, and that we can run it). I've pushed a change that moves canFindChrome onto ChromeLauncher so I could mock it in the tests - PTAL! 2c31a79

@DanTup
Copy link
Contributor Author

DanTup commented Sep 19, 2019

@jonahwilliams I'm out the office shortly for a week so if you're happy with the extra change I pushed, feel free to merge in my absence (@domesticmouse may be happy having the fix to try out :-))

@DanTup
Copy link
Contributor Author

DanTup commented Sep 30, 2019

@jonahwilliams ping! Still looking for confirmation of the changes I pushed (2c31a79) to resolve the test issues.

jonahwilliams
jonahwilliams approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM!

@DanTup DanTup merged commit 7e73cd7 into flutter:master Sep 30, 2019
@DanTup DanTup deleted the fix-server-visibility branch September 30, 2019 13:49
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@DanTup @Inconnu08
Fix visibility of web server device when Chrome is not available (flu…
3ca69fb 
…tter#40757)

* Fix visbility of web server device when Chrome is not available

* Add tests

* Update workflow test

* Fix tests to not rely on Chrome being on the underlying machine
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

flutter run -d server fails when Chrome not present

5 participants

@DanTup @jonahwilliams @blasten @googlebot @fluttergithubbot