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

test: fix flaky tests Dapp viewed Event... Permissions Page... and Connections Page... due to overlaying popup #24580

Merged
merged 5 commits into from
May 20, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented May 17, 2024

Description

This is a PR for fixing the flaky tests:

  • Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before
  • Permissions Page should redirect users to connections page when users click on connected permission
  • Connections page should disconnect when click on Disconnect button in connections page

The problem seems to be that we are trying to click an element, but there is the previous notifcation element on top and has not disappeared yet at the moment we want to perform the click operation.

We can see this in the artifacts of the failure, where the blue notification popup is still there, despite having already clicked 'Got it'.
This is also confirmed by the error message in the logs:

[driver] Called 'clickElement' with arguments [{"text":"Got it","tag":"button"}]
[driver] Called 'clickElement' with arguments [{"text":"127.0.0.1:8080","tag":"p"}]
Failure on testcase: 'Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before', for more information see the artifacts tab in CI
ElementClickInterceptedError: Element <p class="mm-box mm-text mm-text--body-md mm-text--elllipsis mm-text--text-align-left mm-box--color-text-default">
 is not clickable at point (444,92) because another element <h4 class="mm-box mm-text mm-text--heading-sm mm-text--text-align-center mm-box--width-full mm-box--color-info-inverse"> obscures it

The solution here is to wait for the element to disappear. Since this seems a case that can happen more often, a new driver function has been introduced, which basically clicks an element and waits until it disappears. This can be suitable for when accepting notifications/popup that whenever we accept them, those should disappear before we proceed with the next action.

  async clickElementAndWaitToDisappear(rawLocator) {
    const element = await this.findClickableElement(rawLocator);
    await element.click();
    await element.waitForElementState('hidden');
  }

Related issues

Fixes: #24575 and #24597

Manual testing steps

  1. locally: run the test locally several times yarn test:e2e:single test/e2e/tests/metrics/dapp-viewed.spec.js --browser=chrome --leave-running=true
  2. ci: Check ci and see is not failing

Screenshots/Recordings

Artifacts and logs from ci that helped in the investigation:

image

Screenshot from 2024-05-17 11-27-45

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@seaona seaona self-assigned this May 17, 2024
@seaona seaona added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 17, 2024
@seaona seaona marked this pull request as ready for review May 17, 2024 10:08
@seaona seaona requested a review from a team as a code owner May 17, 2024 10:08
@seaona seaona changed the title test: fix flaky test "Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before" https://github.com/MetaMask/metamask-extension/issues/24575 May 17, 2024
@seaona seaona changed the title https://github.com/MetaMask/metamask-extension/issues/24575 test: fix flaky test "Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before" May 17, 2024
@legobeat legobeat requested a review from a team May 20, 2024 07:39
@seaona seaona requested a review from legobeat May 20, 2024 09:42
legobeat
legobeat previously approved these changes May 20, 2024
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM!

#24607)

<!--
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 the following flaky tests:

- Permissions Page should redirect users to connections page when users
click on connected permission
- Connections page should disconnect when click on Disconnect button in
connections page

After this PR is merged, for fixing this tests we use the same approach,
utilizing the new method for waiting for the notification to disappear
after accepting it

```
async clickElementAndWaitToDisappear(rawLocator) {
    const element = await this.findClickableElement(rawLocator);
    await element.click();
    await element.waitForElementState('hidden');
  }
```

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

## **Related issues**

Fixes: #24597

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**
Artifacts from ci


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/e0bdfeab-8fec-4201-b447-e65dd552ebc7)


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

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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
Copy link
Contributor Author

seaona commented May 20, 2024

@legobeat there were more instances of this type of flakyiness, so I've decided to add the fixes in this same PR. Apologies for resetting the review, I just saw it now in this ticket
🙏 🙇‍♀️
#24597

@seaona seaona changed the title test: fix flaky test "Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before" test: fix flaky tests Dapp viewed Event... Permissions Page... and Connections Page... due to overlaying popup May 20, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [27477cb]
Page Load Metrics (910 ± 503 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66156932512
domContentLoaded7461394
load5225539101047503
domInteractive7461394
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@legobeat legobeat requested a review from a team May 20, 2024 11:01
Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.37%. Comparing base (4cf7e80) to head (0766ef0).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #24580   +/-   ##
========================================
  Coverage    67.37%   67.37%           
========================================
  Files         1289     1289           
  Lines        50220    50220           
  Branches     13005    13005           
========================================
  Hits         33835    33835           
  Misses       16385    16385           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks !

@seaona seaona removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 20, 2024
@seaona seaona merged commit caa8876 into develop May 20, 2024
71 of 73 checks passed
@seaona seaona deleted the flaky-fix-dapp-viewed branch May 20, 2024 13:02
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2024
@metamaskbot metamaskbot added the release-11.18.0 Issue or pull request that will be included in release 11.18.0 label May 20, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [0766ef0]
Page Load Metrics (858 ± 520 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint72160972613
domContentLoaded9391473
load5925908581084520
domInteractive9391473
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flaky tests release-11.18.0 Issue or pull request that will be included in release 11.18.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Flaky Test: "Dapp viewed Event @no-mmi is sent when reconnect to a dapp that has been connected before"
6 participants