-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(blockingproxy): Add synchronization with BlockingProxy. #3813
Changes from all commits
54b8e4d
bace069
3bc2964
83a4b62
279eaf9
58740b7
62c2f07
5cb068b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
import {ChildProcess, fork} from 'child_process'; | ||
import * as q from 'q'; | ||
|
||
import {Config} from './config'; | ||
import {Logger} from './logger'; | ||
|
||
const BP_PATH = require.resolve('blocking-proxy/built/lib/bin.js'); | ||
|
||
let logger = new Logger('BlockingProxy'); | ||
|
||
export class BlockingProxyRunner { | ||
bpProcess: ChildProcess; | ||
public port: number; | ||
|
||
constructor(private config: Config) {} | ||
|
||
start() { | ||
return q.Promise((resolve, reject) => { | ||
this.checkSupportedConfig(); | ||
|
||
let args = [ | ||
'--fork', '--seleniumAddress', this.config.seleniumAddress, '--rootElement', | ||
this.config.rootElement | ||
]; | ||
this.bpProcess = fork(BP_PATH, args, {silent: true}); | ||
logger.info('Starting BlockingProxy with args: ' + args.toString()); | ||
this.bpProcess | ||
.on('message', | ||
(data) => { | ||
this.port = data['port']; | ||
resolve(data['port']); | ||
}) | ||
.on('error', | ||
(err) => { | ||
reject(new Error('Unable to start BlockingProxy ' + err)); | ||
}) | ||
.on('exit', (code: number, signal: number) => { | ||
reject(new Error('BP exited with ' + code)); | ||
logger.error('Exited with ' + code); | ||
logger.error('signal ' + signal); | ||
}); | ||
|
||
this.bpProcess.stdout.on('data', (msg: Buffer) => { | ||
logger.debug(msg.toString().trim()); | ||
}); | ||
|
||
this.bpProcess.stderr.on('data', (msg: Buffer) => { | ||
logger.error(msg.toString().trim()); | ||
}); | ||
|
||
process.on('exit', () => { | ||
this.bpProcess.kill(); | ||
}) | ||
}) | ||
} | ||
|
||
checkSupportedConfig() { | ||
if (this.config.directConnect) { | ||
throw new Error('BlockingProxy not yet supported with directConnect!'); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import {BPClient} from 'blocking-proxy'; | ||
import {ActionSequence, By, Capabilities, Command as WdCommand, FileDetector, ICommandName, Options, promise as wdpromise, Session, TargetLocator, TouchSequence, until, WebDriver, WebElement} from 'selenium-webdriver'; | ||
import * as url from 'url'; | ||
|
||
|
@@ -141,6 +142,12 @@ export class ProtractorBrowser extends Webdriver { | |
*/ | ||
driver: WebDriver; | ||
|
||
/** | ||
* The client used to control the BlockingProxy. If unset, BlockingProxy is | ||
* not being used and Protractor will handle client-side synchronization. | ||
*/ | ||
bpClient: BPClient; | ||
|
||
/** | ||
* Helper function for finding elements. | ||
* | ||
|
@@ -186,9 +193,26 @@ export class ProtractorBrowser extends Webdriver { | |
* tests to become flaky. This should be used only when necessary, such as | ||
* when a page continuously polls an API using $timeout. | ||
* | ||
* This property is deprecated - please use waitForAngularEnabled instead. | ||
* | ||
* @deprecated | ||
* @type {boolean} | ||
*/ | ||
ignoreSynchronization: boolean; | ||
set ignoreSynchronization(value) { | ||
this.driver.controlFlow().execute(() => { | ||
if (this.bpClient) { | ||
logger.debug('Setting waitForAngular' + value); | ||
this.bpClient.setSynchronization(!value); | ||
} | ||
}, `Set proxy synchronization to ${value}`); | ||
this.internalIgnoreSynchronization = value; | ||
} | ||
|
||
get ignoreSynchronization() { | ||
return this.internalIgnoreSynchronization; | ||
} | ||
|
||
internalIgnoreSynchronization: boolean; | ||
|
||
/** | ||
* Timeout in milliseconds to wait for pages to load when calling `get`. | ||
|
@@ -272,7 +296,7 @@ export class ProtractorBrowser extends Webdriver { | |
|
||
constructor( | ||
webdriverInstance: WebDriver, opt_baseUrl?: string, opt_rootElement?: string, | ||
opt_untrackOutstandingTimeouts?: boolean) { | ||
opt_untrackOutstandingTimeouts?: boolean, opt_blockingProxyUrl?: string) { | ||
super(); | ||
// These functions should delegate to the webdriver instance, but should | ||
// wait for Angular to sync up before performing the action. This does not | ||
|
@@ -291,6 +315,10 @@ export class ProtractorBrowser extends Webdriver { | |
}); | ||
|
||
this.driver = webdriverInstance; | ||
if (opt_blockingProxyUrl) { | ||
logger.info('Starting BP client for ' + opt_blockingProxyUrl); | ||
this.bpClient = new BPClient(opt_blockingProxyUrl); | ||
} | ||
this.element = buildElementHelper(this); | ||
this.$ = build$(this.element, By); | ||
this.$$ = build$$(this.element, By); | ||
|
@@ -325,6 +353,22 @@ export class ProtractorBrowser extends Webdriver { | |
this.ExpectedConditions = new ProtractorExpectedConditions(this); | ||
} | ||
|
||
/** | ||
* If set to false, Protractor will not wait for Angular $http and $timeout | ||
* tasks to complete before interacting with the browser. This can cause | ||
* flaky tests, but should be used if, for instance, your app continuously | ||
* polls an API with $timeout. | ||
* | ||
* Call waitForAngularEnabled() without passing a value to read the current | ||
* state without changing it. | ||
*/ | ||
waitForAngularEnabled(enabled: boolean = null): boolean { | ||
if (enabled != null) { | ||
this.ignoreSynchronization = !enabled; | ||
} | ||
return !this.ignoreSynchronization; | ||
} | ||
|
||
/** | ||
* Get the processed configuration object that is currently being run. This | ||
* will contain the specs and capabilities properties of the current runner | ||
|
@@ -445,7 +489,7 @@ export class ProtractorBrowser extends Webdriver { | |
} | ||
|
||
let runWaitForAngularScript: () => wdpromise.Promise<any> = () => { | ||
if (this.plugins_.skipAngularStability()) { | ||
if (this.plugins_.skipAngularStability() || this.bpClient) { | ||
return wdpromise.fulfilled(); | ||
} else if (this.rootEl) { | ||
return this.executeAsyncScript_( | ||
|
@@ -668,6 +712,12 @@ export class ProtractorBrowser extends Webdriver { | |
return 'Protractor.get(' + destination + ') - ' + str; | ||
}; | ||
|
||
if (this.bpClient) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, why is this necessary? Because we do executeScript calls, and BpClient isn't smart enough to know that it doesn't need to wait for those? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. We could skip waiting for executeScript calls in general, but I wanted to keep behavior the same. |
||
this.driver.controlFlow().execute(() => { | ||
return this.bpClient.setSynchronization(false); | ||
}); | ||
} | ||
|
||
if (this.ignoreSynchronization) { | ||
this.driver.get(destination); | ||
return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {}); | ||
|
@@ -768,6 +818,12 @@ export class ProtractorBrowser extends Webdriver { | |
} | ||
} | ||
|
||
if (this.bpClient) { | ||
this.driver.controlFlow().execute(() => { | ||
return this.bpClient.setSynchronization(!this.internalIgnoreSynchronization); | ||
}); | ||
} | ||
|
||
this.driver.controlFlow().execute(() => { | ||
return this.plugins_.onPageStable().then(() => { | ||
deferred.fulfill(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,17 +5,20 @@ | |
*/ | ||
import * as q from 'q'; | ||
|
||
import {BlockingProxyRunner} from '../bpRunner'; | ||
import {Config} from '../config'; | ||
|
||
let webdriver = require('selenium-webdriver'); | ||
|
||
export class DriverProvider { | ||
export abstract class DriverProvider { | ||
drivers_: webdriver.WebDriver[]; | ||
config_: Config; | ||
private bpRunner: BlockingProxyRunner; | ||
|
||
constructor(config: Config) { | ||
this.config_ = config; | ||
this.drivers_ = []; | ||
this.bpRunner = new BlockingProxyRunner(config); | ||
} | ||
|
||
/** | ||
|
@@ -28,17 +31,28 @@ export class DriverProvider { | |
return this.drivers_.slice(); // Create a shallow copy | ||
} | ||
|
||
getBPUrl() { | ||
return `http://localhost:${this.bpRunner.port}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use os.hostname()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think localhost is the right thing to do here, since BP is binding to localhost. |
||
} | ||
|
||
/** | ||
* Create a new driver. | ||
* | ||
* @public | ||
* @return webdriver instance | ||
*/ | ||
getNewDriver() { | ||
let builder = new webdriver.Builder() | ||
.usingServer(this.config_.seleniumAddress) | ||
.usingWebDriverProxy(this.config_.webDriverProxy) | ||
.withCapabilities(this.config_.capabilities); | ||
let builder: webdriver.Builder; | ||
if (this.config_.useBlockingProxy) { | ||
builder = new webdriver.Builder() | ||
.usingServer(this.getBPUrl()) | ||
.withCapabilities(this.config_.capabilities); | ||
} else { | ||
builder = new webdriver.Builder() | ||
.usingServer(this.config_.seleniumAddress) | ||
.usingWebDriverProxy(this.config_.webDriverProxy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's blocking us from allowing webdriver proxies (out of curiosity). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing really, it just needs to be implemented on the BlockingProxy side. I'm having trouble coming up with a good test for it. |
||
.withCapabilities(this.config_.capabilities); | ||
} | ||
if (this.config_.disableEnvironmentOverrides === true) { | ||
builder.disableEnvironmentOverrides(); | ||
} | ||
|
@@ -89,13 +103,23 @@ export class DriverProvider { | |
}; | ||
|
||
/** | ||
* Default setup environment method. | ||
* @return a promise | ||
* Default setup environment method, common to all driver providers. | ||
*/ | ||
setupEnv(): q.Promise<any> { | ||
return q.fcall(function() {}); | ||
let driverPromise = this.setupDriverEnv(); | ||
if (this.config_.useBlockingProxy) { | ||
// TODO(heathkit): If set, pass the webDriverProxy to BP. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should just grab this github username :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup |
||
return q.all([driverPromise, this.bpRunner.start()]); | ||
} | ||
return driverPromise; | ||
}; | ||
|
||
/** | ||
* Set up environment specific to a particular driver provider. Overridden | ||
* by each driver provider. | ||
*/ | ||
protected abstract setupDriverEnv(): q.Promise<any>; | ||
|
||
/** | ||
* Teardown and destroy the environment and do any associated cleanup. | ||
* Shuts down the drivers. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should take this opportunity to improve the naming here. I think we had discussed
waitForAngularEnabled(true/false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also be OK with keeping these changes that keep
ignoreSynchronization
working but adding the new API as the 'recommended way' as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll add the new API, but I'd like to leave
ignoreSynchronization
in place and announce that it's deprecated.