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(browser): chain promises in browser.get #4017

Merged
merged 1 commit into from Jan 27, 2017
Merged

Conversation

sjelin
Copy link
Contributor

@sjelin sjelin commented Jan 26, 2017

Part of #3904

@@ -837,79 +848,85 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver {
return 'Protractor.get(' + destination + ') - ' + str;
};

let retPromise: wdpromise.Promise<any>;
let then = (fn: () => wdpromise.Promise<any>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clever, but naming it then is a little confusing for me, since it makes it hard to distinguish chaining from calling .then on a promise at a glance. Maybe just chainThenables, andThen, or something like that?

Also, why do we need this? Why wouldn't regular promise chaining work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regular promise chain would work, but it's tricky, since the first promise could be one of three different ones, and you want to prematurely exit if ignoreSynchronization is on. In the light of day though, if I move the ignoreSynchronization case up, and start the synchronized case with a blank execute, I should be able to make promise chaining work fine.

@heathkit
Copy link
Contributor

Looking at the log for the test failure, it's not the alert timeout issue. It looks like disabling synchronization in get() when using blocking proxy is broken.

this.driver.get(destination);
return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {});
then(() => this.driver.get(destination));
return then(() => this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the bpClient.setSynchronization call after this test. I'm not sure, but I think Circle is failing because waitForAngular gets turned off in BP but never gets restored if get() returns from ehre.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my debugging last night, I don't think that's the problem. But I'll move it anyway

@sjelin sjelin changed the title feat(browser): chain promises in lib/browser.ts and return promise … WIP --- feat(browser): chain promises in lib/browser.ts and return promise … Jan 26, 2017
@sjelin sjelin changed the title WIP --- feat(browser): chain promises in lib/browser.ts and return promise … WIP --- feat(browser): chain promises in browser.get Jan 26, 2017
@sjelin
Copy link
Contributor Author

sjelin commented Jan 26, 2017

Since it appears to be only the browser.get chaining that's causing problems, I've split everything else into a separate PR: #4021

@heathkit
Copy link
Contributor

Looking through the tcpdump from the earlier failure, and it looks like bp.setSynchronization(true) is getting called before addBaseMockModules. BP then runs the wait script, which fails because angular isn't on the page. Not totally sure what's going on, but it is looking like there was some kind of ordering issue earlier.

@sjelin sjelin changed the title WIP --- feat(browser): chain promises in browser.get feat(browser): chain promises in browser.get Jan 26, 2017
@sjelin
Copy link
Contributor Author

sjelin commented Jan 26, 2017

@mgiambalvo @juliemr it appears the problem was solved by rebasing, whatever it was

@sjelin sjelin merged commit 77525af into angular:noCF Jan 27, 2017
sjelin added a commit to sjelin/protractor that referenced this pull request Jan 27, 2017
igniteram pushed a commit to igniteram/protractor that referenced this pull request Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Support ControlFlow-less Selenium
Stage 1: Internals (PRs)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants