ON HOLD. need to not have a PR that is 95 files... #5038

cnishina · 2018-11-13T22:04:30Z

Leave node scripts/errorTest.js commented out since it has something
to do with the control flow where the exit code is not set. Also the
breakpointhook.js file has issues we will need to address (Error: Cannot find
module).
Add TODO block for slowHttpAndTimeout tests.
Update spec/errorTest to use const, let over var and async/await.
Add SELENIUM_PROMISE_MANAGER flag to plugins tests (currently missing)
Turn on the rest of test suite prior to updating to selenium 4
(basicConf using blocking proxy, unit tests, dependency tests, and
typing tests)

spec/errorTest/shardedFailureConf.js

spec/errorTest/slowHttpAndTimeoutConf.js

spec/errorTest/baseCase/error_spec.js

- Leave node scripts/errorTest.js commented out since it has something to do with the control flow where the exit code is not set. Also the breakpointhook.js file has issues we will need to address (Error: Cannot find module). - Add TODO block for slowHttpAndTimeout tests. - Update spec/errorTest to use const, let over var and async/await. - Add SELENIUM_PROMISE_MANAGER flag to plugins tests (currently missing) - Turn on the rest of test suite prior to updating to selenium 4 (basicConf using blocking proxy, unit tests, dependency tests, and typing tests) - Remove spec/errorTest/error_spec.js. Committed in angular@a13a2d3#diff-0b3f10a17756d2a4773c4d716805edf8 but not used.

