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

[e2e] Using ganache requests to getBalance and getAccounts #18215

Merged
merged 4 commits into from
Mar 20, 2023

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Mar 17, 2023

Explanation

Context: sometimes we need to validate tests against the current ETH balance or the account addresst. We used to hardcode this values.

What: with this PR we introduce the Ganache methods for getting the accounts and getting the balance of the account 0, so we do not need to hardcode balances/accounts anymore. Benefits:

  • If we add more transactions in the previous steps, we won't have to update the balance manually
  • We are indirectly adding more test coverage, as we are making sure that what we receive from the node is correctly displayed on the UI
  • This work can unlock potential test scenarios using other Ganache methods like setAccountNonceetc.

Screenshots/Screencaps

n/a

Before

We look for this specific balance value on the UI:
text: '24.9977 ETH',

After

We get the current balance value and look for that value on the UI:
const balance = await ganacheServer.getBalance();
text: ${balance} ETH,

Manual Testing Steps

Run the affected tests manually on your local:

  • yarn test:e2e:single test/e2e/tests/send-hex-address.spec.js --browser=chrome --leave-running=true
  • yarn test:e2e:single test/e2e/tests/metamask-responsive-ui.spec.js --browser=chrome --leave-running=true

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@seaona seaona requested a review from a team as a code owner March 17, 2023 10:52
@seaona seaona requested a review from hmalik88 March 17, 2023 10:52
@github-actions
Copy link
Contributor

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.

const balanceInt = parseInt(balanceHex, 16) / 10 ** 18;

const balanceFormatted =
balanceInt % 1 === 0 ? balanceInt : balanceInt.toFixed(4);
Copy link
Contributor Author

@seaona seaona Mar 17, 2023

Choose a reason for hiding this comment

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

If the balance is a float number, we round it up to 4 decimals to match UI.
If it's a whole number, do nothing

@@ -145,6 +145,7 @@ async function withFixtures(options, testSuite) {
driver: driverProxy ?? driver,
mockServer,
contractRegistry,
ganacheServer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we pass ganacheServer to the testsuite to be able to access its methods

@@ -104,9 +104,10 @@ describe('MetaMask Responsive UI', function () {
await driver.press('#confirm-password', driver.Key.ENTER);

// balance renders
const balance = await ganacheServer.getBalance();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we access the current balance of the acocunt, instead of hardcoding the balance

@metamaskbot
Copy link
Collaborator

Builds ready [b2e3f70]
Page Load Metrics (1628 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96175119199
domContentLoaded1468178716098943
load1469178716288440
domInteractive1468178716098943
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@seaona seaona changed the title [e2e] Using ganache requests to get balances [e2e] Using ganache requests to getBalance and getAccounts Mar 17, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [775498e]
Page Load Metrics (1718 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96148126157
domContentLoaded1572185517008440
load1591196117189345
domInteractive1572185517008440
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #18215 (775498e) into develop (c21c2bd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop   #18215   +/-   ##
========================================
  Coverage    63.96%   63.96%           
========================================
  Files          914      914           
  Lines        35623    35623           
  Branches      9030     9030           
========================================
  Hits         22784    22784           
  Misses       12839    12839           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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! Great idea

@seaona seaona merged commit c940f74 into develop Mar 20, 2023
@seaona seaona deleted the e2e-ganache-requests branch March 20, 2023 09:29
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2023
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.

4 participants