Skip to content

Refactoring metamask-responsive-ui.spec.js to use fixtures #10866

Merged
NiranjanaBinoy merged 9 commits intodevelopfrom
e2e-fixtures-responsive-ui
Apr 15, 2021
Merged

Refactoring metamask-responsive-ui.spec.js to use fixtures #10866
NiranjanaBinoy merged 9 commits intodevelopfrom
e2e-fixtures-responsive-ui

Conversation

@NiranjanaBinoy
Copy link
Contributor

Fixes: #10863

Explanation:

  • Moved the file to test/e2e/tests
  • Updated metamask-responsive-ui.spec.js to use fixtures.
  • Updated run-all.sh to use the new test file.
  • Deleted old files

@NiranjanaBinoy NiranjanaBinoy requested a review from Gudahtt April 9, 2021 17:52
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner April 9, 2021 17:52
@NiranjanaBinoy NiranjanaBinoy self-assigned this Apr 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

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.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be split up into 3 different tests? It seems to be testing 3 distinct things: The onboarding 'create wallet' flow, the import from lock screen flow, and the send flow. Except all with a reduced screen size.

@metamaskbot
Copy link
Collaborator

Builds ready [6170778]
Page Load Metrics (650 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint478064105
domContentLoaded5457626495125
load5467636505125
domInteractive5457616485125

@NiranjanaBinoy NiranjanaBinoy force-pushed the e2e-fixtures-responsive-ui branch from 6170778 to caee9bb Compare April 13, 2021 19:09
@metamaskbot
Copy link
Collaborator

Builds ready [caee9bb]
Page Load Metrics (562 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint439353115
domContentLoaded3426535608943
load3446545628742
domInteractive3426535608943

@metamaskbot
Copy link
Collaborator

Builds ready [711111e]
Page Load Metrics (792 ± 111 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint53140782311
domContentLoaded4801305790232111
load4821306792232111
domInteractive4801304789232111

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Couple of nits but nothing important really. Unfortunately another conflict has been introduced though, as a result of #10852. The web driver refactoring is finished now though so that should be the last time this happens thankfully!

);
});

it('Importing existing wallet from login page', async function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we've been calling this the "lock" page (unlock/lock), instead of the "login" page

Suggested change
it('Importing existing wallet from login page', async function () {
it('Importing existing wallet from lock page', async function () {


// Show account information
// show account details dropdown menu
await driver.clickElement(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'm not sure how useful this part is, or the lock step at the end 🤔 I guess we need to do something after the account is created, to verify that it worked. Checking the balance would suffice though - that's what we do in the other flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought while working on it, then decided to update it based on the review comments 😅

await driver.wait(until.elementTextMatches(balance, /100\s*ETH/u));

// logs out of the vault
await driver.clickElement('.account-menu__icon');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This step of logging out of the vault doesn't seem necessary for testing that the onboarding worked.

@NiranjanaBinoy NiranjanaBinoy force-pushed the e2e-fixtures-responsive-ui branch from b09c400 to 019189e Compare April 14, 2021 15:20
@NiranjanaBinoy NiranjanaBinoy force-pushed the e2e-fixtures-responsive-ui branch from 93e9dfb to 8ae170b Compare April 14, 2021 20:22
@NiranjanaBinoy NiranjanaBinoy force-pushed the e2e-fixtures-responsive-ui branch from 1ec3bcf to 09ad199 Compare April 14, 2021 21:40
@metamaskbot
Copy link
Collaborator

Builds ready [09ad199]
Page Load Metrics (583 ± 34 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43725574
domContentLoaded3586625817034
load3596635837034
domInteractive3576625817034

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@NiranjanaBinoy NiranjanaBinoy merged commit 0974709 into develop Apr 15, 2021
@NiranjanaBinoy NiranjanaBinoy deleted the e2e-fixtures-responsive-ui branch April 15, 2021 13:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor metamask-responsive-ui.spec.js to follow pattern in metamask-extension/test/e2e/tests

3 participants