[e2e] Refactor for custom token add approve action using Ganache seeder #15728
Conversation
|
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. |
Builds ready [7699dce]
Page Load Metrics (1970 ± 120 ms)
|
|
|
||
| describe.skip('Create token, approve token and approve token without gas', function () { | ||
| describe('Create token, approve token and approve token without gas', function () { | ||
| describe('Add a custom token from a dapp', function () { |
There was a problem hiding this comment.
I think if we use it blocks instead of nested describes, we could improve test structure and declare variables like smartContract, ganacheOptions only once before the it blocks (now repeated on line 13, 115, 322).
Something like:
describe('Create token, approve token and approve token without gas', function () {
const smartContract = ...
const ganacheOptions = ...
it('...')
it('...')
it('...')
You can take this example for more clarity.
| assert.equal(await recipientDiv.getText(), '0x2f318C33...C970'); | ||
|
|
||
| await driver.clickElement({ text: 'Confirm', tag: 'button' }); | ||
|
|
||
| await driver.waitForSelector({ | ||
| css: '.transaction-list__completed-transactions .transaction-list-item:first-child .list-item__heading', | ||
| text: 'Approve Token spend limit', |
There was a problem hiding this comment.
I think finishing each test case with an assertion is a good convention. It could be added in this last testcase
| @@ -37,8 +41,17 @@ describe.skip('Create token, approve token and approve token without gas', funct | |||
| await driver.waitForSelector({ text: 'Create Token', tag: 'button' }); | |||
There was a problem hiding this comment.
I think we can remove waitForSelector, as the below clickElement method will make sure to findClickableElement first
| @@ -49,7 +62,7 @@ describe.skip('Create token, approve token and approve token without gas', funct | |||
| await driver.clickElement({ text: 'Save', tag: 'button' }); | |||
There was a problem hiding this comment.
same here, we can remove the above line of waitForSelector, as the below clickElement method will make sure to findClickableElement first
| @@ -99,8 +112,7 @@ describe.skip('Create token, approve token and approve token without gas', funct | |||
| }); | |||
There was a problem hiding this comment.
on lines 83, 101 we can remove await driver.waitForSelector('...'); for same reason above
| const extension = windowHandles[0]; | ||
|
|
||
| await driver.findClickableElement('#deployButton'); | ||
| // approve token from dapp | ||
| await driver.waitForSelector({ |
There was a problem hiding this comment.
we can remove this await driver.waitForSelector('...'); for same reason above
|
|
||
| // approve token from dapp | ||
| await driver.waitForSelector({ |
There was a problem hiding this comment.
we can remove this await driver.waitForSelector('...'); for same reason above
| @@ -25,6 +27,8 @@ describe.skip('Create token, approve token and approve token without gas', funct | |||
| dapp: true, | |||
| fixtures: 'connected-state', | |||
| ganacheOptions, | |||
| smartContract, | |||
| failOnConsoleError: false, | |||
There was a problem hiding this comment.
I am not 100% sure we need the failOnConsoleError on all the tests, as it looks like they do not throw any console errors naturally (at least in my local env), so in the case there is a console error might be that some "real" error is happening?
There was a problem hiding this comment.
We had failure in one test and I put failOnConsoleError just in that test. After I thought maybe it would be good to put it in all tests, but if that is not usual i can remove it.
There was a problem hiding this comment.
I verified and it's not failing for me without the failOnConsoleError. If they are not failing for you I guess we can remove it
There was a problem hiding this comment.
When I remove from all tests it fails on first one on pipeline(it doesn't fail locally) with an error so I left failOnConsoleError just in one test:
1) Create token, approve token and approve token without gas
imports and renders the balance for the new token:
Error: Errors found in browser console:
https://static.metaswap.codefi.network/api/v1/tokenIcons/1337/0x581c3c1a2a4ebde2a0df29b5cf4c116e42945947.png - Failed to load resource: the server responded with a status of 404 (Not Found)
at withFixtures (test/e2e/helpers.js:130:17)
at runMicrotasks ()
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at async Context. (test/e2e/tests/custom-token-add-approve.spec.js:20:5)
Builds ready [6b91add]
Page Load Metrics (1704 ± 27 ms)
|
I saw this approach in metamask-ui.spec.js file so put it like this also here. But yes I agree we could change it. |
Builds ready [8ff007c]
Page Load Metrics (1862 ± 72 ms)
|
| await driver.press('#password', driver.Key.ENTER); | ||
|
|
||
| // create token | ||
| await driver.openNewPage(`http://127.0.0.1:8080/`); |
There was a problem hiding this comment.
you should be able to navigate to the test dapp with the contract deployed:
const contractAddress = await contractRegistry.getContractAddress(
smartContract,
);
await driver.openNewPage(
`http://127.0.0.1:8080/?contract=${contractAddress}`,
);
There was a problem hiding this comment.
similar to the tests below
|
|
||
| // create token | ||
| await driver.openNewPage(`http://127.0.0.1:8080/`); | ||
| await driver.clickElement({ text: 'Create Token', tag: 'button' }); |
There was a problem hiding this comment.
You shouldn't need to create the token, as its already created using the seeder
There was a problem hiding this comment.
similar to the tests below
| ], | ||
| }; | ||
|
|
||
| it('creates, imports and renders the balance for the new token', async function () { |
There was a problem hiding this comment.
We can also remove creates from the test title
e6e948b to
44ffd4b
Compare
Builds ready [84ea775]
Page Load Metrics (2345 ± 91 ms)
|
4371d0e to
9ede8f2
Compare
Builds ready [deedd51]
Page Load Metrics (2191 ± 55 ms)
|
756f803 to
e4711ba
Compare
Builds ready [e4711ba]
Page Load Metrics (2229 ± 122 ms)
|
|
@seaona @PeterYinusa @mirjanaKukic should this be rebased and reviewed again, or dropped? |
lint fix small refactoring removed describe blocks added assert removed waitForSelectors fix added click for View full transaction details removed faildOnConsoleError added failOnConsoleError in one test lint fix added fixture builder
e4711ba to
17efda2
Compare
Builds ready [17efda2]
Page Load Metrics (2426 ± 147 ms)
Bundle size diffs
|
I've done rebase. |
|
This one should not be merged since new screen for token allowance will be merged soon and tests for new screen are also done on this PR: #16740 |
Explanation
We need to refactor these tests in order to use the smart contract deployer, instead of deploying each contract manually, using the Dapp UI.
Tests which will be refactored are in custom-token-add-approve.spec.js file
Resolves #15417
More Information
The main goal is to deploy the e2e smart contracts directly on Ganache, instead of using the Dapp Test interface for achieving the same actions.
These smart contracts can be used later within the e2e tests in order to reduce the amount of code needed to be written in order to execute the same action every time for each of the tests (i.e. contract deployment). This would also reduce the time needed for e2e tests execution because waiting for the UI action response will be eliminated.
Manual Testing Steps
Run testcase locally or check ci pipeline and ensure that it continues to work normally:
yarn test:e2e:single test/e2e/tests/custom-token-add-approve.spec.jsPre-Merge Checklist
+ If there are functional changes: