Skip to content

Commit

Permalink
fix: flaky test `Vault Decryptor Page is able to decrypt the vault pa…
Browse files Browse the repository at this point in the history
…sting the text in the vault-decryptor webapp` (#27921)

<!--
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**
The problem is that we are clicking an element when it's moving, making
the click take no effect.
In the onboarding steps, there is a carousel, where you can move from
one page to the other. The issue is that the classical methods
findElement or clickElement, can take place without having the correct
page fully visible (while it's moving), making the click, take no
effect.

To avoid our clicks taking no effect, we need to add a new method to
wait until an element is not moving. That's the condition we need before
clicking that element.

Note. adding this by default to the clickElement can be an overkill as
the check will delay all instances of clickElement. Since this only
happens in the onboarding, I decided to **not** add it by default in the
clickEelemtn for this reason. Only in the onboarding flow, we need to
make sure we wait for elements not moving.


![Screenshot from 2024-10-17
08-48-13](https://github.com/user-attachments/assets/3b4df3b3-8d3b-49b7-8de7-34082410cfdd)


![image](https://github.com/user-attachments/assets/dbd1d928-3c32-42c4-8e2a-006f1345a54e)


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

## **Related issues**

Fixes: #27922

## **Manual testing steps**

1. Check ci run here
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/106117/workflows/e0bb3426-1c21-481b-8c0b-4a417f149172/jobs/3963202

## **Screenshots/Recordings**





https://github.com/user-attachments/assets/1d00a2b6-3b03-482f-a6c1-c14fb7e5c318


## **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.
  • Loading branch information
seaona authored Oct 17, 2024
1 parent 043ea3f commit f58d598
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 26 deletions.
23 changes: 19 additions & 4 deletions test/e2e/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,10 @@ const onboardingRevealAndConfirmSRP = async (driver) => {

await driver.clickElement('[data-testid="confirm-recovery-phrase"]');

await driver.clickElement({ text: 'Confirm', tag: 'button' });
await driver.clickElementAndWaitToDisappear({
tag: 'button',
text: 'Confirm',
});
};

/**
Expand Down Expand Up @@ -566,7 +569,7 @@ const onboardingCompleteWalletCreationWithOptOut = async (driver) => {
await driver.findElement({ text: 'Congratulations!', tag: 'h2' });

// opt-out from third party API on general section
await driver.clickElement({
await driver.clickElementAndWaitToDisappear({
text: 'Manage default privacy settings',
tag: 'button',
});
Expand All @@ -575,7 +578,10 @@ const onboardingCompleteWalletCreationWithOptOut = async (driver) => {
'[data-testid="basic-functionality-toggle"] .toggle-button',
);
await driver.clickElement('[id="basic-configuration-checkbox"]');
await driver.clickElement({ text: 'Turn off', tag: 'button' });
await driver.clickElementAndWaitToDisappear({
tag: 'button',
text: 'Turn off',
});

// opt-out from third party API on assets section
await driver.clickElement('[data-testid="category-back-button"]');
Expand All @@ -588,10 +594,19 @@ const onboardingCompleteWalletCreationWithOptOut = async (driver) => {
).map((toggle) => toggle.click()),
);
await driver.clickElement('[data-testid="category-back-button"]');

// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving(
'[data-testid="privacy-settings-back-button"]',
);
await driver.clickElement('[data-testid="privacy-settings-back-button"]');

// complete onboarding
await driver.clickElement({ text: 'Done', tag: 'button' });
await driver.clickElementAndWaitToDisappear({
tag: 'button',
text: 'Done',
});
await onboardingPinExtension(driver);
};

Expand Down
41 changes: 24 additions & 17 deletions test/e2e/tests/onboarding/onboarding.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,28 +328,24 @@ describe('MetaMask onboarding @no-mmi', function () {
tag: 'button',
});

await driver.clickElement({ text: 'Save', tag: 'button' });

await driver.delay(largeDelayMs);
await driver.waitForSelector('[data-testid="category-back-button"]');
const generalBackButton = await driver.waitForSelector(
'[data-testid="category-back-button"]',
);
await generalBackButton.click();
await driver.clickElementAndWaitToDisappear({
tag: 'button',
text: 'Save',
});

await driver.delay(largeDelayMs);
await driver.clickElement('[data-testid="category-back-button"]');

await driver.waitForSelector(
// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving(
'[data-testid="privacy-settings-back-button"]',
);
const defaultSettingsBackButton = await driver.findElement(

await driver.clickElement(
'[data-testid="privacy-settings-back-button"]',
);
await defaultSettingsBackButton.click();

await driver.delay(largeDelayMs);

await driver.clickElement({
await driver.clickElementAndWaitToDisappear({
text: 'Done',
tag: 'button',
});
Expand All @@ -359,9 +355,14 @@ describe('MetaMask onboarding @no-mmi', function () {
tag: 'button',
});

await driver.delay(largeDelayMs);
// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving({
text: 'Done',
tag: 'button',
});

await driver.clickElement({
await driver.clickElementAndWaitToDisappear({
text: 'Done',
tag: 'button',
});
Expand Down Expand Up @@ -412,6 +413,12 @@ describe('MetaMask onboarding @no-mmi', function () {
await driver.clickElement('[id="basic-configuration-checkbox"]');
await driver.clickElement({ text: 'Turn off', tag: 'button' });
await driver.clickElement('[data-testid="category-back-button"]');

// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving(
'[data-testid="privacy-settings-back-button"]',
);
await driver.clickElement(
'[data-testid="privacy-settings-back-button"]',
);
Expand Down
30 changes: 25 additions & 5 deletions test/e2e/tests/privacy/basic-functionality.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,35 @@ describe('MetaMask onboarding @no-mmi', function () {
'[data-testid="currency-rate-check-toggle"] .toggle-button',
);
await driver.clickElement('[data-testid="category-back-button"]');
await driver.delay(regularDelayMs);

// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving(
'[data-testid="privacy-settings-back-button"]',
);
await driver.clickElement(
'[data-testid="privacy-settings-back-button"]',
);
await driver.delay(regularDelayMs);

await driver.clickElement({ text: 'Done', tag: 'button' });
await driver.clickElement('[data-testid="pin-extension-next"]');
await driver.clickElement({ text: 'Done', tag: 'button' });
await driver.clickElementAndWaitToDisappear({
text: 'Done',
tag: 'button',
});
await driver.clickElement({
text: 'Next',
tag: 'button',
});

// Wait until the onboarding carousel has stopped moving
// otherwise the click has no effect.
await driver.waitForElementToStopMoving({
text: 'Done',
tag: 'button',
});
await driver.clickElementAndWaitToDisappear({
text: 'Done',
tag: 'button',
});

await driver.clickElement('[data-testid="network-display"]');

Expand Down
40 changes: 40 additions & 0 deletions test/e2e/webdriver/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,46 @@ class Driver {
}
}

/**
* Checks if an element is moving by comparing its position at two different times.
*
* @param {string | object} rawLocator - Element locator.
* @returns {Promise<boolean>} Promise that resolves to a boolean indicating if the element is moving.
*/
async isElementMoving(rawLocator) {
const element = await this.findElement(rawLocator);
const initialPosition = await element.getRect();

await new Promise((resolve) => setTimeout(resolve, 500)); // Wait for a short period

const newPosition = await element.getRect();

return (
initialPosition.x !== newPosition.x || initialPosition.y !== newPosition.y
);
}

/**
* Waits until an element stops moving within a specified timeout period.
*
* @param {string | object} rawLocator - Element locator.
* @param {number} timeout - The maximum time to wait for the element to stop moving.
* @returns {Promise<void>} Promise that resolves when the element stops moving.
* @throws {Error} Throws an error if the element does not stop moving within the timeout period.
*/
async waitForElementToStopMoving(rawLocator, timeout = 5000) {
const startTime = Date.now();

while (Date.now() - startTime < timeout) {
if (!(await this.isElementMoving(rawLocator))) {
return;
}
await new Promise((resolve) => setTimeout(resolve, 500)); // Check every 500ms
}

throw new Error('Element did not stop moving within the timeout period');
}

/** @param {string} title - The title of the window or tab the screenshot is being taken in */
async takeScreenshot(title) {
const filepathBase = `${artifactDir(title)}/test-screenshot`;
Expand Down

0 comments on commit f58d598

Please sign in to comment.