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: Multiple onboarding tests failing due to an error where the "Reveal secret recovery phrase" button cannot be found #24608

Closed
9 tasks
chloeYue opened this issue May 20, 2024 · 3 comments · Fixed by #24874
Assignees
Labels
flaky tests release-11.16.6 Issue or pull request that will be included in release 11.16.6 release-12.0.0 team-extension-platform

Comments

@chloeYue
Copy link
Contributor

chloeYue commented May 20, 2024

What is this about?

Reason of flakiness:
We are trying to find button on wrong screen! Need to check why we are not on expected screen.

Related tests:
Wallet Created Events @no-mmi are sent when onboarding user who chooses to opt in metrics
Wallet Created Events @no-mmi are not sent when onboarding user who chooses to opt out metrics

Failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81114/workflows/d532e9e3-0401-4b01-aae7-1682303bc3fe/jobs/2873090/tests
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81133/workflows/0b962d69-e9d4-4c3e-9ca0-cfa829b667e2/jobs/2874110/tests

Error message
Screenshot 2024-05-20 at 12 08 53

Screenshot 2024-05-20 at 12 15 50

Screenshot
Screenshot 2024-05-20 at 12 09 07

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

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

No response

@chloeYue chloeYue changed the title Flaky Test: "Wallet Created Events @no-mmi are sent when onboarding user who chooses to opt in metrics" Flaky Test: Multiple onboarding tests failing due to an error where the "Reveal secret recovery phrase" button cannot be found May 20, 2024
@seaona seaona self-assigned this May 21, 2024
@seaona
Copy link
Contributor

seaona commented May 21, 2024

Findings

For one type of Onboarding failure, this seems to be the error logs.

[0521/181627.165843:ERROR:file_io_posix.cc(145)] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq: No such file or directory (2)
[0521/181627.165925:ERROR:file_io_posix.cc(145)] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq: No such file or directory (2)

When this error is hit, we stay in the first page and nothing happens. This corresponds to the following failed tests:

  • MetaMask onboarding @no-mmi Clicks import a new wallet, accepts a secure password, reveals the Secret Recovery Phrase, confirm SRP
  • MetaMask onboarding @no-mmi Verify that the user has been redirected to the correct page after creating a password for their new wallet
    MetaMask onboarding @no-mmi Verify that the user has been redirected to the correct page after creating a password for their new wallet
  • MetaMask onboarding @no-mmi Verify that the user has been redirected to the correct page after importing their wallet

https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81546/workflows/df92714b-7157-4126-8b72-0634e69ddf8e/jobs/2896296/artifacts

image

@danjm
Copy link
Contributor

danjm commented May 29, 2024

Re-opening as we discovered the problem was not resolved when we thought it was

danjm added a commit that referenced this issue May 29, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue May 31, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue May 31, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue May 31, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue May 31, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue May 31, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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 added a commit that referenced this issue Jun 4, 2024
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

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

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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.16.6 Issue or pull request that will be included in release 11.16.6 label Jun 4, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.6 on issue. Adding release label release-11.16.6 on issue, as issue is linked to PR #24874 which has this release label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky tests release-11.16.6 Issue or pull request that will be included in release 11.16.6 release-12.0.0 team-extension-platform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants