Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flaky Test: "Send ETH from inside MetaMask finds the transaction in the transactions list when sending to a Multisig Address"? #24574

Closed
seaona opened this issue May 17, 2024 · 1 comment · Fixed by #24639
Assignees
Labels
flaky tests INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-11.18.0 Issue or pull request that will be included in release 11.18.0 team-extension-platform

Comments

@seaona
Copy link
Contributor

seaona commented May 17, 2024

Failure

https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/80402/workflows/41cbd58d-f379-4fb3-98a3-56004e8b8872/jobs/2836380/tests

Screenrecording

gas0.mp4

Gas Limit is set to 0 and there is no way the gas values are updated, even after subsequent successful gas api calls (mocked responses)

Screenshot from 2024-05-20 15-41-32

@seaona seaona self-assigned this May 17, 2024
@metamaskbot metamaskbot added the INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. label May 17, 2024
@seaona
Copy link
Contributor Author

seaona commented May 17, 2024

Findings

This specific failure is due to a race condition where the balance for the account is not loaded and we try to proceed with a transaction (this causes the gasLimit be set to 0 --> this seems a separate bug).

Other Findings

Cannot proceed with the tx

Whenever I am on a regular build, I see that the gas is set to 0 and cannot proceed with the transaction.
Repro:

  1. Select Sepolia netowrk
  2. Start a tx from inside the wallet to 0xB258Ee3d68e5A06a69f49786B29bE940DdC246B3
cannot-proceed-send-contract.mp4

This is similar to what we see in the test failure artifacts where we cannot proceed with the last Confirm screen in this case, and gas fee is set to 0.

image

Unable to Determine Contract Standard

Whenever I am on a Multichain Build, if I try to Send ETH to a contract, I get the error Unable to determine Contract Standard.
I think this is a bug

unable-to-determine-contract-standard.mp4

Contract Interaction

Whenever we are sending ETH to a contract address, the Confirmations page displays it as a Contract Interaction. Is this expected?
@bschorchit
[Update] I think this is expected, since even just a send ETH can trigger a function execution in the smart contract (fallback or payable receive)

image

seaona added a commit that referenced this issue May 21, 2024
…tion` (#24639)

## **Description**
The problem with this flaky test is that in some occurrences we are
proceeding to the last Confirmation screen without the balance being
loaded (race condition). This can be observed at second 4 from this
video. This test fails every time this condition is met (very often in
Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding
with the actions.

However, I think we should also revise the application behaviour, as it
does not seem correct that:
- whenever we proceed to the last screen without the balance being
loaded, the gas is set to 0 (specifically the gas limit)
- then, no matter how long we wait, the gas cannot be recovered from
this and we are stucked in the confirmation screen (Confirm is disabled)
--> this happens despite the balance being then loaded and also getting
numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests



https://github.com/MetaMask/metamask-extension/assets/54408225/93b9bd5d-a9d2-4898-b743-8a59eed0eea2



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

## **Related issues**

Fixes: #24574

## **Manual testing steps**

1. Check ci jobs
2. Run test multiple times locally `yarn test:e2e:single
test/e2e/tests/transaction/send-eth.spec.js --browser=chrome
--leave-running=true --retry-until-failure --retries=15`

## **Screenshots/Recordings**
In the test failing artifacts we can see how we cannot proceed with the
transaction as the Confirm button is disabled. However the problem is
that the gas is set to 0 and never recovered, but the real root cause
comes from proceeding with the previous screen without the balance
loaded.


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/132725c0-91cf-4e67-a888-c4a0c9431cc2)


## **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 metamaskbot added the release-11.18.0 Issue or pull request that will be included in release 11.18.0 label May 21, 2024
danjm pushed a commit that referenced this issue May 31, 2024
…tion` (#24639)

## **Description**
The problem with this flaky test is that in some occurrences we are
proceeding to the last Confirmation screen without the balance being
loaded (race condition). This can be observed at second 4 from this
video. This test fails every time this condition is met (very often in
Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding
with the actions.

However, I think we should also revise the application behaviour, as it
does not seem correct that:
- whenever we proceed to the last screen without the balance being
loaded, the gas is set to 0 (specifically the gas limit)
- then, no matter how long we wait, the gas cannot be recovered from
this and we are stucked in the confirmation screen (Confirm is disabled)
--> this happens despite the balance being then loaded and also getting
numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests



https://github.com/MetaMask/metamask-extension/assets/54408225/93b9bd5d-a9d2-4898-b743-8a59eed0eea2



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

## **Related issues**

Fixes: #24574

## **Manual testing steps**

1. Check ci jobs
2. Run test multiple times locally `yarn test:e2e:single
test/e2e/tests/transaction/send-eth.spec.js --browser=chrome
--leave-running=true --retry-until-failure --retries=15`

## **Screenshots/Recordings**
In the test failing artifacts we can see how we cannot proceed with the
transaction as the Confirm button is disabled. However the problem is
that the gas is set to 0 and never recovered, but the real root cause
comes from proceeding with the previous screen without the balance
loaded.


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/132725c0-91cf-4e67-a888-c4a0c9431cc2)


## **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
…tion` (#24639)

## **Description**
The problem with this flaky test is that in some occurrences we are
proceeding to the last Confirmation screen without the balance being
loaded (race condition). This can be observed at second 4 from this
video. This test fails every time this condition is met (very often in
Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding
with the actions.

However, I think we should also revise the application behaviour, as it
does not seem correct that:
- whenever we proceed to the last screen without the balance being
loaded, the gas is set to 0 (specifically the gas limit)
- then, no matter how long we wait, the gas cannot be recovered from
this and we are stucked in the confirmation screen (Confirm is disabled)
--> this happens despite the balance being then loaded and also getting
numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests



https://github.com/MetaMask/metamask-extension/assets/54408225/93b9bd5d-a9d2-4898-b743-8a59eed0eea2



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

## **Related issues**

Fixes: #24574

## **Manual testing steps**

1. Check ci jobs
2. Run test multiple times locally `yarn test:e2e:single
test/e2e/tests/transaction/send-eth.spec.js --browser=chrome
--leave-running=true --retry-until-failure --retries=15`

## **Screenshots/Recordings**
In the test failing artifacts we can see how we cannot proceed with the
transaction as the Confirm button is disabled. However the problem is
that the gas is set to 0 and never recovered, but the real root cause
comes from proceeding with the previous screen without the balance
loaded.


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/132725c0-91cf-4e67-a888-c4a0c9431cc2)


## **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
…tion` (#24639)

## **Description**
The problem with this flaky test is that in some occurrences we are
proceeding to the last Confirmation screen without the balance being
loaded (race condition). This can be observed at second 4 from this
video. This test fails every time this condition is met (very often in
Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding
with the actions.

However, I think we should also revise the application behaviour, as it
does not seem correct that:
- whenever we proceed to the last screen without the balance being
loaded, the gas is set to 0 (specifically the gas limit)
- then, no matter how long we wait, the gas cannot be recovered from
this and we are stucked in the confirmation screen (Confirm is disabled)
--> this happens despite the balance being then loaded and also getting
numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests



https://github.com/MetaMask/metamask-extension/assets/54408225/93b9bd5d-a9d2-4898-b743-8a59eed0eea2



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

## **Related issues**

Fixes: #24574

## **Manual testing steps**

1. Check ci jobs
2. Run test multiple times locally `yarn test:e2e:single
test/e2e/tests/transaction/send-eth.spec.js --browser=chrome
--leave-running=true --retry-until-failure --retries=15`

## **Screenshots/Recordings**
In the test failing artifacts we can see how we cannot proceed with the
transaction as the Confirm button is disabled. However the problem is
that the gas is set to 0 and never recovered, but the real root cause
comes from proceeding with the previous screen without the balance
loaded.


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/132725c0-91cf-4e67-a888-c4a0c9431cc2)


## **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
…tion` (#24639)

## **Description**
The problem with this flaky test is that in some occurrences we are
proceeding to the last Confirmation screen without the balance being
loaded (race condition). This can be observed at second 4 from this
video. This test fails every time this condition is met (very often in
Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding
with the actions.

However, I think we should also revise the application behaviour, as it
does not seem correct that:
- whenever we proceed to the last screen without the balance being
loaded, the gas is set to 0 (specifically the gas limit)
- then, no matter how long we wait, the gas cannot be recovered from
this and we are stucked in the confirmation screen (Confirm is disabled)
--> this happens despite the balance being then loaded and also getting
numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests



https://github.com/MetaMask/metamask-extension/assets/54408225/93b9bd5d-a9d2-4898-b743-8a59eed0eea2



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

## **Related issues**

Fixes: #24574

## **Manual testing steps**

1. Check ci jobs
2. Run test multiple times locally `yarn test:e2e:single
test/e2e/tests/transaction/send-eth.spec.js --browser=chrome
--leave-running=true --retry-until-failure --retries=15`

## **Screenshots/Recordings**
In the test failing artifacts we can see how we cannot proceed with the
transaction as the Confirm button is disabled. However the problem is
that the gas is set to 0 and never recovered, but the real root cause
comes from proceeding with the previous screen without the balance
loaded.


![image](https://github.com/MetaMask/metamask-extension/assets/54408225/132725c0-91cf-4e67-a888-c4a0c9431cc2)


## **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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky tests INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-11.18.0 Issue or pull request that will be included in release 11.18.0 team-extension-platform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants