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.

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

Revert "Retry browser navigation in WCT if the browser doesn't connect. (#680)" #697

Merged
merged 1 commit into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 8 additions & 61 deletions packages/web-component-tester/runner/browserrunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,6 @@ export interface BrowserDef extends wd.Capabilities {
variant?: string;
}

/**
* Returns a promise that resolves after a given duration to an optional
* result.
*
* @param duration The time, in milliseconds, after which the promise should
* resolve.
* @param result The result to resolve the promise to.
*/
const timeout = (duration: number, result?: any) =>
new Promise(resolve => setTimeout(() => resolve(result), duration));

// Browser abstraction, responsible for spinning up a browser instance via wd.js
// and executing runner.html test files passed in options.files
export class BrowserRunner {
Expand All @@ -65,11 +54,6 @@ export class BrowserRunner {
private _resolve: () => void;
private _reject: (err: any) => void;

private _navigationAttemptCount: number;
private _navigationAttemptTimeout: number;
private _navigationSucceeded: Promise<void>;
private _resolveNavigationSucceeded: Function;

/**
* @param emitter The emitter to send updates about test progress to.
* @param def A BrowserDef describing and defining the browser to be run.
Expand All @@ -92,13 +76,6 @@ export class BrowserRunner {
this.emitter = emitter;
this.url = url;

this._navigationAttemptCount = 3;
this._navigationAttemptTimeout = Math.min(this.timeout / 2, 10000);
this._navigationSucceeded = new Promise((resolve) => {
this._resolveNavigationSucceeded = resolve;
});


this.stats = {status: 'initializing'};

this.donePromise = new Promise<void>((resolve, reject) => {
Expand Down Expand Up @@ -196,53 +173,23 @@ export class BrowserRunner {
} else {
this.sessionId = sessionId;
this.startTest();
this.extendTimeout();
}
}

async startTest() {
startTest() {
const paramDelim = (this.url.indexOf('?') === -1 ? '?' : '&');
const extra = `${paramDelim}cli_browser_id=${this.def.id}`;
await this._attemptNavigation(
this.url + extra,
this._navigationAttemptCount,
this._navigationAttemptTimeout);
this.extendTimeout();
}

/**
* Asks the connected browser to navigate to the given URL, retrying until
* `this.onEvent` receives any event from the browser.
*
* @param url The URL to navigate to.
* @param attemptCount The number of attempts to make.
* @param timeoutDuration The time to wait before considering an attempt as
* having failed, in milliseconds.
*/
private async _attemptNavigation(
url: string, attemptCount: number, timeoutDuration: number) {
for (let remaining = attemptCount; remaining > 0; remaining--) {
this.browser.get(url, (error) => {
if (error) {
this.done(error.data || error);
} else {
this.extendTimeout();
}
});

const timeoutToken = Symbol('timeoutToken');
const raceResult = await Promise.race(
[this._navigationSucceeded, timeout(timeoutDuration, timeoutToken)]);
if (raceResult !== timeoutToken) {
return;
this.browser.get(this.url + extra, (error) => {
if (error) {
this.done(error.data || error);
} else {
this.extendTimeout();
}
}

this.done(new Error(`The browser failed to navigate to "${url}".`));
});
}

onEvent(event: string, data: any) {
this._resolveNavigationSucceeded();

this.extendTimeout();
if (event === 'browser-start') {
// Always assign, to handle re-runs (no browser-init).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
{
"stats": {
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"ES6 works": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
{
"stats": {
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"inline passing test": {"state": "passing"}
}
}
}
}

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
{
"stats": {
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"inline passing test": {"state": "passing"}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,25 +1,21 @@
{
"variants": {
"": {
"stats": {
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {"state": "passing"}
}
}
},
"foo": {
"stats": {
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete"
},
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {"state": "failing"}
Expand All @@ -35,12 +31,10 @@
}
},
"bar": {
"stats": {
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete"
},
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {"state": "failing"}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
{
"stats": {
"passing": 2,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 2,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/tests.html": {
"suite": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
{
"stats": {
"passing": 3,
"pending": 0,
"failing": 3,
"status": "complete"
},
"passing": 3,
"pending": 0,
"failing": 3,
"status": "complete",
"tests": {
"test/": {
"failing test": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
{
"stats": {
"passing": 0,
"pending": 0,
"failing": 0,
"status": "error"
}
}
"passing": 0,
"pending": 0,
"failing": 0,
"status": "error"
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
{
"stats": {
"passing": 10,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 10,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"inline suite": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
{
"variants": {
"": {
"stats": {
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete"
},
"passing": 1,
"pending": 0,
"failing": 0,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {
Expand All @@ -16,12 +14,10 @@
}
},
"foo": {
"stats": {
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete"
},
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {
Expand All @@ -39,12 +35,10 @@
}
},
"bar": {
"stats": {
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete"
},
"passing": 0,
"pending": 0,
"failing": 1,
"status": "complete",
"tests": {
"test/": {
"only works with mainline components": {
Expand Down
Loading