Skip to content

Move to Safari drive to try to make safari testing less flaky#2598

Open
kevmoo wants to merge 6 commits intomasterfrom
safari_sily
Open

Move to Safari drive to try to make safari testing less flaky#2598
kevmoo wants to merge 6 commits intomasterfrom
safari_sily

Conversation

@kevmoo
Copy link
Member

@kevmoo kevmoo commented Feb 23, 2026

No description provided.

@github-actions
Copy link

github-actions bot commented Feb 23, 2026

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

@kevmoo kevmoo marked this pull request as ready for review February 24, 2026 20:07
@kevmoo kevmoo requested a review from a team as a code owner February 24, 2026 20:07
@kevmoo
Copy link
Member Author

kevmoo commented Feb 24, 2026

@natebosch – I fixed some things up and added a changelog entry

Runtime.safari: ExecutableSettings(
macOSExecutable: '/Applications/Safari.app/Contents/MacOS/Safari',
macOSExecutable: 'safaridriver',
environmentOverride: 'SAFARI_EXECUTABLE',
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this to SAFARI_DRIVER_EXECUTABLE?

I'm a little worried about breaking existing users of this variable, but I think overall I still lean towards supporting only the safaridriver flow.

port.toString(),
]);
} catch (_) {
stderr.writeln('safaridriver failed to start.');
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we write directly to stderr except for in higher layers of the runner.

I think we want to throw a LoadException and we can put the extra user help there.

Similar below.

Comment on lines +129 to +130
# Authenticate safaridriver for the current session
sudo safaridriver --enable
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, the sudo isn't universally required. I think we can show the command without it.

What does "for the current session" mean here?

Suggested change
# Authenticate safaridriver for the current session
sudo safaridriver --enable
# Authenticate safaridriver (may need sudo)
safaridriver --enable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants