[web] move browser installation to BrowserEnvironment.prepare #29801
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
||
| print('INFO: Starting Chrome download.'); | ||
|
|
||
| final String url = PlatformBinding.instance.getChromeDownloadUrl(version); |
There was a problem hiding this comment.
Reading this code, it looks like we always re-download Chrome even if LUCI has provided us with a Chrome from CIPD. In fact, in the code above, we check to make sure that LUCI provided a Chrome installation, but here we download it again anyway.
There was a problem hiding this comment.
Good point. We actually shouldn't enter this method when running on LUCI because the call is predicated on installer.isInstalled above:
if (installer.isInstalled) {
infoLog.writeln(
'Installation was skipped because Chrome version ${installer.version} is already installed.');
}
} else {
// calls this method
}I'm going to assert here that we're not running on LUCI and see what happens.
lib/web_ui/dev/browser.dart
Outdated
| /// | ||
| /// This may include things like staring web drivers, iOS Simulators, and/or | ||
| /// Android emulators. | ||
| /// This may include things like installing browsers, and staring web drivers, |
There was a problem hiding this comment.
| /// This may include things like installing browsers, and staring web drivers, | |
| /// This may include things like installing browsers, and starting web drivers, |
cc3485e to
d4f0e55
Compare
This way browser installation happens early in the build process, and if it fails we get reasonable errors in the output.
Prior to this PR the browser was installed on-demand while the test framework was attempting to run a test. If the browser fails to install the test simply hangs (internally it is awaiting for the browser-localhost handshake), and there's no log output (probably because the test framework sets up a custom zone and intercepts console output).