feat(blockingproxy): Add synchronization with BlockingProxy. #3813

heathkit · 2016-12-10T00:07:12Z

This adds support for BlockingProxy behind the flag --useBlockingProxy.

If set, the driver providers will start a proxy during their setup phase, passing the selenium address to the proxy and starting a webdriver client that talks to the proxy.

Starting a proxy for each driver provider isn't strictly necessary. However, when we run with multiple capabilities it's easier to handle the logging if each Protractor instance has it's own proxy.

Known issues:

Doesn't work with directConnect. You can get the address of chromedriver by mucking around in Selenium internals, but this probably changed for Selenium 3.0 and I doubt it's worth figuring out until we upgrade.
Doesn't yet work with webDriverProxy (but it's an easy fix)
Doesn't work with wrapDriver. The new browser instance will need to know somehow that the flag is set, still working on this.

juliemr · 2016-12-13T05:47:44Z

circle.yml

@@ -1,4 +1,6 @@
 machine:
+  node:
+    version: 6.9.1


Can we remove the npm install -g npm@3 now?

juliemr · 2016-12-13T05:48:47Z

lib/bpRunner.ts

+import {Config} from './config';
+import {Logger} from './logger';
+
+const BP_PATH = 'node_modules/blocking-proxy/built/lib/bin.js';


Can we use require.resolve here to avoid mucking around with node_modules?

juliemr · 2016-12-13T05:59:39Z

lib/bpRunner.ts

+      throw new Error('BlockingProxy not yet supported with directConnect!');
+    }
+  }
+}


Newline please!

juliemr · 2016-12-13T06:02:23Z

lib/browser.ts

