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

Replace Leadfoot with WebDriver API #26477

Merged

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Nov 30, 2018

@elasticmachine
Copy link
Contributor

💔 Build Failed

@liza-mae liza-mae mentioned this pull request Dec 3, 2018
8 tasks
@dmlemeshko dmlemeshko force-pushed the fix/webdriver-service-to-replace-leadfoot branch 2 times, most recently from 0e22f4a to 13b6716 Compare December 3, 2018 16:36
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@dmlemeshko dmlemeshko force-pushed the fix/webdriver-service-to-replace-leadfoot branch from 13b6716 to 94ddf21 Compare December 4, 2018 10:46
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@dmlemeshko dmlemeshko added WIP Work in progress Team:QA Team label for QA Team labels Dec 5, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-qa

@dmlemeshko dmlemeshko force-pushed the fix/webdriver-service-to-replace-leadfoot branch 2 times, most recently from aa1ca7d to 19c6ab9 Compare December 6, 2018 14:24
@elasticmachine
Copy link
Contributor

💔 Build Failed

@dmlemeshko dmlemeshko force-pushed the fix/webdriver-service-to-replace-leadfoot branch from 19c6ab9 to 085a7e1 Compare December 6, 2018 16:14
return await this.exists(async driver => await driver.findElements(By.css(selector)), timeout);
}

async moveMouseTo(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we put moveMouseTo() on the element is because it requires the element that is being wrapped instead of the wrapper itself. I think we should keep it that way so that we can keep direct access to the webdriver element inside of the wrapper and because I think element.moveMouseTo() is more logical than find.moveMouseTo(element). If there is a good reason not to put moveMouseTo() on element then I think we should put it on browser instead.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

await this.byLinkText(selector, timeout);
return wrapAll(await driver.findElements(By.linkText(selector)));
} catch (err) {
return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test the error here to make sure it's the specific type of error we want to replace with an empty array? There are other types of errors that we want to rethrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, we can. I will specify it explicitly

@elasticmachine

This comment has been minimized.

@spalger spalger dismissed their stale review February 15, 2019 23:36

Examining the deadlock we just saw, I'm not comfortable putting this out there is a serious chance it will randomly deadlock on us

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

spalger added 3 commits February 15, 2019 16:29
Best we can tell WebDriver locks up sometimes when we send too many
commands at once, sometimes... It causes random lockups where we never
receive another response from WedDriver and we don't want to live with
that risk, so for now I've shimmed the Executor class in WebDiver to
queue all calls to Executor#send() if there is already a call in
progress.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 16, 2019

💚 Build Succeeded

Reminder, this build ran the tests 4 times.

@spalger
Copy link
Contributor

spalger commented Feb 16, 2019

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 16, 2019

💚 Build Succeeded

Again, this build ran the tests 4 times.

@spalger
Copy link
Contributor

spalger commented Feb 16, 2019

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 16, 2019

💚 Build Succeeded

Again, this build ran the tests 4 times.

@spalger
Copy link
Contributor

spalger commented Feb 16, 2019

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 16, 2019

💚 Build Succeeded

Alright, that's 16 total passes.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Alright, ready to merge when you are @dmlemeshko!

@dmlemeshko dmlemeshko merged commit 0bd3b4f into elastic:master Feb 16, 2019
spalger pushed a commit to spalger/kibana that referenced this pull request Feb 18, 2019
spalger pushed a commit that referenced this pull request Feb 18, 2019
* Revert "Replace Leadfoot with WebDriver API (#26477)"

This reverts commit 0bd3b4f.

* leadfoot expectes execute args to be an array

* disable flaky graphql tests
@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented May 4, 2019

Seriously?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:QA Team label for QA Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants