Create E2E tests for verified/unverified tokens

[michalinacienciala]

This commit introduces E2E test that checks the functiolality of verified/unverified tokens. Following general steps are executed:

1. Import account
2. Enable Show unverified assets
3. Hide asset
4. Trust asset
5. Hide trusted asset
   A number of checks is performed during each step.

The commit also introduces helper functions and adds data-testid attribute to a couple of DOM elements.

TODO:

• As some of the code is similar or identical as in the Use fork in E2E tests of transactions #3418 PR which is yet not merged to main, some conflicts may arise and will need to be resolved before this change lands on main.
• Also some changes will need to be made once
  Token Discovery - Remap redux asset balances #3195 gets merged to main, as this PR solves a bug causing failures in the tests (the failing part of the test was temporarily commented out).
• Add E2E_TEST_ONLY_WALLET_JSON_BODY and E2E_TEST_ONLY_WALLET_JSON_PASSWORD secrets in GitHub's settings.
• Wait for Use JSON onboarding in e2e tests of transactions #3559 to be merged and merge main to feature branch (should fix the failing e2e-tests job)
• Handle TODOs in the code

Latest build: extension-builds-3472 (as of Wed, 16 Aug 2023 01:05:46 GMT).

[michalinacienciala]

The tests were passing locally, but we have a failing check in CI. I'll investigate the failures.

[jagodarybacka]

A bunch of possible improvements 🏗️

[jagodarybacka]

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

[michalinacienciala]

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.

[michalinacienciala]

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?

[hyphenized]

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.

[jagodarybacka]

😬 thanks for fixing

[michalinacienciala]

No problem :)

[jagodarybacka]

I think this part should be encapsulated in a method or a util as we most likely will be using this JSON wallet often

[michalinacienciala]

Hmmm, but will that make much difference? When calling the method, we will still need to provide the environment variables as arguments. And this means that we'll still need to have the if/else conditions present in the token-trust.spec.ts, right? Or maybe there's something I'm missing here?

[hyphenized]

If we move these to a helper we can reduce duplication. Since we're doing very similar things with TEST_WALLET_JSON_BODY and E2E_TEST_ONLY_WALLET_JSON_BODY, we could put both of these with the corresponding env variables inside a function (if not onboardWithJSON)

[michalinacienciala]

Is something like this what you had in mind: 5f45f7f?

[jagodarybacka]

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

[jagodarybacka]

this can be declared outside of this test so we don't have to repeat it

[michalinacienciala]

See 89019b4.

[jagodarybacka]

Can we delete it?

[michalinacienciala]

Removed in e9d7951.

[jagodarybacka]

what about this comment?

[michalinacienciala]

Oops, this step and a couple of others nearby were not supposed to land in the final code. Removed in 4e740df.

[jagodarybacka]

don't we want to add proper test ids?

[michalinacienciala]

I couldn't do it simpler than this (maybe there's something I didn't think of?).

The problem is that the slide_up_menu DOM element for Accounts contains a bunch of identical slide_up_menu child elements, all containing their own Close button. All Close buttons except of one are not visible for users. On the DOM side, the difference is that those invisible buttons are wrapped with the div element of slide_up_menu closed class, whereas the visible button is wrapped by slide_up_menu with_overflow div.
Adding a test id to the button or the div element does not solve the issue - the id is added to all the elements and we still have the problem with identifying the one we want to click.

As a workaround we use ElementHandle to locate the right item to click, by specifying that the element shouldn't be wrapped by the DOM item of the closed class (I couldn't do that using classic Locator syntax).

And now, going back to your suggestion - switching to use of the test ids everywhere in the function would not work from the above mentioned reasons. And sticking with use of the ElemenHandle to locate the right slide_up_menu and using the test id to locate the Close menu would also not work, because the getByTestId cannot be called on the ElementHandle objects.

We could add a data-testid="slide_up_menu_close" line after this one and then try to do

this.popup.getByTestId("slide_up_menu_close").click()

in the closeAccountsPopup() function, but this does not work. It finds multiple identical matching DOM elements. The only significant difference between the matched elements is that the parent element of the one we want to click has class slide_up_menu with_overflow

Anyway, as all of this is not straightforward, I've added a comment to the closeAccountsPopup() function, that explains why we use ElementHandle there: b4a61d2.

[hyphenized]

We can add a test id here like accounts_list_slide_up_menu

extension/ui/components/TopMenu/TopMenu.tsx

Lines 127 to 133 in 085b99c

	<SharedSlideUpMenu
	isOpen={isNotificationsOpen}
	allowOverflow
	close={() => {
	setIsNotificationsOpen(false)
	}}
	>

Then use playwright assertions so we don't have to throw up errors if the locator doesn't return anything

const accountsSlideUp = this.popup.getByTestId(
      "accounts_list_slide_up_menu"
    )

    await expect(accountsSlideUp).toBeVisible()

This slide up is indeed trickier because there are nested slide ups so to get the right close button we can just ask playwright to return the first one it finds, we'll get the right one because we start looking from the right slide up menu.

    await accountsSlideUp
      .getByRole("button", { name: "Close menu" })
      .first()
      .click()

This way playwright throws if it can't find the element to click so we can skip the manual if check as well.

[michalinacienciala]

It looks to me that this the test id set in this place does not get reflected in the final DOM. I've set this in the extension/ui/components/TopMenu/TopMenu.tsx:

      <SharedSlideUpMenu
        isOpen={isNotificationsOpen}
        allowOverflow
        close={() => {
          setIsNotificationsOpen(false)
        }}
        data-testid="accounts_list_slide_up_menu"
      >

Then I rebuilt the code and run a test that does this:

      await popup.getByTestId("top_menu_profile_button").last().click()
      await walletPageHelper.closeAccountsSlideUp()

