Skip to content

Commit

Permalink
fix: flaky test `Request-queue UI changes handles three confirmations…
Browse files Browse the repository at this point in the history
… on three confirmations concurrently` (#25675)

<!--
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 queue UI test fails has a couple of flaky points.

Sometimes we see the console errors for network unresponsive -- when
this happens the test fails.
When we don't see the console errors for network unresponsive the test
passes. The problem is that in the tests we were ignoring the
PollingBlock errors, but more errors appear when the network is down,
making the test fail when they appear in the console.

![Screenshot from 2024-07-05
14-45-37](https://github.com/MetaMask/metamask-extension/assets/54408225/4f105322-3bd0-4908-a107-8984fce1915d)


https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/90602/workflows/e41e1e65-d445-4b60-9602-89a261cc5654/jobs/3362082/parallel-runs/4?filterBy=ALL

There are more sources of flakiness, that can also cause this test to
fail. Those are described here and fixed in this same PR:

- Performing a subsequent action when the popup is closed, causes the
`NoSuchWindowError: no such window: target window already closed` error.
We need to switch back to an existing window, before proceeding with
test actions
- Triggering a tx/signature with an unresponsive network (when we kill
ganache) takes a significant amount of time to open the popup. Now we
wait for the expected number of windows before switching to the popup
and we add a big timeout for this case (it's a max value and won't delay
the test if it's not needed, like in FF). There's an issue open to fix
this on the app level in [this
ticket](#25690).
- Asserting values from elements found by their css/testid can introduce
flakiness as the elements could be rendered without having the expected
text yet loaded. Now the approach for these validations has been fixed
by looking for the exact selector which includes the text
- A lavamoat error appeared once, but this seem unrelated to this
specific test and just saw it once so it's left outside the scope of
this PR `error caught in retry(): JavascriptError: javascript error:
LavaMoat - property "name" of globalThis is inaccessible under scuttling
mode. To learn more visit
https://github.com/LavaMoat/LavaMoat/pull/360.`


- Failure 2: 
-
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/90432/workflows/81444bef-1d2c-4da0-bf60-1c0c51a5b530/jobs/3353522/parallel-runs/17

- Logs:

```
[driver] Called 'switchToWindowWithTitle' with arguments ["MetaMask Dialog",["8C425F6EC00FF98E5C0A643C0CDF0DD6","0BF6602327E4E63F0FAE641DF2BB71E0","45802EBF75EF61BE99D7F235354A0FC6","109C90EB942C664573D051FD7952C0CE","4199B96477AFE13BB203FFC80C399B9F"]]
[driver] Called 'findClickableElement' with arguments ["[data-testid=\"confirmation-submit-button\"]"]
[driver] Called 'clickElement' with arguments ["[data-testid=\"confirmation-submit-button\"]"]
[driver] Called 'openNewPage' with arguments ["http://127.0.0.1:8082"]
Failed to handle request: socket hang up
Failure on testcase: 'Request-queue UI changes handles three confirmations on three confirmations concurrently @no-mmi', for more information see the artifacts tab in CI

NoSuchWindowError: no such window: target window already closed
from unknown error: web view not found
```


- Failure 3:
-
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/90534/workflows/87faa6e4-aa65-4273-8d7e-d369782cfe1d/jobs/3358659/parallel-runs/2

- Logs from error 2:

```
JsonRpcEngine: Response has no error or result for request:
{
  "jsonrpc": "2.0",
  "id": "8871ca57-2969-45f2-8838-115e34bfa417",
  "method": "eth_blockNumber",
  "params": [],
  "origin": "metamask",
  "networkClientId": "networkConfigurationId",
  "tabId": 1870836432
}
  at m._fetchLatestBlock (chrome-extension://aohcjlklkjjpdgebibknknkinbdpdmoe/common-1.js:1:357185)
  at async m._updateLatestBlock (chrome-extension://aohcjlklkjjpdgebibknknkinbdpdmoe/common-1.js:1:356869)
  at async m._updateAndQueue (chrome-extension://aohcjlklkjjpdgebibknknkinbdpdmoe/common-1.js:1:357352)
  at m._updateAndQueue (chrome-extension://aohcjlklkjjpdgebibknknkinbdpdmoe/common-1.js:1:357401)

----------End of Chrome error----------
---This error is on the ignore list----


[driver] Called 'findElement' with arguments ["[data-testid=\"network-display\"], [data-testid=\"signature-request-network-display\"]"]
[driver] Called 'findElement' with arguments [".confirm-page-container-summary__origin bdi, .request-signature__origin .chip__label"]
[driver] Called 'executeScript' with arguments [null]
Failure on testcase: 'Request-queue UI changes should gracefully handle network connectivity failure for confirmations @no-mmi', for more information see the artifacts tab in CI
```

- Failure 4: hasn't appeared but since it could potentially appear in
the future, has also been fixed


https://github.com/MetaMask/metamask-extension/assets/54408225/4ba1effc-61a4-4fe5-b4a9-c2785856eba3





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

## **Related issues**

Fixes: #25674

## **Manual testing steps**

1. Check ci

## **Screenshots/Recordings**
![Screenshot from 2024-07-04
09-03-08](https://github.com/MetaMask/metamask-extension/assets/54408225/8acaf835-7b25-4fb6-b0fc-fa01f026d627)

![Screenshot from 2024-07-05
10-03-57](https://github.com/MetaMask/metamask-extension/assets/54408225/22530cc1-376b-4075-9871-43fff63bbe10)

## **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 Jul 5, 2024
1 parent 2050886 commit 52f6ad3
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 33 deletions.
59 changes: 28 additions & 31 deletions test/e2e/tests/request-queuing/ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ async function openDappAndSwitchChain(
'[data-testid="confirmation-submit-button"]',
);
await driver.clickElement('[data-testid="confirmation-submit-button"]');

// Switch back to the dapp
await driver.switchToWindowWithUrl(dappUrl);
}
}

Expand All @@ -93,29 +96,23 @@ async function switchToNotificationPopoverValidateDetails(
const windowHandles = await driver.getAllWindowHandles();
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog, windowHandles);

// Get UI details
const networkPill = await driver.findElement(
// Differs between confirmation and signature
'[data-testid="network-display"], [data-testid="signature-request-network-display"]',
);
const networkText = await networkPill.getText();
const originElement = await driver.findElement(
// Differs between confirmation and signature
'.confirm-page-container-summary__origin bdi, .request-signature__origin .chip__label',
);
const originText = await originElement.getText();
await driver.findElement({
css: '[data-testid="network-display"], [data-testid="signature-request-network-display"]',
text: expectedDetails.networkText,
});

await driver.findElement({
css: '.confirm-page-container-summary__origin bdi, .request-signature__origin .chip__label',
text: expectedDetails.originText,
});

// Get state details
const notificationWindowState = await driver.executeScript(() =>
window.stateHooks?.getCleanAppState?.(),
);
const { chainId } = notificationWindowState.metamask.providerConfig;

// Ensure accuracy
validateConfirmationDetails(
{ networkText, originText, chainId },
expectedDetails,
);
const { chainId } = notificationWindowState.metamask.providerConfig;
assert.equal(chainId, expectedDetails.chainId);
}

async function rejectTransaction(driver) {
Expand All @@ -126,15 +123,6 @@ async function confirmTransaction(driver) {
await driver.clickElement({ tag: 'button', text: 'Confirm' });
}

function validateConfirmationDetails(
{ chainId, networkText, originText },
expected,
) {
assert.equal(chainId, expected.chainId);
assert.equal(networkText, expected.networkText);
assert.equal(originText, expected.originText);
}

async function switchToNetworkByName(driver, networkName) {
await driver.clickElement('[data-testid="network-display"]');
await driver.clickElement(`[data-testid="${networkName}"]`);
Expand Down Expand Up @@ -462,8 +450,8 @@ describe('Request-queue UI changes', function () {
],
},
// This test intentionally quits Ganache while the extension is using it, causing
// PollingBlockTracker errors. These are expected.
ignoredConsoleErrors: ['PollingBlockTracker'],
// PollingBlockTracker errors and others. These are expected.
ignoredConsoleErrors: ['ignore-all'],
dappOptions: { numberOfDapps: 2 },
title: this.test.fullTitle(),
},
Expand Down Expand Up @@ -494,7 +482,11 @@ describe('Request-queue UI changes', function () {

// Go back to first dapp, try an action, ensure network connection failure doesn't block UI
await selectDappClickPersonalSign(driver, DAPP_URL);
await driver.delay(veryLargeDelayMs);

// When the network is down, there is a performance degradation that causes the
// popup to take a few seconds to open in MV3 (issue #25690)
await driver.waitUntilXWindowHandles(4, 1000, 15000);

await switchToNotificationPopoverValidateDetails(driver, {
chainId: '0x539',
networkText: 'Localhost 8545',
Expand Down Expand Up @@ -528,8 +520,8 @@ describe('Request-queue UI changes', function () {
],
},
// This test intentionally quits Ganache while the extension is using it, causing
// PollingBlockTracker errors. These are expected.
ignoredConsoleErrors: ['PollingBlockTracker'],
// PollingBlockTracker errors and others. These are expected.
ignoredConsoleErrors: ['ignore-all'],
dappOptions: { numberOfDapps: 2 },
title: this.test.fullTitle(),
},
Expand Down Expand Up @@ -560,6 +552,11 @@ describe('Request-queue UI changes', function () {

// Go back to first dapp, try an action, ensure network connection failure doesn't block UI
await selectDappClickSend(driver, DAPP_URL);

// When the network is down, there is a performance degradation that causes the
// popup to take a few seconds to open in MV3 (issue #25690)
await driver.waitUntilXWindowHandles(4, 1000, 15000);

await switchToNotificationPopoverValidateDetails(driver, {
chainId: '0x539',
networkText: 'Localhost 8545',
Expand Down
6 changes: 4 additions & 2 deletions test/e2e/webdriver/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,8 @@ class Driver {
}

async checkBrowserForConsoleErrors(_ignoredConsoleErrors) {
const ignoreAllErrors = _ignoredConsoleErrors.includes('ignore-all');

const ignoredConsoleErrors = _ignoredConsoleErrors.concat([
// Third-party Favicon 404s show up as errors
'favicon.ico - Failed to load resource: the server responded with a status of 404',
Expand All @@ -1151,15 +1153,15 @@ class Driver {
eventDescription?.description,
);

if (!ignored) {
if (!ignored && !ignoreAllErrors) {
this.errors.push(eventDescription?.description);
}
} else if (event.args.length !== 0) {
const newError = this.#getErrorFromEvent(event);

const ignored = logBrowserError(ignoredConsoleErrors, newError);

if (!ignored) {
if (!ignored && !ignoreAllErrors) {
this.errors.push(newError);
}
}
Expand Down

0 comments on commit 52f6ad3

Please sign in to comment.