fix: Fixed flaky test related to change language spec

[hjetpoluru]

Description

The test was initially written to change the language in the settings. Due to the dropdown not closing properly, the line await driver.navigate() was used to refresh the page. However, with recent changes in navigation, this step now appears to cause a driver/browser error.

To resolve this issue, instead of refreshing the page it is more logical to close the settings page by clicking the close button after changing the language. Upon @chloeYue suggestion by replacing the line await driver.navigate() with await driver.clickElementAndWaitToDisappear('.settings-page__header__title-container__close-button') solves the flaky test.

Related issues

Fixes:
#27390

Manual testing steps

Run the change language spec locally or in codespace using below commands against chrome browser:
yarn
yarn build:test:webpack
ENABLE_MV3=false yarn test:e2e:single test/e2e/tests/settings/change-language.spec.ts --browser=chrome

Pre-merge author checklist

• I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
• 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 format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). 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.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[seaona]

we should also get rid of the driver.refresh() from here

[hjetpoluru]

Thanks @seaona for the feedback. The test scenario involves refreshing the page to ensure that the language change is retained. Hence the refresh is required. However the current title of the test scenario is misleading and does not clearly represent this purpose. I can update the title to better reflect the scenario or do you want me to remove the scenario.

Also I see in the CI its failing due to the reason about the Error: failed to fetch es locale because of TypeError: Failed to fetch. I think we should fix this issue too.

[seaona]

Thanks @seaona for the feedback. The test scenario involves refreshing the page to ensure that the language change is retained. Hence the refresh is required. However the current title of the test scenario is misleading and does not clearly represent this purpose. I can update the title to better reflect the scenario or do you want me to remove the scenario.

oh I see. So the test is currently failing on ci at the refresh point, for the same reason that it was failing on navigate. If this refresh is part of the test, I think we'll need to await until at least one word has changed to the new language, before refreshing. This can mitigate the race condition below

Let me know if this makes sense!

[hjetpoluru]

Thanks @seaona for the feedback. The test scenario involves refreshing the page to ensure that the language change is retained. Hence the refresh is required. However the current title of the test scenario is misleading and does not clearly represent this purpose. I can update the title to better reflect the scenario or do you want me to remove the scenario.

oh I see. So the test is currently failing on ci at the refresh point, for the same reason that it was failing on navigate. If this refresh is part of the test, I think we'll need to await until at least one word has changed to the new language, before refreshing. This can mitigate the race condition below

Let me know if this makes sense!

I think we both agree on the CI failures, but we are considering different approaches to fix them.

• My approach was to address the type error itself.
• I did not fully understand the statement, "we'll need to await until at least one word has changed to the new language." Could you please clarify this? Currently, the test checks the language and then performs a page refresh.

[seaona]

Despite changing the language and checking Idioma actual is there, the language hasn't fully changed until the loading disappears, ie when the language is set to Español. If you do a refresh without waiting for the full language to be changed and loaded into the wallet, we run into the error above.

You can observe this in the video -- Idioma actual is present, but we are still at English language and a loading spinner happens:

language-loading.mp4

[hjetpoluru]

@seaona, Thank you. I see it clearly now with the video you have posted. I appreciate your patience in explaining this to me. I will add a wait for the text to be fully loaded before performing the refresh.

[hjetpoluru]

@seaona, I have pushed the changes you suggested. Please take a look at your convenience. Locally, everything has been working fine, and I will rerun the tests again the CI to verify.

[metamaskbot]

Builds ready [df9b75e]

• builds: chrome, firefox
• builds (beta): chrome
• builds (flask): chrome, firefox
• builds (MMI): chrome, firefox
• builds (test): chrome, firefox
• builds (test-flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5, 6
  • common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
  • content-script: 0
  • offscreen: 0
  • ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1678 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	314	2037	1607	321	154
		domContentLoaded	1451	1892	1640	104	50
		load	1459	1976	1678	113	54
		domInteractive	15	162	46	38	18

Bundle size diffs

• background: 0 Bytes (0.00%)
• ui: 0 Bytes (0.00%)
• common: 0 Bytes (0.00%)

[seaona]

hi @hjetpoluru my suggestion is to add this conditions before doing the refresh. Otherwise we'd still can run into the race condition when refreshing. Let me know if this makes sense 🙏

[hjetpoluru]

Oops! I thought it was needed after the refresh and before clicking on the dropdown again, cause the overlay appears.

@seaona reading this comment, I feel it would be better to include it both before(just wait for overlay) and after(overlay is gone and the label is visible). Please let me know.

[hjetpoluru]

   await changeLanguage(driver, languageIndex);
   await driver.assertElementNotPresent('.loading-overlay__spinner');

    // Validate the label changes to Spanish
    const isLanguageLabelChanged = await driver.isElementPresent(
      selectors.labelSpanish,
    );
    assert.equal(isLanguageLabelChanged, true, 'Language did not change');

    await driver.refresh();

    await driver.assertElementNotPresent('.loading-overlay__spinner');
    await driver.findVisibleElement(selectors.labelSpanish);

[seaona]

yeah I think it's okay. I would keep the labelSpanish before the refresh too, since this needs to happen, to ensure we don't refresh before fetching the language change.

We do have a validation that the language changed below, so I think we can remove the labelSpanish after the refresh (mainly doing the same thing)

   await changeLanguage(driver, languageIndex);
   await driver.assertElementNotPresent('.loading-overlay__spinner');
   await driver.waitForSelector(selectors.labelSpanish);

    // Validate the label changes to Spanish
    const isLanguageLabelChanged = await driver.isElementPresent(
      selectors.labelSpanish,
    );
    assert.equal(isLanguageLabelChanged, true, 'Language did not change');

    await driver.refresh();

    await driver.assertElementNotPresent('.loading-overlay__spinner');

let me know if this makes sense!

[hjetpoluru]

Thanks @seaona for guiding me through this change. That makes sense and I will make the change and push up.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[hjetpoluru]

After getting the latest develop updates, the test failed on this scenario, which is the intended change. Hence, I am converting this PR as draft for further investigation.

Thanks @seaona and @HowardBraham for reviewing this PR and if there is any suggestion please do let me know.