Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web] use a different method for launching desktop safari #27567

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Jul 19, 2021

Replace open -F -W -n -b com.apple.Safari with a direct invocation of the /Applications/Safari.app/Contents/MacOS/Safari executable.

Problems with open:

  • The open process itself is short-lived and exits immediately after launching Safari. This is contrary to the assumptions made by the rest of the code that the lifetime of the process returned by the Browser class corresponds to the browser process.
  • The Safari process is left open after the test is finished.
  • The open command and/or com.apple.Safari also open the "default browser". For example, if your default browser is set to Chrome, then the test will also open in Chrome.

The method used here is borrowed from Karma: karma-runner/karma-safari-launcher#29

This also removes the "clear console" functionality when felt is run in normal mode (as opposed to watch mode), which could be messing with LUCI logs.

This also removes the printouts of macOS info, because commands we use to extract this info are flaky.

Fixes flutter/flutter#86676

@flutter-dashboard
Copy link

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.

@flutter-dashboard flutter-dashboard bot added platform-web Code specifically for the web engine needs tests labels Jul 19, 2021
@google-cla google-cla bot added the cla: yes label Jul 19, 2021
@yjbanov
Copy link
Contributor Author

yjbanov commented Jul 20, 2021

Merging on red LUCI to fix the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes needs tests platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LUCI timeout on Mac Web Engine/felt test safari desktop
2 participants