- update browser.get method because of control flow.... and then it became whack-a-mole for q promises, wdpromises...fix one, two more type errors pop up. - remove q from protractor!!! huzzah. things are probably broken. (i'll have to do another review. i might not have done the debugger file) - will slowly add tests back into this test and pass the test suite.

cnishina · 2018-11-21T09:46:06Z

Also these "approved" changes will need another lgtm.

- Removed jshint to reduce noise. We will not need this when switching suite to TypeScript. - Changed Protractor configuration files to be ".conf.js" for uniformity (I was trying to exclude ".conf.js" from jshint. After removing jshint, this decision seems pointless but cleaner?) - Cleaned up logging. Ran all the tests and most seem to pass. There are some issues where catching and unhandled promises throwing errors report slightly differently. - Fixed the driverProviderAttachSession test.

cnishina

Probably the most important files we need to code review is browser, plugins, launcher, runner, taskRunner

cnishina · 2018-11-22T00:12:34Z

lib/debugger.ts

@@ -6,13 +6,11 @@ import {ProtractorBrowser} from './browser';
 import {Locator} from './locators';
 import {Logger} from './logger';
 import {Ptor} from './ptor';
-import * as helper from './util';
 let breakpointHook = require('./breakpointhook.js');


These are unused and deleting.

cnishina · 2018-11-22T00:13:14Z

circle.yml

@@ -52,7 +52,7 @@ jobs:
          name: Selenium Start
          background: true
          command: |
-            ./node_modules/webdriver-manager/bin/webdriver-manager update
+            ./bin/webdriver-manager update


This was to just call the correct webdriver-manager in circle. We need these binaries for some tests.

cnishina · 2018-11-22T00:13:46Z

gulpfile.js

@@ -41,7 +41,7 @@ gulp.task('tslint', function() {
 });

 gulp.task('lint', function(done) {
-  runSequence('tslint', 'jshint', 'format:enforce', done);
+  runSequence('tslint', 'format:enforce', done);


Most of these here are to clean up jshint and remove a not used done statement.

cnishina · 2018-11-22T00:14:20Z

lib/driverProviders/attachSession.ts

@@ -38,7 +38,7 @@ export class AttachSession extends DriverProvider {
  getNewDriver(): WebDriver {
    const httpClient = new http.HttpClient(this.config_.seleniumAddress);
    const executor = new http.Executor(httpClient);
-    const newDriver = WebDriver.attachToSession(executor, this.config_.seleniumSessionId);
+    const newDriver = WebDriver.attachToSession(executor, this.config_.seleniumSessionId, null);


Setting null here will ensure that the control flow option is set to null.

cnishina · 2018-11-22T00:14:32Z

lib/driverProviders/driverProvider.ts

@@ -3,7 +3,7 @@
 *  It is responsible for setting up the account object, tearing
 *  it down, and setting up the driver correctly.
 */
-import {Builder, Session, WebDriver} from 'selenium-webdriver';
+import {Builder, WebDriver} from 'selenium-webdriver';


Removed unused Session.

cnishina · 2018-11-22T00:19:51Z

lib/ptor.ts

    return null;
  };
-  $$ = function(search: string): ElementArrayFinder {
+  $$ = function(_: string): ElementArrayFinder {


Minor clean up stuff. No biggie

cnishina · 2018-11-22T00:20:47Z

lib/browser.ts

-   * browser.get('page2'); // 'page2' gotten by restarted browser
-   *
-   * // Running against global browser, with control flow disabled
+   * // Running against global browser
   * await browser.get('page1');


Doc clean up. No more restartSync and no more control flow.

cnishina · 2018-11-22T00:22:50Z

lib/browser.ts

-        .then(() => {
-          if (this.bpClient) {
-            return this.driver.controlFlow().execute(() => {
-              return this.bpClient.setWaitEnabled(false);


No more control flow!

cnishina · 2018-11-22T00:24:21Z

lib/browser.ts

-    if (this.ignoreSynchronization) {
-      return this.driver.navigate().refresh();
+  async refresh(opt_timeout?: number) {
+    if (!await this.waitForAngularEnabled()) {


ignoreSynchronization is problematic. It was previously noted as deprecated and I think we should remove this property.

cnishina · 2018-11-22T00:30:06Z

scripts/test.js

+      {message: 'from teardown'}
+    ]);
+
+// TODO(selenium4): turn these on when we figure out the correct error message handling.


We need to talk about these and the following tests commented out. These are UnhandledPromiseRejectionWarnings. I'm not sure where to fix this but maybe for the next PR.

cnishina · 2018-12-15T01:48:45Z

Closing this one too!

googlebot added the cla: yes label Nov 13, 2018

cnishina requested a review from heathkit November 13, 2018 22:30

cnishina mentioned this pull request Nov 13, 2018

move test_util out of q, rework due to es6 #5036

Merged

5 tasks

heathkit approved these changes Nov 15, 2018

View reviewed changes

CrispusDH reviewed Nov 15, 2018

View reviewed changes

spec/errorTest/shardedFailureConf.js Outdated Show resolved Hide resolved

CrispusDH reviewed Nov 15, 2018

View reviewed changes

spec/errorTest/slowHttpAndTimeoutConf.js Outdated Show resolved Hide resolved

cnishina force-pushed the misc branch 2 times, most recently from 091b5c9 to fd23742 Compare November 15, 2018 20:16

juliemr approved these changes Nov 17, 2018

View reviewed changes

spec/errorTest/baseCase/error_spec.js Outdated Show resolved Hide resolved

cnishina force-pushed the misc branch from fd23742 to f587638 Compare November 19, 2018 19:25

cnishina added 2 commits November 19, 2018 11:31

cnishina force-pushed the misc branch from f587638 to 6684f0f Compare November 20, 2018 02:19

well there are things that need to be cleaned up but it kinda works?

ce4867e

cnishina force-pushed the misc branch 4 times, most recently from 977cf70 to c797148 Compare November 22, 2018 00:28

cnishina force-pushed the misc branch from c797148 to 84fe80f Compare November 22, 2018 00:31

cnishina commented Nov 22, 2018

View reviewed changes

cnishina changed the title ~~chore(test): move error tests off of the control flow.~~ ON HOLD. need to not have a PR that is 95 files... Nov 22, 2018

cnishina closed this Dec 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ON HOLD. need to not have a PR that is 95 files... #5038

ON HOLD. need to not have a PR that is 95 files... #5038

Uh oh!

cnishina commented Nov 13, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cnishina commented Nov 21, 2018

Uh oh!

cnishina left a comment

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina Nov 22, 2018

Uh oh!

cnishina commented Dec 15, 2018

Uh oh!

Uh oh!

ON HOLD. need to not have a PR that is 95 files... #5038

ON HOLD. need to not have a PR that is 95 files... #5038

Uh oh!

Conversation

cnishina commented Nov 13, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cnishina commented Nov 21, 2018

Uh oh!

cnishina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnishina commented Dec 15, 2018

Uh oh!

Uh oh!