fix(types): update for selenium-webdriver types creating transpile errors #3848

cnishina · 2016-12-20T00:29:42Z

No description provided.

cnishina · 2016-12-20T08:44:51Z

There is a test that leaves a browser session open. Will investigate.

cnishina · 2016-12-20T09:18:30Z

Narrowed down to this test. It passes but the browser is still open: spec/driverProviderLocalConf.js.

We can fix this later.

cnishina · 2016-12-20T10:38:25Z

Also we are setting a specific version of @types/selenium-webdriver to make sure we don't run into this problem again.

When angular/protractor#3848 is ready it should fix the webdriver types issues, and the pinning can be reverted. The zone.js problem is being tracked on angular/zone.js#554. Fix #325

juliemr · 2016-12-20T18:52:53Z

lib/driverProviders/mock.ts

@@ -4,10 +4,15 @@
 * server.
 */
 import * as q from 'q';
+import {DeferredExecutor, Executor, Session, WebDriver} from 'selenium-webdriver';


DeferredExecutor, Executor, and Session look unused?

Nice catch. I had a few issues with mock last night... I'll remove this.

Removing DeferredExecutor and Executor. The Session constructor is used at getNewDriver.

…rors

juliemr · 2016-12-20T18:59:50Z

LGTM!

sjelin

Lots of nits and one important issue in the package.json

sjelin · 2016-12-20T21:26:41Z

package.json

@@ -15,7 +15,7 @@
    "@types/jasmine": "^2.5.36",
    "@types/node": "^6.0.46",
    "@types/q": "^0.0.32",
-    "@types/selenium-webdriver": "~2.53.31",
+    "@types/selenium-webdriver": "2.53.31",


Should probably be "2.53.37". Additionally, we should maybe fix all the other @types in a similar way and put something in release.md about updating types.

sjelin · 2016-12-20T21:32:38Z

lib/browser.ts

