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.

Sign up for GitHub

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

Retry when safaridriver fails #48791

Merged
merged 2 commits into from
Dec 7, 2023
Merged

Retry when safaridriver fails #48791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12c74c7
Retry when safaridriver fails
vashworth Dec 7, 2023
c2a7edf
revert config and update retry count
vashworth Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions lib/web_ui/dev/safari_macos.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'package:test_api/src/backend/runtime.dart';
Expand All @@ -23,6 +24,58 @@ class SafariMacOsEnvironment extends WebDriverBrowserEnvironment {
@override
Uri get driverUri => Uri(scheme: 'http', host: 'localhost', port: portNumber);

late Process _driverProcess;
int _retryCount = 0;
static const int _waitBetweenRetryInSeconds = 1;
static const int _maxRetryCount = 10;

@override
Future<Process> spawnDriverProcess() => Process.start('safaridriver', <String>['-p', portNumber.toString()]);

@override
Future<void> prepare() async {
await _startDriverProcess();
}

/// Pick an unused port and start `safaridriver` using that port.
///
/// On macOS 13, starting `safaridriver` can be flaky so if it returns an
/// "Operation not permitted" error, kill the `safaridriver` process and try
/// again with a different port. Wait [_waitBetweenRetryInSeconds] seconds
/// between retries. Try up to [_maxRetryCount] times.
Future<void> _startDriverProcess() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this isn't just the implementation of spawnDriverProcess rather than overriding prepare?

Copy link
Contributor Author

@vashworth vashworth Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not really, mostly just to mimic the prepare function in webdriver_browser.dart. Like picking the port and starting the stderr/stdout listeners happens in prepare rather than the spawnDriverProcess. I suppose I could have just used the prepare function rather that _startDriverProcess. What would you suggest?

^^ I read the question wrong.

Yes, in webdriver_browser.dart prepare, it sets the _driverProcess to the result of spawnDriverProcess, but we want to call spawnDriverProcess and update _driverProcess multiple times depending on the stderr from _driverProcess. We could move the _startDriverProcess to webdriver_browser.dart's prepare, but I wasn't sure how other implementations of WebDriverBrowserEnvironment are used so I thought it safer to make the logic exclusive to SafariMacOsEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the clarification!

_retryCount += 1;
if (_retryCount > 1) {
await Future<void>.delayed(const Duration(seconds: _waitBetweenRetryInSeconds));
}
portNumber = await pickUnusedPort();

print('Attempt $_retryCount to start safaridriver on port $portNumber');

_driverProcess = await spawnDriverProcess();

_driverProcess.stderr
.transform(utf8.decoder)
.transform(const LineSplitter())
.listen((String error) {
print('[Webdriver][Error] $error');
if (_retryCount > _maxRetryCount) {
print('[Webdriver][Error] Failed to start after $_maxRetryCount tries.');
} else if (error.contains('Operation not permitted')) {
_driverProcess.kill();
_startDriverProcess();
}
});
_driverProcess.stdout
.transform(utf8.decoder)
.transform(const LineSplitter())
.listen((String log) {
print('[Webdriver] $log');
});
}

@override
Future<void> cleanup() async {
_driverProcess.kill();
}
}
6 changes: 3 additions & 3 deletions lib/web_ui/dev/webdriver_browser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import 'package:webdriver/async_io.dart' show WebDriver, createDriver;
import 'browser.dart';

abstract class WebDriverBrowserEnvironment extends BrowserEnvironment {
late final int portNumber;
late int portNumber;
late final Process _driverProcess;

Future<Process> spawnDriverProcess();
Uri get driverUri;

/// Finds and returns an unused port on the test host in the local port range.
Future<int> _pickUnusedPort() async {
Future<int> pickUnusedPort() async {
// Use bind to allocate an unused port, then unbind from that port to
// make it available for use.
final ServerSocket socket = await ServerSocket.bind('localhost', 0);
Expand All @@ -33,7 +33,7 @@ abstract class WebDriverBrowserEnvironment extends BrowserEnvironment {

@override
Future<void> prepare() async {
portNumber = await _pickUnusedPort();
portNumber = await pickUnusedPort();

_driverProcess = await spawnDriverProcess();

Expand Down