From a55704a49a6605c16f799a64f8ab8aba586de1e3 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Tue, 15 Mar 2022 17:14:51 -0700 Subject: [PATCH 1/5] Add some retry around browser launching Towards #1578 Recursively attempt to launch the browser on the first few timeouts before throwing the ApplicationException. --- .../src/runner/browser/browser_manager.dart | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/pkgs/test/lib/src/runner/browser/browser_manager.dart b/pkgs/test/lib/src/runner/browser/browser_manager.dart index 5183cf8c4..9d1ad3600 100644 --- a/pkgs/test/lib/src/runner/browser/browser_manager.dart +++ b/pkgs/test/lib/src/runner/browser/browser_manager.dart @@ -98,11 +98,21 @@ class BrowserManager { /// Returns the browser manager, or throws an [ApplicationException] if a /// connection fails to be established. static Future start( + Runtime runtime, + Uri url, + Future future, + ExecutableSettings settings, + Configuration configuration) => + _start(runtime, url, future, settings, configuration, 1); + + static const _maxRetries = 3; + static Future _start( Runtime runtime, Uri url, Future future, ExecutableSettings settings, - Configuration configuration) { + Configuration configuration, + int attempt) { var browser = _newBrowser(url, runtime, settings, configuration); var completer = Completer(); @@ -129,9 +139,12 @@ class BrowserManager { return completer.future.timeout(Duration(seconds: 45), onTimeout: () { browser.close(); - throw ApplicationException( - 'Timed out waiting for ${runtime.name} to connect.\n' - 'Browser output: ${utf8.decode(browser.output)}'); + if (attempt >= _maxRetries) { + throw ApplicationException( + 'Timed out waiting for ${runtime.name} to connect.\n' + 'Browser output: ${utf8.decode(browser.output)}'); + } + return _start(runtime, url, future, settings, configuration, attempt++); }); } From 675245b953907769e2b5bad0a89e2c3882f6b883 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Tue, 15 Mar 2022 17:27:53 -0700 Subject: [PATCH 2/5] Restore previous timeout of 30 seconds The 45 second timeout was mainly to investigate whether extra time would resolve flakes. It didn't seem to help at all and it doesn't seem likely that even more time would help. Go back to 30 seconds to reduce the pain of hitting retries (though it will still be painful). --- pkgs/test/lib/src/runner/browser/browser_manager.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/test/lib/src/runner/browser/browser_manager.dart b/pkgs/test/lib/src/runner/browser/browser_manager.dart index 9d1ad3600..fc4a5e74b 100644 --- a/pkgs/test/lib/src/runner/browser/browser_manager.dart +++ b/pkgs/test/lib/src/runner/browser/browser_manager.dart @@ -137,7 +137,7 @@ class BrowserManager { completer.completeError(error, stackTrace); }); - return completer.future.timeout(Duration(seconds: 45), onTimeout: () { + return completer.future.timeout(Duration(seconds: 30), onTimeout: () { browser.close(); if (attempt >= _maxRetries) { throw ApplicationException( From 742b563bc9a5f4f8ad236d3ed9857980dce2a558 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Tue, 15 Mar 2022 17:29:41 -0700 Subject: [PATCH 3/5] Add changelog --- pkgs/test/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/pkgs/test/CHANGELOG.md b/pkgs/test/CHANGELOG.md index 8aea7798b..739371dd9 100644 --- a/pkgs/test/CHANGELOG.md +++ b/pkgs/test/CHANGELOG.md @@ -2,6 +2,7 @@ * Drop `dart2js-path` command line argument. * Allow loading tests under a path with the directory named `packages`. +* Add retry for launching browsers. Reduce timeout back to 30 seconds. ## 1.20.1 From 1d7a67b897d54bce2b0b91a8aaf77bdfccc9082c Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Wed, 16 Mar 2022 12:26:17 -0700 Subject: [PATCH 4/5] Don't retry forever --- pkgs/test/lib/src/runner/browser/browser_manager.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/test/lib/src/runner/browser/browser_manager.dart b/pkgs/test/lib/src/runner/browser/browser_manager.dart index fc4a5e74b..23c601099 100644 --- a/pkgs/test/lib/src/runner/browser/browser_manager.dart +++ b/pkgs/test/lib/src/runner/browser/browser_manager.dart @@ -144,7 +144,7 @@ class BrowserManager { 'Timed out waiting for ${runtime.name} to connect.\n' 'Browser output: ${utf8.decode(browser.output)}'); } - return _start(runtime, url, future, settings, configuration, attempt++); + return _start(runtime, url, future, settings, configuration, ++attempt); }); } From c7ff54d34d85cd52287b2f0a3eae597eccbb8177 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Wed, 16 Mar 2022 12:46:17 -0700 Subject: [PATCH 5/5] Drop -dev to publish without another PR --- pkgs/test/CHANGELOG.md | 2 +- pkgs/test/pubspec.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/test/CHANGELOG.md b/pkgs/test/CHANGELOG.md index 739371dd9..11faa50c9 100644 --- a/pkgs/test/CHANGELOG.md +++ b/pkgs/test/CHANGELOG.md @@ -1,4 +1,4 @@ -## 1.20.2-dev +## 1.20.2 * Drop `dart2js-path` command line argument. * Allow loading tests under a path with the directory named `packages`. diff --git a/pkgs/test/pubspec.yaml b/pkgs/test/pubspec.yaml index 1f3123288..237dd4133 100644 --- a/pkgs/test/pubspec.yaml +++ b/pkgs/test/pubspec.yaml @@ -1,5 +1,5 @@ name: test -version: 1.20.2-dev +version: 1.20.2 description: >- A full featured library for writing and running Dart tests across platforms. repository: https://github.com/dart-lang/test/blob/master/pkgs/test