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

[Refactor] Replace getText() with findElement using css #19870

Open
DDDDDanica opened this issue Jul 4, 2023 · 2 comments · Fixed by #27606 or #27664
Open

[Refactor] Replace getText() with findElement using css #19870

DDDDDanica opened this issue Jul 4, 2023 · 2 comments · Fixed by #27606 or #27664
Assignees
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-extension-platform

Comments

@DDDDDanica
Copy link
Contributor

DDDDDanica commented Jul 4, 2023

After a few fixes for flaky tests in MV3, it is noticeable that lots are related to an assertion pattern change, which is to find an element, then assert what its contents are. There's a racing condition in some e2e tests to find the dom and assert, and we'll try to avoid this pattern.

Proposed by Mark in this thread

e.g. Instead of this:

const element = await driver.findElement('[data-testid="foo"]');
assert(await element.getText(), 'Bar')

Do this:

await driver.findElement({
  css: '[data-testid="foo"]',
  text: 'Bar',
});

Hence, we want to introduce an new lint rules to forbid this pattern from being written;
Also refactor all existing places we used this pattern to reduce the noise if monitoring MV3 e2e tests.

Acceptance criteria:

  1. lint error for using findElement and getText when comes to comparing element text and string
  2. all existing tests use new findElement pattern.
github-merge-queue bot pushed a commit that referenced this issue Oct 7, 2024
)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This PR fixes an anti-pattern in our e2e tests, where we assert that an
element value is equal to a condition. This opens the door to a race
condition where the element is already present, but not the value we
want yet, making the assertion to fail.
We should find the element by its value, instead of asserting its inner
value.



[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27606?quickstart=1)

## **Related issues**

Fixes: #19870
Note: we will need several PRs to address all the occurrences, so we
cannot close the issue yet, despite merging this first PR


## **Manual testing steps**

1. Check ci

## **Screenshots/Recordings**

n/a
<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 7, 2024
@seaona seaona reopened this Oct 7, 2024
@seaona
Copy link
Contributor

seaona commented Oct 7, 2024

I re-open the issue, since we'll need several PRs to fix all occurrences. So far we merged 1

github-merge-queue bot pushed a commit that referenced this issue Oct 8, 2024
)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

This PR fixes an anti-pattern in our e2e tests, where we assert that an
element value is equal to a desired value. This opens the door to a race
condition where the element is already present, but it does not have the
value we want yet, making the assertion to fail.
We should find the element by its value, instead of asserting its inner
value.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27664?quickstart=1)

## **Related issues**

Fixes: #19870
Note: this is the second PR for this work. The first PR was merged
[here](#27606)

## **Manual testing steps**

1. Check ci

## **Screenshots/Recordings**
n/a

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@seaona seaona reopened this Oct 16, 2024
@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-extension-platform
Projects
None yet
4 participants