@@ -188,7 +195,21 @@ export class ProtractorBrowser extends Webdriver {
   *
   * @type {boolean}
   */
-  ignoreSynchronization: boolean;
+  set ignoreSynchronization(value) {


I think we should take this opportunity to improve the naming here. I think we had discussed waitForAngularEnabled(true/false)

I'd also be OK with keeping these changes that keep ignoreSynchronization working but adding the new API as the 'recommended way' as well.

Good point. I'll add the new API, but I'd like to leave ignoreSynchronization in place and announce that it's deprecated.

juliemr · 2016-12-13T06:20:09Z

lib/browser.ts

@@ -668,6 +693,12 @@ export class ProtractorBrowser extends Webdriver {
      return 'Protractor.get(' + destination + ') - ' + str;
    };

+    if (this.bpClient) {


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?

Exactly. We could skip waiting for executeScript calls in general, but I wanted to keep behavior the same.

juliemr · 2016-12-13T06:29:30Z

package.json

@@ -72,7 +73,7 @@
  },
  "license": "MIT",
  "engines": {
-    "node": ">=4.2.x"
+    "node": ">=6.9.x"


Let's avoid this change until 5.x

Out of curiosity, what in this PR depends on it?

Rebased to the beta branch.

juliemr · 2016-12-13T06:30:12Z

spec/blockingProxyConf.js

@@ -0,0 +1,34 @@
+var env = require('./environment.js');


Add this to the test.sh script for npm test?

juliemr · 2016-12-13T06:30:33Z

spec/blockingProxyConf.js

@@ -0,0 +1,34 @@
+var env = require('./environment.js');
+
+// The main suite of Protractor tests.


Update comment.

juliemr · 2016-12-13T06:32:23Z

spec/blockingProxyConf.js

+
+  blockingProxyUrl: 'http://localhost:8111',
+
+  // Exclude patterns are relative to this directory.


Unnecessary.

juliemr · 2016-12-13T06:32:50Z

spec/blockingProxyConf.js

+    'hybrid/async_spec.js'
+  ],
+
+  blockingProxyUrl: 'http://localhost:8111',


I think this should be useBlockingProxy: true now

googlebot · 2016-12-13T18:53:49Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-12-13T18:54:36Z

CLAs look good, thanks!

googlebot · 2016-12-15T01:27:20Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-12-15T01:27:20Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

juliemr · 2016-12-15T03:59:56Z

lib/driverProviders/driverProvider.ts

+    } else {
+      builder = new webdriver.Builder()
+                    .usingServer(this.config_.seleniumAddress)
+                    .usingWebDriverProxy(this.config_.webDriverProxy)


what's blocking us from allowing webdriver proxies (out of curiosity).

Nothing really, it just needs to be implemented on the BlockingProxy side. I'm having trouble coming up with a good test for it.

juliemr · 2016-12-15T04:10:42Z

Failing test is the one we decided to kill - so after this is gone and passing, LGTM.

juliemr · 2016-12-15T04:11:04Z

Dunno why CLA-bot is so unhappy today.

googlebot · 2016-12-16T18:15:14Z

CLAs look good, thanks!

googlebot · 2016-12-16T18:15:14Z

CLAs look good, thanks!

…#3813) This adds support for BlockingProxy behind the flag --useBlockingProxy. If set, the driver providers will start a proxy during their setup phase, passing the selenium address to the proxy and starting a webdriver client that talks to the proxy. Starting a proxy for each driver provider isn't strictly necessary. However, when we run with multiple capabilities it's easier to handle the logging if each Protractor instance has it's own proxy. Known issues: - Doesn't work with directConnect. You can get the address of chromedriver by mucking around in Selenium internals, but this probably changed for Selenium 3.0 and I doubt it's worth figuring out until we upgrade. - Doesn't yet work with webDriverProxy (but it's an easy fix)

This adds support for BlockingProxy behind the flag --useBlockingProxy. If set, the driver providers will start a proxy during their setup phase, passing the selenium address to the proxy and starting a webdriver client that talks to the proxy. Starting a proxy for each driver provider isn't strictly necessary. However, when we run with multiple capabilities it's easier to handle the logging if each Protractor instance has it's own proxy. Known issues: - Doesn't work with directConnect. You can get the address of chromedriver by mucking around in Selenium internals, but this probably changed for Selenium 3.0 and I doubt it's worth figuring out until we upgrade. - Doesn't yet work with webDriverProxy (but it's an easy fix)

heathkit added the pr: needs cleanup label Dec 10, 2016

googlebot added the cla: yes label Dec 10, 2016

juliemr reviewed Dec 13, 2016

View reviewed changes

heathkit changed the base branch from master to beta December 13, 2016 18:50

heathkit force-pushed the blockingproxy branch from 4ef23fa to cb1fb3e Compare December 13, 2016 18:53

googlebot added cla: no and removed cla: yes labels Dec 13, 2016

heathkit force-pushed the blockingproxy branch from cb1fb3e to 88f2a31 Compare December 13, 2016 18:54

googlebot added cla: yes and removed cla: no labels Dec 13, 2016

heathkit force-pushed the blockingproxy branch 5 times, most recently from 49c2655 to 7f109ea Compare December 15, 2016 01:27

googlebot added cla: no and removed cla: yes labels Dec 15, 2016

heathkit force-pushed the beta branch from 9ea6dd1 to 329c32e Compare December 15, 2016 01:40

juliemr approved these changes Dec 15, 2016

View reviewed changes

juliemr added cla: yes and removed cla: no labels Dec 15, 2016

heathkit added pr: LGTM and removed pr: needs cleanup labels Dec 15, 2016

heathkit added 8 commits December 16, 2016 10:11

Rough first stab at hooking in the proxy.

54b8e4d

Disable sync for bootstrapping.

bace069

Cleaning up a little.

3bc2964

Fix name for blocking-proxy package, reduce logging.

83a4b62

Remove old hook.

279eaf9

Requires node v6.

58740b7

Fix import name, run clang format.

62c2f07

Update with feedback.

5cb068b

heathkit force-pushed the blockingproxy branch from 7f109ea to 5cb068b Compare December 16, 2016 18:15

heathkit merged commit 43502e1 into angular:beta Dec 16, 2016

heathkit mentioned this pull request Dec 20, 2016

Make it easier to control when ignoreSynchronization is off or on #1939

Closed

heathkit deleted the blockingproxy branch January 4, 2017 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blockingproxy): Add synchronization with BlockingProxy. #3813

feat(blockingproxy): Add synchronization with BlockingProxy. #3813

heathkit commented Dec 10, 2016

juliemr Dec 13, 2016

juliemr Dec 13, 2016

heathkit Dec 14, 2016

juliemr Dec 13, 2016

heathkit Dec 14, 2016

juliemr Dec 13, 2016

juliemr Dec 13, 2016

heathkit Dec 13, 2016

juliemr Dec 13, 2016

heathkit Dec 13, 2016

juliemr Dec 13, 2016

heathkit Dec 14, 2016

juliemr Dec 13, 2016

juliemr Dec 13, 2016

juliemr Dec 13, 2016

juliemr Dec 13, 2016

googlebot commented Dec 13, 2016

googlebot commented Dec 13, 2016

googlebot commented Dec 15, 2016

googlebot commented Dec 15, 2016

juliemr Dec 15, 2016

heathkit Dec 15, 2016

juliemr commented Dec 15, 2016

juliemr commented Dec 15, 2016

googlebot commented Dec 16, 2016

googlebot commented Dec 16, 2016

		@@ -0,0 +1,34 @@
		var env = require('./environment.js');

		// The main suite of Protractor tests.


		blockingProxyUrl: 'http://localhost:8111',

		// Exclude patterns are relative to this directory.

feat(blockingproxy): Add synchronization with BlockingProxy. #3813

feat(blockingproxy): Add synchronization with BlockingProxy. #3813

Conversation

heathkit commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Dec 13, 2016

googlebot commented Dec 13, 2016

googlebot commented Dec 15, 2016

googlebot commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliemr commented Dec 15, 2016

juliemr commented Dec 15, 2016

googlebot commented Dec 16, 2016

googlebot commented Dec 16, 2016