@@ -562,7 +562,7 @@ export class ProtractorBrowser extends Webdriver {
   * @returns {!webdriver.promise.Promise} A promise that will be resolved to an


nit: way up on line 13, we have const webdriver = ..., but we only actually use webdriver in the for loop a couple lines later. Let's scope webdriver to that for loop so we don't have two copies of the same library floating around.

Scoping the require('selenium-webdriver') in only the for loop

sjelin · 2016-12-20T21:34:23Z

lib/debugger.ts

@@ -60,17 +61,15 @@ export class DebugHelper {
    }


nit: on line 55, prefer wdpromise

Also, on line 42, I think I'd prefer (wdpromise.ControlFlow as any).prototype and get rid of the webdriver variable entirely

done and removed require('selenium-webdriver');

sjelin · 2016-12-20T21:34:54Z

lib/debugger.ts

@@ -230,9 +229,9 @@ export class DebugHelper {
   *     is done. The promise will resolve to a boolean which represents whether
   *     this is the first time that the debugger is called.
   */
-  private validatePortAvailability_(port: number): webdriver.promise.Promise<any> {


nit: prefer wdpromise

sjelin · 2016-12-20T21:35:00Z

lib/debugger.ts

    if (this.debuggerValidated_) {
-      return webdriver.promise.fulfilled(false);


nit: prefer wdpromise

sjelin · 2016-12-20T22:04:22Z

lib/locators.ts

@@ -1,6 +1,6 @@
 import {By, promise as wdpromise, WebDriver, WebElement} from 'selenium-webdriver';

-let webdriver = require('selenium-webdriver');
+const webdriver = require('selenium-webdriver');


nit: if you can address my other nit in this file, remove this declaration.

sjelin · 2016-12-20T22:06:47Z

lib/ptor.ts

@@ -23,7 +23,7 @@ export class Ptor {
  By: ProtractorBy;
  by: ProtractorBy;
  wrapDriver:
-      (webdriver: webdriver.WebDriver, baseUrl?: string, rootElement?: string,
+      (webdriver: WebDriver, baseUrl?: string, rootElement?: string,
       untrackOutstandingTimeouts?: boolean) => ProtractorBrowser;
  ExpectedConditions: ProtractorExpectedConditions;



nit: below, for everything under // Export selenium webdriver., prefer the imported version vs the webdriver. version

sjelin · 2016-12-20T22:08:04Z

lib/ptor.ts

@@ -23,7 +23,7 @@ export class Ptor {
  By: ProtractorBy;


nit: up on line 11, prefer const webdriver = .... Or possibly just remove the declaration entirely if you can address my other nit in this file

sjelin · 2016-12-20T22:10:10Z

lib/runner.ts

@@ -1,5 +1,6 @@
 import {EventEmitter} from 'events';
 import * as q from 'q';
+import {Session} from 'selenium-webdriver';


nit: add promise as wdpromise to this and replace all the instances of webdriver.promise below with wdpromise

sjelin · 2016-12-20T22:11:11Z

lib/runner.ts

@@ -1,5 +1,6 @@
 import {EventEmitter} from 'events';
 import * as q from 'q';
+import {Session} from 'selenium-webdriver';
 import * as util from 'util';

 import {ProtractorBrowser} from './browser';


nit: below on line 17, prefer const webdriver = .... Or, if you can address my other nit from this file, remove the declaration entirely.

…the webdriver typings. Protractor 4.0.14 incorporates DefinitelyTyped/DefinitelyTyped#13382, fixing angular/protractor#3848

When angular/protractor#3848 is ready it should fix the webdriver types issues, and the pinning can be reverted. The zone.js problem is being tracked on angular/zone.js#554. Fix angular/quickstart#325

googlebot added the cla: yes label Dec 20, 2016

cnishina requested a review from sjelin December 20, 2016 00:29

cnishina added the pr: needs review label Dec 20, 2016

cnishina force-pushed the fix_selenium_types branch from 2142d83 to 3d5f84a Compare December 20, 2016 00:30

cnishina removed the request for review from sjelin December 20, 2016 00:31

cnishina force-pushed the fix_selenium_types branch from 3d5f84a to 87ebf34 Compare December 20, 2016 07:05

cnishina requested a review from juliemr December 20, 2016 09:22

cnishina mentioned this pull request Dec 20, 2016

Make selenium-webdriver declaration a module DefinitelyTyped/DefinitelyTyped#13382

Merged

This was referenced Dec 20, 2016

'npm start' error with protractor angular/quickstart#325

Closed

fix(ci): pin webdriver and zone versions angular/angular.io#3010

Merged

ghost mentioned this pull request Dec 20, 2016

Issue with latest selenium-webdriver definition DefinitelyTyped/DefinitelyTyped#13461

Closed

filipesilva mentioned this pull request Dec 20, 2016

fix(deps): pin webdriver and zone.js angular/quickstart#330

Merged

cnishina mentioned this pull request Dec 20, 2016

Investigate spec/driverProviderLocalConf.js browser window not closed #3853

Open

juliemr reviewed Dec 20, 2016

View reviewed changes

fix(types): update for selenium-webdriver types creating transpile er…

6395c1c

…rors

cnishina force-pushed the fix_selenium_types branch from 87ebf34 to 6395c1c Compare December 20, 2016 18:57

juliemr approved these changes Dec 20, 2016

View reviewed changes

juliemr added pr: LGTM and removed pr: needs review labels Dec 20, 2016

cnishina merged commit 83694f5 into angular:master Dec 20, 2016

cnishina mentioned this pull request Dec 20, 2016

chore(types): fixed types to use selenium-webdriver #3851

Closed

sjelin reviewed Dec 20, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types): update for selenium-webdriver types creating transpile errors #3848

fix(types): update for selenium-webdriver types creating transpile errors #3848

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016 •

edited

Loading

juliemr Dec 20, 2016

cnishina Dec 20, 2016

cnishina Dec 20, 2016

juliemr commented Dec 20, 2016

sjelin left a comment

sjelin Dec 20, 2016

cnishina Dec 20, 2016

sjelin Dec 20, 2016

cnishina Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

cnishina Dec 20, 2016 •

edited

Loading

sjelin Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

sjelin Dec 20, 2016

		@@ -562,7 +562,7 @@ export class ProtractorBrowser extends Webdriver {
		* @returns {!webdriver.promise.Promise} A promise that will be resolved to an

		if (this.debuggerValidated_) {
		return webdriver.promise.fulfilled(false);

fix(types): update for selenium-webdriver types creating transpile errors #3848

fix(types): update for selenium-webdriver types creating transpile errors #3848

Conversation

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016

cnishina commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliemr commented Dec 20, 2016

sjelin left a 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

cnishina Dec 20, 2016 • edited Loading

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

cnishina commented Dec 20, 2016 •

edited

Loading

cnishina Dec 20, 2016 •

edited

Loading