Flaky Test: Multiple tests failing with error "No window with title: MetaMask Dialog" #24603
Closed
5 of 15 tasks
Labels
flaky tests
release-11.16.6
Issue or pull request that will be included in release 11.16.6
release-12.0.0
Issue or pull request that will be included in release 12.0.0
team-extension-platform
Comments
chloeYue
changed the title
Investigation IncreaseTokenAllowanceThe IncreaseTokenAllowance test fails because whenever we trigger the transferFrom, we get this error
The reason for that is because the previous IncreaseAllowance call failed, so the Account 2 doesn't have enough allowance to proceed with the transfer from
Investigation Request Queuing for Multiple Dapps and TxsThe window count get's messed up and we wait for 5 windows when we should wait for 4
|
7 tasks
58 tasks
7 tasks
seaona
added a commit
that referenced
this issue
May 23, 2024
…ng cap to allow other accounts to transfer tokens @no-mmi` (#24734) ## **Description** This PR fixes the flaky test `Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi).` The problem with this flaky test is that we are changing the allowance amount (from 0 to 1) and we confirm the transaction before the gas has been updated. This makes that the increase token allowance transaction fails, and the subsequent transaction with the Account 2 cannot be performed as it doesn't have the allowance for doing a transfer from action. So the dialog window cannot be found, as the popup is never triggered (see dapp error). The fix is to simply await for the expected gas for the increaseAllowance with amount 1. This can be clearly seen when you check the tx history from Account 1: - Here is a successful increaseAllowance with amount 1, and the total gas is `31000` -- correct  - Here is a failed increaseAllowance with amount 1, and the total gas limit is `26776` -- incorrect (this value corresponds to the increaseAllowance with amount 0)  [](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1) ## **Related issues** Fixes: one of this cases, but not all of them, as the rest of testcases need to be looked separately #24603 ## **Manual testing steps** 1. Run this test locally multiple times 2. Check ci jobs `yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** https://github.com/MetaMask/metamask-extension/assets/54408225/c0d32e57-6ace-4c59-a667-dcafc3421a7c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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
added a commit
that referenced
this issue
May 23, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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
added a commit
that referenced
this issue
May 24, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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
added a commit
that referenced
this issue
May 28, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1){kind=link}
…ng cap to allow other accounts to transfer tokens @no-mmi` (#24734) ## **Description** This PR fixes the flaky test `Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi).` The problem with this flaky test is that we are changing the allowance amount (from 0 to 1) and we confirm the transaction before the gas has been updated. This makes that the increase token allowance transaction fails, and the subsequent transaction with the Account 2 cannot be performed as it doesn't have the allowance for doing a transfer from action. So the dialog window cannot be found, as the popup is never triggered (see dapp error). The fix is to simply await for the expected gas for the increaseAllowance with amount 1. This can be clearly seen when you check the tx history from Account 1: - Here is a successful increaseAllowance with amount 1, and the total gas is `31000` -- correct  - Here is a failed increaseAllowance with amount 1, and the total gas limit is `26776` -- incorrect (this value corresponds to the increaseAllowance with amount 0)  [](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1) ## **Related issues** Fixes: one of this cases, but not all of them, as the rest of testcases need to be looked separately #24603 ## **Manual testing steps** 1. Run this test locally multiple times 2. Check ci jobs `yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** https://github.com/MetaMask/metamask-extension/assets/54408225/c0d32e57-6ace-4c59-a667-dcafc3421a7c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…ng cap to allow other accounts to transfer tokens @no-mmi` (#24734) ## **Description** This PR fixes the flaky test `Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi).` The problem with this flaky test is that we are changing the allowance amount (from 0 to 1) and we confirm the transaction before the gas has been updated. This makes that the increase token allowance transaction fails, and the subsequent transaction with the Account 2 cannot be performed as it doesn't have the allowance for doing a transfer from action. So the dialog window cannot be found, as the popup is never triggered (see dapp error). The fix is to simply await for the expected gas for the increaseAllowance with amount 1. This can be clearly seen when you check the tx history from Account 1: - Here is a successful increaseAllowance with amount 1, and the total gas is `31000` -- correct  - Here is a failed increaseAllowance with amount 1, and the total gas limit is `26776` -- incorrect (this value corresponds to the increaseAllowance with amount 0)  [](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1) ## **Related issues** Fixes: one of this cases, but not all of them, as the rest of testcases need to be looked separately #24603 ## **Manual testing steps** 1. Run this test locally multiple times 2. Check ci jobs `yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** https://github.com/MetaMask/metamask-extension/assets/54408225/c0d32e57-6ace-4c59-a667-dcafc3421a7c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…ng cap to allow other accounts to transfer tokens @no-mmi` (#24734) ## **Description** This PR fixes the flaky test `Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi).` The problem with this flaky test is that we are changing the allowance amount (from 0 to 1) and we confirm the transaction before the gas has been updated. This makes that the increase token allowance transaction fails, and the subsequent transaction with the Account 2 cannot be performed as it doesn't have the allowance for doing a transfer from action. So the dialog window cannot be found, as the popup is never triggered (see dapp error). The fix is to simply await for the expected gas for the increaseAllowance with amount 1. This can be clearly seen when you check the tx history from Account 1: - Here is a successful increaseAllowance with amount 1, and the total gas is `31000` -- correct  - Here is a failed increaseAllowance with amount 1, and the total gas limit is `26776` -- incorrect (this value corresponds to the increaseAllowance with amount 0)  [](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1) ## **Related issues** Fixes: one of this cases, but not all of them, as the rest of testcases need to be looked separately #24603 ## **Manual testing steps** 1. Run this test locally multiple times 2. Check ci jobs `yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** https://github.com/MetaMask/metamask-extension/assets/54408225/c0d32e57-6ace-4c59-a667-dcafc3421a7c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…ng cap to allow other accounts to transfer tokens @no-mmi` (#24734) ## **Description** This PR fixes the flaky test `Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi).` The problem with this flaky test is that we are changing the allowance amount (from 0 to 1) and we confirm the transaction before the gas has been updated. This makes that the increase token allowance transaction fails, and the subsequent transaction with the Account 2 cannot be performed as it doesn't have the allowance for doing a transfer from action. So the dialog window cannot be found, as the popup is never triggered (see dapp error). The fix is to simply await for the expected gas for the increaseAllowance with amount 1. This can be clearly seen when you check the tx history from Account 1: - Here is a successful increaseAllowance with amount 1, and the total gas is `31000` -- correct  - Here is a failed increaseAllowance with amount 1, and the total gas limit is `26776` -- incorrect (this value corresponds to the increaseAllowance with amount 0)  [](https://codespaces.new/MetaMask/metamask-extension/pull/24734?quickstart=1) ## **Related issues** Fixes: one of this cases, but not all of them, as the rest of testcases need to be looked separately #24603 ## **Manual testing steps** 1. Run this test locally multiple times 2. Check ci jobs `yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** https://github.com/MetaMask/metamask-extension/assets/54408225/c0d32e57-6ace-4c59-a667-dcafc3421a7c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
May 31, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
Jun 4, 2024
…s up on Token Approval transaction confirmations` (#24739) ## **Description** This PR attempts to fix the flaky test `Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations` This test fails whenever we click Create Token for deploying an ERC20 token, as the popup is not triggered and we remain in the test dapp. I haven't been able to reproduce this failure locally. However, after looking into the code and the context of flakiness, I can confirm that: - I've seen that we are doing several unnecessary steps for the test setup, like connecting to the test dapp, or deploying an ERC20 contract. This can be achieved by fixtures, so I've added the fixtures and got rid of a good chunk of the code. The good part is that the test failure precisely happens on the test setup, which is now removed - It might be that there is a race condition between the Connect to the test Dapp and the Create Token steps, where the dapp has not yet the connected address for triggering the Create Token but the button is already enabled, resulting in the error in the test dapp `unknown account` and popup not being opened (see video below). However, those 2 steps are now removed from the test, meaning any potential race condition at this point won't happen anymore - I've added the `logInWithBalanceValidation` instead of the `unlockWallet` to make sure the balance is loaded before triggering the interaction For the above points, I have strong reasons for wanting to implement the changes on this PR, despite not being 100% sure of the above being the root cause. [](https://codespaces.new/MetaMask/metamask-extension/pull/24739?quickstart=1) ## **Related issues** Fixes: another item of this list #24603 ## **Manual testing steps** 1. Run this test locally several times `yarn test:e2e:single test/e2e/tests/ppom/migrate-opensea-to-blockaid-banner.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10 2. Check ci jobs ## **Screenshots/Recordings**   This is not the test environment, however here I demonstrate how would look like if the create token is triggered before the dapp has the connected account resolved https://github.com/MetaMask/metamask-extension/assets/54408225/1121af80-bb95-4201-86db-ff37bb99c3e7 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
Jun 4, 2024
…different networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24741) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls `. The problem is that sometimes when we call the `const currentWindowHandles = await driver.getAllWindowHandles` there is still the previous popup open, meaning that we get the total count of 4 windows. Right after that, we trigger a send `await driver.clickElement('#sendButton');` and now we wait for the windowHandles to be `currentWindowHandles + 1` meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total. ``` const currentWindowHandles = await driver.getAllWindowHandles(); await driver.clickElement('#sendButton'); const newWindowHandles = await driver.waitUntilXWindowHandles( process.env.ENABLE_MV3 ? currentWindowHandles.length : currentWindowHandles.length + 1, ``` The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches, - wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔 - add a delay ## **Related issues** Fixes: the third item of this list #24603 ## **Manual testing steps** 1. Run the test several times `yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` 2. Check ci jobs ## **Screenshots/Recordings** See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen. https://github.com/MetaMask/metamask-extension/assets/54408225/a3eb7f74-cf2c-43ba-a5bb-0835544b448c ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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.
danjm
pushed a commit
that referenced
this issue
Jun 4, 2024
…fferent networks. should switch to the dapps network automatically when handling sendTransaction calls` (#24809) ## **Description** This PR fixes the flaky test `Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls`. - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/82995/workflows/51b18b16-f786-4ee8-96d0-9c751af6fa17/jobs/2965502/tests The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails. The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.  **Circle ci Screenshot**: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.  [](https://codespaces.new/MetaMask/metamask-extension/pull/24809?quickstart=1) ## **Related issues** Fixes: #24603 ## **Manual testing steps** 1. Check ci jobs 2. Run test multiple times `yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs` ## **Screenshots/Recordings** Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page https://github.com/MetaMask/metamask-extension/assets/54408225/726911da-a044-48eb-a60f-938099db1176 Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video) https://github.com/MetaMask/metamask-extension/assets/54408225/a757587d-8bfa-4ef4-acc4-78f2489daa83 <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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
added
the
release-11.16.6
|
Missing release label release-11.16.6 on issue. Adding release label release-11.16.6 on issue, as issue is linked to PR #24809 which has this release label. |
gauthierpetetin
added
the
release-12.0.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is this about?
Related tests:
Migrate Opensea to Blockaid Banner @no-mmi Shows up on Token Approval transaction confirmations#24739Increase Token Allowance increases token spending cap to allow other accounts to transfer tokens @no-mmi#24734Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls#24741Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls#24809Failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81005/workflows/cb406794-a062-4775-b288-2e72b27ce5c6/jobs/2867466/tests
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81133/workflows/0b962d69-e9d4-4c3e-9ca0-cfa829b667e2/jobs/2874110/tests
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81133/workflows/0b962d69-e9d4-4c3e-9ca0-cfa829b667e2/jobs/2874111/tests
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81194/workflows/10f49a13-e57c-416a-8c6c-d4923a8dce45/jobs/2877421/tests
Error message
Screenshot
Scenario
No response
Design
No response
Technical Details
No response
Threat Modeling Framework
No response
Acceptance Criteria
No response
Stakeholder review needed before the work gets merged
References
No response
The text was updated successfully, but these errors were encountered: