From daeedab216e1b4505c1f43e4feea546799672d4e Mon Sep 17 00:00:00 2001
From: Natalie Weizenbaum <nweiz@google.com>
Date: Fri, 17 Apr 2015 15:16:23 -0700
Subject: [PATCH] More gracefully handle browser errors.
R=kevmoo@google.com
Review URL: https://codereview.chromium.org//1091883002
---
CHANGELOG.md | 2 ++
lib/src/runner/browser/chrome.dart | 29 +++++++++++++---------
lib/src/runner/browser/content_shell.dart | 20 ++++++++++++---
lib/src/runner/browser/dartium.dart | 29 +++++++++++++---------
lib/src/runner/browser/firefox.dart | 30 ++++++++++++++---------
lib/src/runner/browser/phantom_js.dart | 20 ++++++++++++---
lib/src/runner/browser/safari.dart | 19 +++++++++++---
lib/src/runner/browser/server.dart | 2 +-
8 files changed, 107 insertions(+), 44 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2bfa5202..1cd35ffd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@
* Fix running VM tests against `pub serve`.
+* More gracefully handle browser errors.
+
* Properly load Dartium from the Dart Editor when possible.
### 0.12.0-beta.8
diff --git a/lib/src/runner/browser/chrome.dart b/lib/src/runner/browser/chrome.dart
index fc4e2329..bca71a48 100644
--- a/lib/src/runner/browser/chrome.dart
+++ b/lib/src/runner/browser/chrome.dart
@@ -5,11 +5,15 @@
library test.runner.browser.chrome;
import 'dart:async';
+import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as p;
+import 'package:stack_trace/stack_trace.dart';
import '../../util/io.dart';
+import '../../utils.dart';
+import '../application_exception.dart';
import 'browser.dart';
// TODO(nweiz): move this into its own package?
@@ -25,12 +29,6 @@ class Chrome implements Browser {
/// The underlying process.
Process _process;
- /// The temporary directory used as the browser's user data dir.
- ///
- /// A new data dir is created for each run to ensure that they're
- /// well-isolated.
- String _dir;
-
Future get onExit => _onExitCompleter.future;
final _onExitCompleter = new Completer();
@@ -52,9 +50,8 @@ class Chrome implements Browser {
// for the process to actually start. They should just wait for the HTTP
// request instead.
withTempDir((dir) {
- _dir = dir;
return Process.start(executable, [
- "--user-data-dir=$_dir",
+ "--user-data-dir=$dir",
url.toString(),
"--disable-extensions",
"--disable-popup-blocking",
@@ -72,9 +69,19 @@ class Chrome implements Browser {
return _process.exitCode;
});
}).then((exitCode) {
- if (exitCode != 0) throw "Chrome failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+ if (exitCode == 0) return null;
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "Chrome failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start Chrome: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/content_shell.dart b/lib/src/runner/browser/content_shell.dart
index 2df48c32..6849e391 100644
--- a/lib/src/runner/browser/content_shell.dart
+++ b/lib/src/runner/browser/content_shell.dart
@@ -8,6 +8,9 @@ import 'dart:async';
import 'dart:convert';
import 'dart:io';
+import 'package:stack_trace/stack_trace.dart';
+
+import '../../utils.dart';
import '../application_exception.dart';
import 'browser.dart';
@@ -71,9 +74,20 @@ class ContentShell implements Browser {
"latest/dartium/");
}
- if (exitCode != 0) throw "Content shell failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+ if (exitCode == 0) return null;
+
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "Content shell failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start content shell: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/dartium.dart b/lib/src/runner/browser/dartium.dart
index f8d81513..627b17d3 100644
--- a/lib/src/runner/browser/dartium.dart
+++ b/lib/src/runner/browser/dartium.dart
@@ -5,11 +5,15 @@
library test.runner.browser.dartium;
import 'dart:async';
+import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as p;
+import 'package:stack_trace/stack_trace.dart';
import '../../util/io.dart';
+import '../../utils.dart';
+import '../application_exception.dart';
import 'browser.dart';
/// A class for running an instance of Dartium.
@@ -23,12 +27,6 @@ class Dartium implements Browser {
/// The underlying process.
Process _process;
- /// The temporary directory used as the browser's user data dir.
- ///
- /// A new data dir is created for each run to ensure that they're
- /// well-isolated.
- String _dir;
-
Future get onExit => _onExitCompleter.future;
final _onExitCompleter = new Completer();
@@ -50,9 +48,8 @@ class Dartium implements Browser {
// for the process to actually start. They should just wait for the HTTP
// request instead.
withTempDir((dir) {
- _dir = dir;
return Process.start(executable, [
- "--user-data-dir=$_dir",
+ "--user-data-dir=$dir",
url.toString(),
"--disable-extensions",
"--disable-popup-blocking",
@@ -70,9 +67,19 @@ class Dartium implements Browser {
return _process.exitCode;
});
}).then((exitCode) {
- if (exitCode != 0) throw "Dartium failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+ if (exitCode == 0) return null;
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "Dartium failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start Dartium: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/firefox.dart b/lib/src/runner/browser/firefox.dart
index 215f3a85..36eed533 100644
--- a/lib/src/runner/browser/firefox.dart
+++ b/lib/src/runner/browser/firefox.dart
@@ -5,11 +5,15 @@
library test.runner.browser.firefox;
import 'dart:async';
+import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as p;
+import 'package:stack_trace/stack_trace.dart';
import '../../util/io.dart';
+import '../../utils.dart';
+import '../application_exception.dart';
import 'browser.dart';
final _preferences = '''
@@ -29,12 +33,6 @@ class Firefox implements Browser {
/// The underlying process.
Process _process;
- /// The temporary directory used as the browser's user data dir.
- ///
- /// A new data dir is created for each run to ensure that they're
- /// well-isolated.
- String _dir;
-
Future get onExit => _onExitCompleter.future;
final _onExitCompleter = new Completer();
@@ -56,13 +54,11 @@ class Firefox implements Browser {
// for the process to actually start. They should just wait for the HTTP
// request instead.
withTempDir((dir) {
- _dir = dir;
-
new File(p.join(dir, 'prefs.js')).writeAsStringSync(_preferences);
return Process.start(executable, [
"--profile",
- "$_dir",
+ "$dir",
url.toString(),
"--no-remote"
], environment: {
@@ -76,9 +72,19 @@ class Firefox implements Browser {
return _process.exitCode;
});
}).then((exitCode) {
- if (exitCode != 0) throw "Firefox failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+ if (exitCode == 0) return null;
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "Firefox failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start Firefox: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/phantom_js.dart b/lib/src/runner/browser/phantom_js.dart
index 0eadafb4..c13ee63f 100644
--- a/lib/src/runner/browser/phantom_js.dart
+++ b/lib/src/runner/browser/phantom_js.dart
@@ -5,12 +5,15 @@
library test.runner.browser.phantom_js;
import 'dart:async';
+import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as p;
+import 'package:stack_trace/stack_trace.dart';
import '../../util/exit_codes.dart' as exit_codes;
import '../../util/io.dart';
+import '../../utils.dart';
import '../application_exception.dart';
import 'browser.dart';
@@ -82,9 +85,20 @@ class PhantomJS implements Browser {
throw new ApplicationException(
"Only PhantomJS version 2.0.0 or greater is supported.");
}
- if (exitCode != 0) throw "PhantomJS failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+
+ if (exitCode == 0) return null;
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "PhantomJS failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start PhantomJS: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/safari.dart b/lib/src/runner/browser/safari.dart
index 4ad28da4..af697850 100644
--- a/lib/src/runner/browser/safari.dart
+++ b/lib/src/runner/browser/safari.dart
@@ -9,8 +9,11 @@ import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as p;
+import 'package:stack_trace/stack_trace.dart';
import '../../util/io.dart';
+import '../../utils.dart';
+import '../application_exception.dart';
import 'browser.dart';
/// A class for running an instance of Safari.
@@ -59,9 +62,19 @@ class Safari implements Browser {
return _process.exitCode;
});
}).then((exitCode) {
- if (exitCode != 0) throw "Safari failed with exit code $exitCode.";
- }).then(_onExitCompleter.complete)
- .catchError(_onExitCompleter.completeError);
+ if (exitCode == 0) return null;
+
+ return UTF8.decodeStream(_process.stderr).then((error) {
+ throw new ApplicationException(
+ "Safari failed with exit code $exitCode:\n$error");
+ });
+ }).then(_onExitCompleter.complete).catchError((error, stackTrace) {
+ if (stackTrace == null) stackTrace = new Trace.current();
+ _onExitCompleter.completeError(
+ new ApplicationException(
+ "Failed to start Safari: ${getErrorMessage(error)}."),
+ stackTrace);
+ });
}
Future close() {
diff --git a/lib/src/runner/browser/server.dart b/lib/src/runner/browser/server.dart
index fb0e55ab..17862ebc 100644
--- a/lib/src/runner/browser/server.dart
+++ b/lib/src/runner/browser/server.dart
@@ -266,7 +266,7 @@ void main() {
// TODO(nweiz): Don't start the browser until all the suites are compiled.
return _browserManagerFor(browser).then((browserManager) {
- if (_closed) return null;
+ if (_closed || browserManager == null) return null;
return browserManager.loadSuite(path, suiteUrl, metadata);
}).then((suite) {
if (_closed) return null;
--
GitLab