Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] using pkill to quit safari #17533

Merged
merged 8 commits into from
Apr 9, 2020
Merged

Conversation

nturgut
Copy link
Contributor

@nturgut nturgut commented Apr 6, 2020

felt was leaving lots of Safari tabs open after running unit test.

Quit safari after running felt with safari.

I used applescript since it is the most reliable (and easy way of) controlling MacOs applications. It is very easy to give commands to give commands to build in applications such as Safari, Finder, Xcode.

One time permission to run 'osascript' from the terminal should be given to luci bots before merging the PR.

Fixes flutter/flutter#53724

@nturgut nturgut force-pushed the kill_safari_tabs branch from 809d68b to f399a6a Compare April 6, 2020 19:59
@nturgut
Copy link
Contributor Author

nturgut commented Apr 6, 2020

@digiter we need to manually give permission to Terminal to control Safari.

System Preferences > Security & Privacy > Privacy > Automation. (Adding Terminal to Safari)

@yjbanov
Copy link
Contributor

yjbanov commented Apr 6, 2020

I don't think we check in binaries in our repos /cc @Hixie to confirm the policy

What's the source of the quit_safari.scpt file? Can we check in the source, build it, and then invoke?

@digiter
Copy link

digiter commented Apr 6, 2020

@godofredoc See Nurhan's comment above, I'm worried about that the setting of "System Preferences > Security & Privacy > Privacy > Automation. (Adding Terminal to Safari)" can only be set via GUI and not possible to be automated via scripts. Which would increase maintenance burden as we will have to do that on every web engine bot.

My suggestion is simply call sudo pkill -lf Safari, though we might need to tweak the command a bit to make sure it kills the right things.

@nturgut nturgut requested review from digiter and godofredoc April 6, 2020 21:17
@digiter
Copy link

digiter commented Apr 6, 2020

Besides, granting the Terminal app to control safari may not work, as LUCI recipes don't run in the Terminal app AFAIU.

@nturgut
Copy link
Contributor Author

nturgut commented Apr 6, 2020

Besides, granting the Terminal app to control safari may not work, as LUCI recipes don't run in the Terminal app AFAIU.

Ah, I didn't know that. We can add that application to the permission list from settings if we want to follow this path.

On the hand @yjbanov also does not want to use applescript since it is not saved as text in the source control repo.

@godofredoc
Copy link
Contributor

I'd suggest to use kill instead of adding something that requires configurations to the bots. Manual configurations are expensive and error prone, besides mac machines are configured by chromium team and we have limited access to them.

@yjbanov
Copy link
Contributor

yjbanov commented Apr 6, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

@nturgut
Copy link
Contributor Author

nturgut commented Apr 6, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

I am trying it with safari driver desktop now. I'll let you know if I'm able to close the browsers (cleanup tabs)

@nturgut
Copy link
Contributor Author

nturgut commented Apr 6, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

The driver is not usable for quitting safari. Delete session does not close the Safari Desktop. I closes the session successfully yet the browser stays open.

I'll go with Tong's suggestion pkill -lf Safari How does it sound to you?

@yjbanov
Copy link
Contributor

yjbanov commented Apr 6, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

The driver is not usable for quitting safari. Delete session does not close the Safari Desktop. I closes the session successfully yet the browser stays open.

Does closing the session close the tab? It is OK if the Safari process stays open as long as opening new sessions after closing old sessions does not result in accumulation of new tabs or new processes.

I'll go with Tong's suggestion pkill -lf Safari How does it sound to you?

If it works, then sure!

@yjbanov
Copy link
Contributor

yjbanov commented Apr 6, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

The driver is not usable for quitting safari. Delete session does not close the Safari Desktop. I closes the session successfully yet the browser stays open.

Does closing the session close the tab? It is OK if the Safari process stays open as long as opening new sessions after closing old sessions does not result in accumulation of new tabs or new processes.

I'll go with Tong's suggestion pkill -lf Safari How does it sound to you?

If it works, then sure!

One downside is that when testing locally it will be closing unrelated browsers that you might be using for something else.

@nturgut nturgut changed the title [web] adding an apple script to quit safari [web] using pkill to quit safari Apr 7, 2020
@nturgut
Copy link
Contributor Author

nturgut commented Apr 7, 2020

My gut feeling is that safaridriver is the best way to control the browser rather than open and applescript. This is what I would have tried first. I may be wrong though.

The driver is not usable for quitting safari. Delete session does not close the Safari Desktop. I closes the session successfully yet the browser stays open.

Does closing the session close the tab? It is OK if the Safari process stays open as long as opening new sessions after closing old sessions does not result in accumulation of new tabs or new processes.

I'll go with Tong's suggestion pkill -lf Safari How does it sound to you?

If it works, then sure!

One downside is that when testing locally it will be closing unrelated browsers that you might be using for something else.

Please check the current version. I am closing the Safari for CI environments (which will fix the issue), I'm only printing an info message for the local developers.

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be really nice to restore separation between commands. Can be done in a separate PR.

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM after addressing Yegor's comments.

@nturgut
Copy link
Contributor Author

nturgut commented Apr 9, 2020

Merging the PR. I checked all the tests are successfully ran. Looks like LUCI is delaying sending the results to github.

@nturgut nturgut merged commit 6245149 into flutter:master Apr 9, 2020
@nturgut
Copy link
Contributor Author

nturgut commented Apr 9, 2020

LGTM after addressing Yegor's comments.

Thanks I addressed the comments.

@nturgut
Copy link
Contributor Author

nturgut commented Apr 9, 2020

LGTM, but it would be really nice to restore separation between commands. Can be done in a separate PR.

Thanks I addressed the comments.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2020
goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020
* adding an apple script to quit safari

* making changes to utils and adding cleanup after unit tests failed

* switch saai quit to pkill instead of applescript

* adding toolexception

* adding a cleanup function option to felt.

* removing unnecassary await's added for the previos version of the PR

* removing unnecassary async

* fix analyze error
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web][safari] Tabs remain open on LUCI mac try bots
6 participants