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

Fix for Nightmare waitTimeout. Resolves #236 #391

Merged
merged 5 commits into from Feb 4, 2017

Conversation

Projects
None yet
2 participants
@HughZurname
Contributor

HughZurname commented Jan 24, 2017

Hi,

I spotted an issue with the waitFor functions in the Nightmare helper as mentioned in #236.

In cases where the element you're waiting for takes longer than 30 seconds to appear, the default nightmare timeout is hit. The reason this is happening is that optionWaitTimeout isn't valid in Nightmare and was causing it to fall back to it's default of 30 seconds.

I've changed this to correctly set the Nightmare options.waitTimeout value. I also removed the /1000 on waitForTimeout as Nightmare requires milliuseconds and the functions below are the only place this was being used.

I added a test for for the timeout function on waitForText

@DavertMik

Thank you very much. Good catch and good fix!

@@ -759,7 +759,7 @@ class Nightmare extends Helper {
context = this.context;
}
let locator = guessLocator(context) || { css: context};
this.browser.optionWaitTimeout = sec * 1000 || this.options.waitForTimeout;
this.browser.options.waitTimeout = sec * 1000 || this.options.waitForTimeout;

This comment has been minimized.

@DavertMik

DavertMik Jan 24, 2017

Member

yeah, i see the bug. It was due different implementations in WebDriverIO, SeleniumWebDriver and Nightmare

@DavertMik

DavertMik Jan 24, 2017

Member

yeah, i see the bug. It was due different implementations in WebDriverIO, SeleniumWebDriver and Nightmare

@@ -502,6 +502,13 @@ module.exports.tests = function() {
.then(() => I.waitForText('Dynamic text', 2, '#text'))
.then(() => I.see('Dynamic text'));
});
it('should wait for text after timeout', () => {

This comment has been minimized.

@DavertMik

DavertMik Jan 24, 2017

Member

thanks for the test!

@DavertMik

DavertMik Jan 24, 2017

Member

thanks for the test!

@HughZurname

This comment has been minimized.

Show comment
Hide comment
@HughZurname

HughZurname Jan 25, 2017

Contributor

No worries, happy to help! :)

I've pushed some updates to make the test work. It was weirdly failing on the first commit but not returning an exit code of 1 from mocha so the build was passing.

Contributor

HughZurname commented Jan 25, 2017

No worries, happy to help! :)

I've pushed some updates to make the test work. It was weirdly failing on the first commit but not returning an exit code of 1 from mocha so the build was passing.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Feb 4, 2017

Member

Thank you, getting it merged!

Member

DavertMik commented Feb 4, 2017

Thank you, getting it merged!

@DavertMik DavertMik merged commit e551869 into Codeception:master Feb 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment