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.

Sign up for GitHub

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

Jump to bottom

Create E2E tests for verified/unverified tokens #3472

Merged
merged 37 commits into from
Aug 16, 2023
Merged

Create E2E tests for verified/unverified tokens #3472

Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
fdf09c6
Create E2E tests for verified/unverified tokens
michalinacienciala Jun 13, 2023
19cebd0
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Jun 26, 2023
2ce0f5c
Uncomment temporarily turned off test steps
michalinacienciala Jun 29, 2023
db5302a
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Jun 29, 2023
1b6d888
Switch test to use account that we control
michalinacienciala Jul 6, 2023
bdf5b87
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Jul 20, 2023
1debde9
Adjust the code after the merge
michalinacienciala Jul 21, 2023
b1d8382
Improve comments
michalinacienciala Jul 21, 2023
c0f9193
Refactor code asserting asset's balance
michalinacienciala Jul 24, 2023
3f0a378
Uncomment temporarily disabled assertions
michalinacienciala Jul 24, 2023
bf682cc
Merge `assertAssetDetailsPage()` with `verifyAssetActivityScreen()`
michalinacienciala Jul 24, 2023
1a75c5d
Remove obsolete comment
michalinacienciala Jul 24, 2023
06208b6
Improve comments
michalinacienciala Jul 24, 2023
7511aa4
Update names of the JSON-related variables and secrets
michalinacienciala Jul 24, 2023
f37e61b
Wait 1s before switching to Polygon network
michalinacienciala Jul 24, 2023
a07be0a
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Jul 24, 2023
22a9b91
Give dev fork builds the -fork suffix
michalinacienciala Jul 24, 2023
4ef9622
Increase waiting time before switching network
michalinacienciala Jul 24, 2023
5debafc
Uncomment previously failing code
michalinacienciala Jul 25, 2023
cbd2990
Remove empty line
michalinacienciala Jul 25, 2023
b16c9a0
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Jul 27, 2023
5ad6ca6
Rename `verify...` functions to `assert...`
michalinacienciala Jul 27, 2023
833cbf4
Update dApp connection banner assertion
michalinacienciala Jul 27, 2023
0fa759f
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Aug 3, 2023
95db29f
Extract the name and address of the testing account to a helper file
michalinacienciala Aug 4, 2023
b4a61d2
Explain usage of ElementHandle in `closeAccountsPopup()`
michalinacienciala Aug 6, 2023
4e740df
Remove obsolete steps in `addAddressToAccount()`
michalinacienciala Aug 6, 2023
e9d7951
Remove obsolete comment
michalinacienciala Aug 6, 2023
89019b4
Improve comments
michalinacienciala Aug 6, 2023
c562952
Refactor function closing Accounts slide up
michalinacienciala Aug 14, 2023
1cf604e
Move the `closeAccountsSlideUp()` steps under `addAddressToAccount()`
michalinacienciala Aug 14, 2023
c21299e
Remove obsolete timeout
michalinacienciala Aug 14, 2023
1265763
Support assets with the same name in `assertVerifiedAssetOnWalletPage`
michalinacienciala Aug 15, 2023
5f45f7f
Refactor onboarding with JSON
michalinacienciala Aug 15, 2023
bd19521
Access account-specific data in e2e tests using interface
michalinacienciala Aug 15, 2023
6bfbd18
Merge remote-tracking branch 'origin/main' into trust-tokens-e2e
michalinacienciala Aug 15, 2023
0013d2a
Merge branch 'main' into trust-tokens-e2e
hyphenized Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ jobs:
# tests on every PR update. We'll tag those tests with the `@expensive`
# tag.
- name: Run free Playwright tests
env:
E2E_TEST_ONLY_WALLET_JSON_BODY: ${{ secrets.E2E_TEST_ONLY_WALLET_JSON_BODY }}
E2E_TEST_ONLY_WALLET_JSON_PASSWORD: ${{ secrets.E2E_TEST_ONLY_WALLET_JSON_PASSWORD }}
run: xvfb-run npx playwright test --grep-invert @expensive
#env:
# DEBUG: pw:api*
Expand Down
74 changes: 39 additions & 35 deletions e2e-tests/fork-based/transactions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fs from "fs"
import { test, expect } from "../utils"
import { account2Name } from "../utils/onboarding"

test.describe("Transactions", () => {
test.beforeAll(async () => {
Expand All @@ -24,6 +25,7 @@ test.describe("Transactions", () => {
page: popup,
walletPageHelper,
transactionsHelper,
assetsHelper,
}) => {
await test.step("Import account", async () => {
/**
Expand All @@ -45,12 +47,12 @@ test.describe("Transactions", () => {
* Verify we're on Ethereum network. Verify common elements on the main
* page.
*/
await walletPageHelper.verifyCommonElements(
await walletPageHelper.assertCommonElements(
/^Ethereum$/,
false,
/^testertesting\.eth$/
account2Name
)
await walletPageHelper.verifyAnalyticsBanner()
await walletPageHelper.assertAnalyticsBanner()

/**
* Verify ETH is visible on the asset list and has the correct balance
Expand All @@ -74,9 +76,9 @@ test.describe("Transactions", () => {
* already selected. Verify elements on the page. Make sure `Continue`
* isn't active.
*/
await transactionsHelper.verifyUnfilledSendAssetScreen(
await transactionsHelper.assertUnfilledSendAssetScreen(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
"ETH",
"0\\.1021",
true
Expand Down Expand Up @@ -110,7 +112,7 @@ test.describe("Transactions", () => {
/**
* Check if "Transfer" has opened and verify elements on the page.
*/
await transactionsHelper.verifyTransferScreen(
await transactionsHelper.assertTransferScreen(
"Ethereum",
"testertesting\\.eth",
"0x47745a7252e119431ccf973c0ebd4279638875a6",
Expand Down Expand Up @@ -143,12 +145,12 @@ test.describe("Transactions", () => {
/**
* Verify elements on the asset activity screen
*/
await transactionsHelper.verifyAssetActivityScreen(
await assetsHelper.assertAssetDetailsPage(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a variable for testertesting.eth string as well? Also other strings like Ethereum etc - so we can avoid typos or other easy bugs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we use a variable for testertesting.eth string as well?

Did you mean this comment for line 115 (and other similar)? If so, I've applied that in 95db29f.

Also other strings like Ethereum etc - so we can avoid typos or other easy bugs

and

Network's strings should be reusable variables, same with most common assets like /^MATIC$/, /^WMATIC$/ etc

I feel a bit torn about this...

The good side of moving commonly used labels to some external place is what you said and also possibility to apply changes to the labels quickly, without having to modify them in many places.

But there are also some cons, mainly in cases where we also have the same labels abstracted in the app's code (as this is the case for network names and base assets labels stored in network.ts). What I mean is that having similar implementation in tests and in tested code may lead to introducing same bug in both places and not catching it by the tests.

Another issue is that in tests we sometimes want to use RegExp, sometimes string, sometimes value with all letters capitalized, sometimes only with the first letter captalized etc, depending on the context. We'll either need a separate variable for each different version of the label we use or we'll need to use some method that modifies given variable into the formatting we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not saying definitely NO to this idea though. The pros may outweigh the cons...
What would be the best way to approach this? Having all the constant values used by the E2E tests stored in one helper file? Or storing them in files they are related to (something like what we have currently, where names of testing accounts are in the onboarding helper)? The first solution seems more universal to me, but I'm curious if there some good practice I should follow here?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that having similar implementation in tests and in tested code may lead to introducing same bug in both places and not catching it by the tests

Another issue is that in tests we sometimes want to use RegExp, sometimes string, sometimes value with all letters capitalized, sometimes only with the first letter captalized etc, depending on the context. We'll either need a separate variable for each different version of the label we use or we'll need to use some method that modifies given variable into the formatting we need.

Both are great points, I'm not entirely onboard with abstracting simple names as it reduces readability on written test cases, which should remain as simple as possible and, are also unlikely to change unless there are changes in the implementation.

Still, I don't think there's much harm if it's a simple string that's repeated multiple times and it's a simple change. Considering that does not seem to be the case here given the multiple variants, I'd keep the hardcoded strings.

/^ETH$/,
/^0\.0914$/,
true
"base"
)
// This is what we expect currently on forked network. If ve ever fix
// displaying activity on fork, we should perform following checks
Expand Down Expand Up @@ -192,12 +194,12 @@ test.describe("Transactions", () => {
.first()
).toBeVisible()

await walletPageHelper.verifyCommonElements(
await walletPageHelper.assertCommonElements(
/^Ethereum$/,
false,
/^testertesting\.eth$/
account2Name
)
await walletPageHelper.verifyAnalyticsBanner()
await walletPageHelper.assertAnalyticsBanner()
}
)
})
Expand All @@ -206,6 +208,7 @@ test.describe("Transactions", () => {
page: popup,
walletPageHelper,
transactionsHelper,
assetsHelper,
}) => {
await test.step("Import account", async () => {
/**
Expand All @@ -227,12 +230,12 @@ test.describe("Transactions", () => {
* Verify we're on Ethereum network. Verify common elements on the main
* page.
*/
await walletPageHelper.verifyCommonElements(
await walletPageHelper.assertCommonElements(
/^Ethereum$/,
false,
/^testertesting\.eth$/
account2Name
)
await walletPageHelper.verifyAnalyticsBanner()
await walletPageHelper.assertAnalyticsBanner()

/**
* Verify KEEP is visible on the asset list and has the correct balance
Expand Down Expand Up @@ -261,9 +264,9 @@ test.describe("Transactions", () => {
* already selected. Verify elements on the page. Make sure `Continue`
* isn't active.
*/
await transactionsHelper.verifyUnfilledSendAssetScreen(
await transactionsHelper.assertUnfilledSendAssetScreen(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
"KEEP",
"65\\.88",
false
Expand Down Expand Up @@ -297,7 +300,7 @@ test.describe("Transactions", () => {
/**
* Check if "Transfer" has opened and verify elements on the page.
*/
await transactionsHelper.verifyTransferScreen(
await transactionsHelper.assertTransferScreen(
"Ethereum",
"testertesting\\.eth",
"0x47745a7252e119431ccf973c0ebd4279638875a6",
Expand Down Expand Up @@ -328,12 +331,12 @@ test.describe("Transactions", () => {
/**
* Verify elements on the asset activity screen
*/
await transactionsHelper.verifyAssetActivityScreen(
await assetsHelper.assertAssetDetailsPage(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
/^KEEP$/,
/^65\.88$/,
false,
"knownERC20",
"https://etherscan.io/token/0x85eee30c52b0b379b046fb0f85f4f3dc3009afec"
)
// This is what we expect currently on a forked network.
Expand All @@ -353,6 +356,7 @@ test.describe("Transactions", () => {
page: popup,
walletPageHelper,
transactionsHelper,
assetsHelper,
}) => {
await test.step("Import account", async () => {
/**
Expand All @@ -374,12 +378,12 @@ test.describe("Transactions", () => {
* Verify we're on Ethereum network. Verify common elements on the main
* page.
*/
await walletPageHelper.verifyCommonElements(
await walletPageHelper.assertCommonElements(
/^Ethereum$/,
false,
/^testertesting\.eth$/
account2Name
)
await walletPageHelper.verifyAnalyticsBanner()
await walletPageHelper.assertAnalyticsBanner()

/**
* Verify KEEP is visible on the asset list and has the correct balance
Expand Down Expand Up @@ -408,9 +412,9 @@ test.describe("Transactions", () => {
* already selected. Verify elements on the page. Make sure `Continue`
* isn't active.
*/
await transactionsHelper.verifyUnfilledSendAssetScreen(
await transactionsHelper.assertUnfilledSendAssetScreen(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
"ETH",
"\\d+\\.\\d{4}",
true
Expand Down Expand Up @@ -466,9 +470,9 @@ test.describe("Transactions", () => {
* Verify elements on the page after selecting token. Make sure
* `Continue` isn't active.
*/
await transactionsHelper.verifyUnfilledSendAssetScreen(
await transactionsHelper.assertUnfilledSendAssetScreen(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
"KEEP",
"65\\.88",
false
Expand Down Expand Up @@ -502,7 +506,7 @@ test.describe("Transactions", () => {
/**
* Check if "Transfer" has opened and verify elements on the page.
*/
await transactionsHelper.verifyTransferScreen(
await transactionsHelper.assertTransferScreen(
"Ethereum",
"testertesting\\.eth",
"0x47745a7252e119431ccf973c0ebd4279638875a6",
Expand Down Expand Up @@ -535,12 +539,12 @@ test.describe("Transactions", () => {
/**
* Verify elements on the asset activity screen
*/
await transactionsHelper.verifyAssetActivityScreen(
await assetsHelper.assertAssetDetailsPage(
/^Ethereum$/,
/^testertesting\.eth$/,
account2Name,
/^KEEP$/,
/^53\.54$/,
false,
"knownERC20",
"https://etherscan.io/token/0x85eee30c52b0b379b046fb0f85f4f3dc3009afec"
)
// This is what we expect currently on forked network. If ve ever fix
Expand Down Expand Up @@ -585,12 +589,12 @@ test.describe("Transactions", () => {
.first()
).toBeVisible()

await walletPageHelper.verifyCommonElements(
await walletPageHelper.assertCommonElements(
/^Ethereum$/,
false,
/^testertesting\.eth$/
account2Name
)
await walletPageHelper.verifyAnalyticsBanner()
await walletPageHelper.assertAnalyticsBanner()
}
)
})
Expand Down
Loading
Loading