Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

chore(driverProviders): clean up driver provider q usage - #5034

Merged
cnishina merged 3 commits into
angular:selenium4from
cnishina:driverProviderUseExisting
Nov 15, 2018
Merged

chore(driverProviders): clean up driver provider q usage#5034
cnishina merged 3 commits into
angular:selenium4from
cnishina:driverProviderUseExisting

Conversation

@cnishina

@cnishina cnishina commented Nov 12, 2018

Copy link
Copy Markdown
Contributor
  • Remove driverProviderUseExistingWebDriver since the generation of the selenium server is already accomplished when providing a selenium address in driverProvider.ts. Also clean up docs and tests.
  • Use native promises over q promises.

@cnishina
cnishina force-pushed the driverProviderUseExisting branch from 12195c1 to 8b1f1db Compare November 13, 2018 00:05
@cnishina
cnishina requested review from heathkit and juliemr November 13, 2018 00:11
heathkit
heathkit suggested changes Nov 13, 2018
View reviewed changes
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/hosted.ts Outdated
Comment thread lib/driverProviders/mock.ts Outdated
@cnishina
cnishina force-pushed the driverProviderUseExisting branch 10 times, most recently from caea081 to 7ff7d2d Compare November 13, 2018 22:44
@cnishina
chore(driverProviders): clean up driver provider q usage
8796ad8 
- Remove driverProviderUseExistingWebDriver since the generation of the selenium server is already accomplished when providing a selenium address in driverProvider.ts. Also clean up docs and tests.
- Use native promises over q promises.
- Comment out driverProviderLocal tests. If the flag is set, the
controlFlow method is undefined and fails this test. The error is as
follows: this.driver.controlFlow is not a function at ProtractorBrowser.angularAppRoot
@cnishina
cnishina force-pushed the driverProviderUseExisting branch 5 times, most recently from 39e02d1 to e13478b Compare November 14, 2018 01:25
@cnishina
fix - remove control flow from waitForAngularEnabled, waitForAngular,…
052b591 
… and angularAppRoot

- Enabled the driverProviderLocal tests
- Remove auto unwrap test for a WebElement. Reference PR angular#3471
@cnishina
cnishina force-pushed the driverProviderUseExisting branch from e13478b to 052b591 Compare November 14, 2018 01:35
@cnishina

cnishina commented Nov 14, 2018

Copy link
Copy Markdown
Contributor Author

Errrr....Fixed it!

first try

heathkit
heathkit reviewed Nov 14, 2018
View reviewed changes
Comment thread lib/browser.ts Outdated
Comment thread lib/browser.ts Outdated
Comment thread lib/browser.ts Outdated
Comment thread lib/browser.ts
return this.bpClient.setWaitEnabled(!this.internalIgnoreSynchronization);
}
})
.then(() => {

@heathkit heathkit Nov 14, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This promise chain should eventually become async/await, but let's save that for another Pr.

@cnishina cnishina Nov 14, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this promise chain might be better contained in a separate PR.

Comment thread lib/driverProviders/attachSession.ts Outdated
Comment thread lib/driverProviders/direct.ts Outdated
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/driverProvider.ts Outdated
Comment thread lib/driverProviders/local.ts Outdated
@cnishina
cnishina force-pushed the driverProviderUseExisting branch from 7170367 to e36ebc2 Compare November 14, 2018 22:57
@cnishina

cnishina commented Nov 14, 2018

Copy link
Copy Markdown
Contributor Author

Fixes are in!

done

@cnishina
cnishina force-pushed the driverProviderUseExisting branch from e36ebc2 to e52b7d9 Compare November 14, 2018 23:57
heathkit
heathkit suggested changes Nov 15, 2018
View reviewed changes
Comment thread lib/browser.ts
Comment thread lib/browser.ts Outdated
Comment thread lib/browser.ts
if (timeout) {
let errMsg = `Timed out waiting for asynchronous Angular tasks to finish after ` +
`${timeout}. This may be because the current page is not an Angular ` +
`application. Please see the FAQ for more details: ` +

@heathkit heathkit Nov 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do ourselves a favor and extract as much as possible from this if statement into a private function.

@cnishina cnishina Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea but I think this PR is getting a little out of hand and not keeping on point to what the original description was. I'll write an issue on this for an improvement.

Comment thread lib/browser.ts
}, this.allScriptsTimeout, 'Plugins.waitForCondition()');
} catch (err) {
let timeout: RegExpExecArray;
if (/asynchronous script timeout/.test(err.message)) {

@heathkit heathkit Nov 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well extract this into a private function to make it more readable.

@cnishina cnishina Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this.

@cnishina
cnishina force-pushed the driverProviderUseExisting branch from e52b7d9 to d729bb7 Compare November 15, 2018 17:45
@cnishina
address comments
e0bf7c4 
- In lieu of Promise.resolve(...) for async await, just return the item
- Clean up JSDocs for q.promise
- Clean up interactive tests to close the browser window and let
Protractor quit the driver session
@cnishina
cnishina force-pushed the driverProviderUseExisting branch from d729bb7 to e0bf7c4 Compare November 15, 2018 17:55
@cnishina cnishina mentioned this pull request Nov 15, 2018
Closed
@cnishina

cnishina commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Reference #4995

@cnishina
cnishina merged commit 149a2ac into angular:selenium4 Nov 15, 2018
cnishina added a commit to cnishina/protractor that referenced this pull request Dec 19, 2018
@cnishina
chore(promises): clean up driver providers and browser control flow (a…
915dd21 
…ngular#5034)

Driver providers and tests:

- Use native promises over q promises in driver providers
- Remove driverProviderUseExistingWebDriver since the generation of the selenium server is already accomplished when providing a selenium address in driverProvider.ts. Also clean up docs and tests.
- Enabled the driverProviderLocal tests
- Clean up JSDocs for q.promise

Basic lib spec:

- Remove auto unwrap test for a WebElement. Reference PR angular#3471

Browser:

- Remove control flow from waitForAngularEnabled, waitForAngular, and angularAppRoot in the Browser class.
cnishina added a commit to cnishina/protractor that referenced this pull request Mar 23, 2019
@cnishina
chore(promises): clean up driver providers and browser control flow (a…
e22065c 
…ngular#5034)

Driver providers and tests:

- Use native promises over q promises in driver providers
- Remove driverProviderUseExistingWebDriver since the generation of the selenium server is already accomplished when providing a selenium address in driverProvider.ts. Also clean up docs and tests.
- Enabled the driverProviderLocal tests
- Clean up JSDocs for q.promise

Basic lib spec:

- Remove auto unwrap test for a WebElement. Reference PR angular#3471

Browser:

- Remove control flow from waitForAngularEnabled, waitForAngular, and angularAppRoot in the Browser class.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@juliemr juliemr Awaiting requested review from juliemr
1 more reviewer
@heathkit heathkit heathkit approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cnishina @heathkit @googlebot