where closeAccountsSlideUp() looks like this:

  async closeAccountsSlideUp(): Promise<void> {
    const accountsSlideUp = await this.popup.getByTestId(
      "accounts_list_slide_up_menu"
    )
    await expect(accountsSlideUp).toBeVisible()
    await accountsSlideUp
      .getByRole("button", { name: "Close menu" })
      .first()
      .click()

I've got a failure of the await expect(accountsSlideUp).toBeVisible() assertion. I checked the DOM of the app and there was no element with the accounts_list_slide_up_menu test id. The Accounts slide up had the slide_up_menu test id, which is set in

extension/ui/components/Shared/SharedSlideUpMenu.tsx

Line 51 in 6b8e4b0

testid = "slide_up_menu",

.
Maybe we can't overwrite one test id with another?

[michalinacienciala]

This works:

  async closeAccountsSlideUp(): Promise<void> {
    const accountsSlideUp = await this.popup.getByTestId("slide_up_menu").nth(2)
    await expect(accountsSlideUp).toBeVisible()
    await accountsSlideUp
      .getByRole("button", { name: "Close menu" })
      .first()
      .click()
  }

However I'm not a fan of using .nth(2) to locate the right slide up menu.
Playing a bit more with this, I was able to simplify the code by using filtering:

  async closeAccountsSlideUp(): Promise<void> {
    const accountsSlideUp = await this.popup
      .getByTestId("slide_up_menu")
      .filter({ hasText: "Accounts" })
    await expect(accountsSlideUp).toBeVisible()
    await accountsSlideUp
      .getByRole("button", { name: "Close menu" })
      .first()
      .click()
  }

[hyphenized]

It looks to me that this the test id set in this place does not get reflected in the final DOM. I've set this in the extension/ui/components/TopMenu/TopMenu.tsx:

      <SharedSlideUpMenu
        isOpen={isNotificationsOpen}
        allowOverflow
        close={() => {
          setIsNotificationsOpen(false)
        }}
        data-testid="accounts_list_slide_up_menu"
      >

On this code, it seems we passed a data-testid prop instead of testid to SharedSlideUpMenu. If that's the case, it would explain why the test id was not set correctly on the element.

[michalinacienciala]

Implemented in c562952.

[jagodarybacka]

These regex should be named variables

[jagodarybacka]

can we add some html attribute or check it by CSS class to make it easier to test?

[michalinacienciala]

According to Playwright documentation

Element is considered visible when it has non-empty bounding box and does not have visibility:hidden computed style. Note that elements of zero size or with display:none are not considered visible.

One thing that we could do is to do something to make the elements' style be computed to visibility:hidden. I'm not sure though how easily can we do this...

Another approach would be to use ElemenHandle syntax to check that elements we want to make sure are invisible are children of the div element with the hidden_assets class, but no visible class. But this seems to me unnecessarily complex, given that checking

await expect(
      this.popup.getByTestId("hidden_assets_container")
    ).not.toBeVisible()

already tells us that the section with unverified assets does not have the visible class.

[michalinacienciala]

BTW, due to the fact that during work on this PR I switched from importing the tests account via recovery phrase to import via JSON, I no longer use this function (and closeAccountsPopup as well). However, those function may come handy some day. What is the best practice here? Leave it as it is or maybe comment out or remove completely?

[hyphenized]

This one looks useful and generic enough that we could benefit from keeping it but I'd suggest that we move here the code from closeAccountsPopup and separate it later if we need the logic somewhere else.

[michalinacienciala]

See 1cf604e.

[hyphenized]

Looks good overall 💪! Left a few comments

[hyphenized]

I think this won't work if we have some spam asset with the same name as the one we just verified

[michalinacienciala]

I've refactored the assertVerifiedAssetOnWalletPage() method to handle such case. See 1265763.

[hyphenized]

We can set the timeout on the assertion itself too (unless for some reason it does not work)

Suggested change

	await this.popup.waitForTimeout(2000) // We need to wait for the background elements to disappear
	await expect(asset.getByText(assetSymbol)).toBeVisible()
	await this.popup.waitForTimeout(2000) // We need to wait for the background elements to disappear
	await expect(asset.getByText(assetSymbol)).toBeVisible({ timeout: 2000 })

[michalinacienciala]

It seems that some of the recent changes made the timeout completely obsolete. I removed it in c21299e.

[hyphenized]

We can add a test id here like accounts_list_slide_up_menu

extension/ui/components/TopMenu/TopMenu.tsx

Lines 127 to 133 in 085b99c

	<SharedSlideUpMenu
	isOpen={isNotificationsOpen}
	allowOverflow
	close={() => {
	setIsNotificationsOpen(false)
	}}
	>

Then use playwright assertions so we don't have to throw up errors if the locator doesn't return anything

const accountsSlideUp = this.popup.getByTestId(
      "accounts_list_slide_up_menu"
    )

    await expect(accountsSlideUp).toBeVisible()

This slide up is indeed trickier because there are nested slide ups so to get the right close button we can just ask playwright to return the first one it finds, we'll get the right one because we start looking from the right slide up menu.

    await accountsSlideUp
      .getByRole("button", { name: "Close menu" })
      .first()
      .click()

This way playwright throws if it can't find the element to click so we can skip the manual if check as well.

[hyphenized]

This one looks useful and generic enough that we could benefit from keeping it but I'd suggest that we move here the code from closeAccountsPopup and separate it later if we need the logic somewhere else.

[Shadowfiend]

@hyphenized do you consider any of your notes above blockers?

[hyphenized]

@hyphenized do you consider any of your notes above blockers?

I don't consider these as blockers for merging, but I do see these as concerns regarding code quality and potential future technical debt.

[Shadowfiend]

Cool, since Michalina is back on Monday we'll keep this fellow open for visibility and merge after she's had a look.

[hyphenized]

Comments addressed