Skip to content

add timeouts & messages to all driver.wait calls #4841

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

Merged
merged 2 commits into from
May 28, 2019

Conversation

cwillisf
Copy link
Contributor

@cwillisf cwillisf commented May 22, 2019

Resolves

Should help debug multiple issues with the GUI integration tests such as #4653

Proposed Changes

For each call to Selenium's wait method, set a timeout which is shorter than the overall Jasmine timeout and also include a custom message to help indicate which call timed out.

Reason for Changes

Without these changes every timeout displays the same message with the same call stack -- a call stack which doesn't include any of our test files:

Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
      
      at node_modules/jest-jasmine2/build/queue_runner.js:64:21
      at Timeout.callback [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:523:19)
      at ontimeout (timers.js:498:11)
      at tryOnTimeout (timers.js:323:5)
      at Timer.listOnTimeout (timers.js:290:5)

The new message for a timeout during findByXpath looks something like this:

    TimeoutError: findByXpath timed out for path: //body//*//*[contains(text(), 'Code')]
    Wait timed out after 20014ms

      at node_modules/selenium-webdriver/lib/promise.js:2201:17
      at ManagedPromise.invokeCallback_ (node_modules/selenium-webdriver/lib/promise.js:1376:14)
      at TaskQueue.execute_ (node_modules/selenium-webdriver/lib/promise.js:3084:14)
      at TaskQueue.executeNext_ (node_modules/selenium-webdriver/lib/promise.js:3067:27)
      at asyncRun (node_modules/selenium-webdriver/lib/promise.js:2927:27)
      at node_modules/selenium-webdriver/lib/promise.js:668:7
      at process._tickCallback (internal/process/next_tick.js:68:7)
      From: Task: findByXpath timed out for path: //body//*//*[contains(text(), 'Code')]
      at scheduleWait (node_modules/selenium-webdriver/lib/promise.js:2188:20)
      at ControlFlow.wait (node_modules/selenium-webdriver/lib/promise.js:2517:12)
      at thenableWebDriverProxy.wait (node_modules/selenium-webdriver/lib/webdriver.js:934:29)
      at SeleniumHelper.findByXpath (test/helpers/selenium-helper.js:89:26)
      at SeleniumHelper.wrapper [as findByXpath] (node_modules/lodash.bindall/index.js:550:15)
      at SeleniumHelper.findByText (test/helpers/selenium-helper.js:94:19)
      at SeleniumHelper.wrapper [as findByText] (node_modules/lodash.bindall/index.js:550:15)
      at SeleniumHelper.clickText (test/helpers/selenium-helper.js:117:19)
      at wrapper (node_modules/lodash.bindall/index.js:550:15)
      at _callee2$ (test/integration/blocks.test.js:32:15)

An earlier version of this change included detailed messages for element-based timeouts as well (waitUntilGone and elementVisible) but they failed in some cases. I started trying to make them work for all cases and the complexity started growing rapidly so I decided to leave that as future work. This version still reports which function timed out (waitUntilGone for example) along with a call stack, which is better than what we had before.

@cwillisf
Copy link
Contributor Author

That awkward moment when you kinda want a build to fail on Travis but it succeeds...

@cwillisf cwillisf force-pushed the test-failure-feedback branch from 558f295 to 1b055af Compare May 23, 2019 01:26
Copy link
Contributor

@BryceLTaylor BryceLTaylor left a comment

Choose a reason for hiding this comment

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

Though I think the change is good, it's implementation is a little hard to follow. A less clever implementation will make it easier to maintain.

Copy link
Contributor

@BryceLTaylor BryceLTaylor left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants