Skip to content

Commit

Permalink
test: Fix flaky e2e tests | Add retry logic to switchToWindowWithTitl…
Browse files Browse the repository at this point in the history
…e | Update retry helper fn (#21394)

## **Description**
We've recently had a [spike in flaky e2e
tests](https://app.circleci.com/insights/github/MetaMask/metamask-extension/workflows/test_and_release/tests).
We observed these tests to have a common denominator:
`switchToWindowWithTitle`. This PR adds retry logic to
`switchToWindowWithTitle`.

After the first try, the logic will retry a max number of 8 times with
2500ms delay in-between. These numbers are arbitrary and generous just
in case.

This PR also updates the retry logic in `development/lib/retry.js` to
return the function value on success.

Links:
- [CircleCI Insights - Flaky
Tests](https://app.circleci.com/insights/github/MetaMask/metamask-extension/workflows/test_and_release/tests)
- [Internal
discussion](https://consensys.slack.com/archives/GTQAGKY5V/p1697150920731179)

## **Manual testing steps**

1. Run CI/CD to observe tests

## **Screenshots/Recordings**

### **Before**

Screenshot 2023-10-16 21:00:
<img
src="https://github.com/MetaMask/metamask-extension/assets/20778143/f6cde1bb-b85e-4ceb-81ad-f7d8ba9f7ceb"
width="420" />

Screenshot 2023-10-16 21:50:
<img
src="https://github.com/MetaMask/metamask-extension/assets/20778143/71583e57-6e80-4129-a51f-560b65b2bbfa"
width="420" />

### **After**

## **Related issues**

Fixes #17047

## **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 clearly explained:
  - [ ] What problem this PR is solving.
  - [ ] How this problem was solved.
  - [ ] How reviewers can test my changes.
- [ ] I’ve indicated what issue this PR is linked to: Fixes #???
- [ ] I’ve included tests if applicable.
- [ ] I’ve documented any added code.
- [ ] 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)).
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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
digiwand authored Oct 17, 2023
1 parent 4af7578 commit 957c8b9
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 7 deletions.
12 changes: 7 additions & 5 deletions development/lib/retry.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
* @param {string} args.retryUntilFailure - Retries until the function fails.
* @param {Function} functionToRetry - The function that is run and tested for
* failure.
* @returns {Promise<null | Error>} a promise that either resolves to null if
* the function is successful or is rejected with rejectionMessage otherwise.
* @returns {Promise<* | null | Error>} a promise that either resolves with one of the following:
* - If successful, resolves with the return value of functionToRetry.
* - If functionToRetry fails while retryUntilFailure is true, resolves with null.
* - Otherwise it is rejected with rejectionMessage.
*/
async function retry(
{
Expand All @@ -33,14 +35,14 @@ async function retry(
}

try {
await functionToRetry();
const result = await functionToRetry();
if (!retryUntilFailure) {
return;
return result;
}
} catch (error) {
console.error(error);
if (retryUntilFailure) {
return;
return null;
}
} finally {
attempts += 1;
Expand Down
15 changes: 13 additions & 2 deletions test/e2e/webdriver/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
until,
} = require('selenium-webdriver');
const cssToXPath = require('css-to-xpath');
const { retry } = require('../../../development/lib/retry');

/**
* Temporary workaround to patch selenium's element handle API with methods
Expand Down Expand Up @@ -435,15 +436,25 @@ class Driver {
initialWindowHandles,
delayStep = 1000,
timeout = this.timeout,
{ retries = 8, retryDelay = 2500 } = {},
) {
let windowHandles =
initialWindowHandles || (await this.driver.getAllWindowHandles());
let timeElapsed = 0;

while (timeElapsed <= timeout) {
for (const handle of windowHandles) {
await this.driver.switchTo().window(handle);
const handleTitle = await retry(
{
retries,
delay: retryDelay,
},
async () => {
await this.driver.switchTo().window(handle);
return await this.driver.getTitle();
},
);

const handleTitle = await this.driver.getTitle();
if (handleTitle === title) {
return handle;
}
Expand Down

0 comments on commit 957c8b9

Please sign in to comment.