Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): Support setting SELENIUM_PROMISE_MANAGER flag via the… #4023

Merged
merged 1 commit into from Jan 27, 2017

Conversation

sjelin
Copy link
Contributor

@sjelin sjelin commented Jan 27, 2017

… config

Closes #3691

@sjelin
Copy link
Contributor Author

sjelin commented Jan 27, 2017

CircleCI: ✅

@sjelin
Copy link
Contributor Author

sjelin commented Jan 27, 2017

Travis: ✅

*
* At the moment, by default, the WebDriver control flow is still enabled. You can disable it by
* setting the environment variable `SELENIUM_PROMISE_MANAGER` to `0`. In about one year, the
* control flow will be disabled by default, and you will be able to re-enable it by setting
Copy link
Member

Choose a reason for hiding this comment

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

Instead of 'in about one year', maybe "in a webdriver release in Q4 2018"

* At the moment, by default, the WebDriver control flow is still enabled. You can disable it by
* setting the environment variable `SELENIUM_PROMISE_MANAGER` to `0`. In about one year, the
* control flow will be disabled by default, and you will be able to re-enable it by setting
* `SELENIUM_PROMISE_MANAGER` to `1`. In about two years, the control flow will be removed for
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "in about two year", "at a later point"

*
* If you don't like managing environment variables, you can set this option in your config file,
* and Protractor will handle enabling/disabling the control flow for you. Setting this option
* will override the `SELENIUM_PROMISE_MANAGER` environment variable.
Copy link
Member

Choose a reason for hiding this comment

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

Instead say "Setting this option is higher priority than the SELENIUM_PROMISE_MANAGER environment variable".

const env = require('../environment.js');


// The main suite of Protractor tests.
Copy link
Member

Choose a reason for hiding this comment

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

Please fix this comment.


it('should allow handling errors', async function() {
await browser.get('index.html#/form');
await $('.nopenopenope').getText().then(async function(/* string */) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for this test to instead use try/catch (that works with async, yeah?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMG YOU'RE RIGHT

@juliemr
Copy link
Member

juliemr commented Jan 27, 2017

Whoo! A few comments, mostly on wording.

@sjelin
Copy link
Contributor Author

sjelin commented Jan 27, 2017

@juliemr all comments addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Support ControlFlow-less Selenium
Stage 2: User facing (PRs)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants