Skip to content

use locator abstraction in tests folder#10833

Merged
brad-decker merged 2 commits intodevelopfrom
use-locator-abstraction
Apr 7, 2021
Merged

use locator abstraction in tests folder#10833
brad-decker merged 2 commits intodevelopfrom
use-locator-abstraction

Conversation

@brad-decker
Copy link
Contributor

Implements the abstraction for the 'by' method from selenium implemented in #10802

@brad-decker brad-decker marked this pull request as ready for review April 6, 2021 18:44
@brad-decker brad-decker requested a review from a team as a code owner April 6, 2021 18:44
@brad-decker brad-decker requested a review from shanejonas April 6, 2021 18:44
@Gudahtt
Copy link
Member

Gudahtt commented Apr 7, 2021

This has conflicts, and will conflict will #10820 as well.

cc @NiranjanaBinoy, maybe we can coordinate this to avoid further conflicts? e.g. merge #10820, then this, then the rest of the fixture migrations after that (or something along those lines).

@brad-decker
Copy link
Contributor Author

I can also break this out into smaller pieces, prioritizing those files that are either a few steps away from being refactored or have already been refactored.

@Gudahtt
Copy link
Member

Gudahtt commented Apr 7, 2021

That could work! Though I expect that I'd be able to review this fairly quickly, since the changes are all pretty easy to follow. If we're quick to review and merge this, the all-at-once approach might result in the least conflicts.

@brad-decker brad-decker force-pushed the use-locator-abstraction branch from c9a4c0d to a6b2629 Compare April 7, 2021 14:21
@Gudahtt
Copy link
Member

Gudahtt commented Apr 7, 2021

#10820 has been merged now as well, and I've just heard from @NiranjanaBinoy that there aren't any further big changes in draft yet, so now would be a good time to get this updated and reviewed/merged

@brad-decker brad-decker force-pushed the use-locator-abstraction branch from a6b2629 to e1c8b06 Compare April 7, 2021 14:24
@brad-decker
Copy link
Contributor Author

@Gudahtt updated the branch with latest from develop

await driver.clickElement(
By.xpath(`//button/div/div[contains(text(), "Fast")]`),
);
await driver.clickElement({ text: 'Fast', tag: 'button/div/div' });
Copy link
Member

Choose a reason for hiding this comment

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

I guess this means the tag property is only compatible with xpath 🤔

It might be worth using the xpath locator here explicitly if we want to make the text and tag-based locators implementable without using xpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you make text locators implementable without xpath with Selenium? For what its worth this can be expressed very easily with Playwright button > div > div:has-text("fast")

Copy link
Member

Choose a reason for hiding this comment

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

Oh I meant in Playwright - I think we need to use xpath for this in Selenium.

if this slash syntax is easily parsed/handled by Playwright that works too! Just wasn't sure it was portable. That approach you just gave an example of seems fine to me.

await driver.clickElement({ text: 'Settings', tag: 'div' });

// await driver.findElement(By.css('.tab-bar'))
// await driver.findElement('.tab-bar')
Copy link
Member

Choose a reason for hiding this comment

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

😂 Whoops! I wonder how that got there.

@metamaskbot
Copy link
Collaborator

Builds ready [e1c8b06]
Page Load Metrics (562 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint458662126
domContentLoaded3856715619847
load3876725629847
domInteractive3856715599747

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

The only remaining uses of the selenium By API are in the drivers themselves. All of the changes here seem functionally equivalent to me.

@brad-decker brad-decker merged commit cd97340 into develop Apr 7, 2021
@brad-decker brad-decker deleted the use-locator-abstraction branch April 7, 2021 14:57
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants