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

Fix e2e tests #3651

Merged
merged 11 commits into from
Nov 20, 2023
Merged

Fix e2e tests #3651

merged 11 commits into from
Nov 20, 2023

Conversation

michalinacienciala
Copy link
Contributor

@michalinacienciala michalinacienciala commented Nov 2, 2023

This PR fixes a couple of issues with the e2e tests we've been experiencing lately (and also upgrades Playwrght to the latest stable version). Changes made:

  • NFT collection that we checked in the test had slight change in it's name, the tests were updated to expect the new name
  • Sometimes tests creating new wallets were failing during seed phrase verification because Playwright was saving the seed phrase before it was displayed. We've added a step that checks if the seed phrase is displayed before proceeding further.
  • We've upgraded to latest version of Playwright to have access to some debug functionalities not available in the version we used before
  • We've changed a locator identifying Select token element, because it stopped working after upgrading to newer Playwright version
  • We've changed a RegEx checking the format of the date of a transaction, because it looks that one of the space characters used in the date had changed from no-break space to a regular space.
  • We've temporarily marked a step checking behavior after SIWE in the signing.spec.ts as expected to fail because of the bug on login.xyz. Playwright will treat the test as passing when the step fails and will mark the test as failed if the step passes.

Latest build: extension-builds-3651 (as of Mon, 20 Nov 2023 15:39:07 GMT).

The name of the NFT collection was changed from `Notable Crypto Punks` to
`Notable Punks`. We're updating e2e tests to reflect that.
Previously when constructing an array of the seed words, we didn't verify that
the words are already loaded, which could result in returning an empty array.
Now we make sure all 24 words are present before creating the array with seed
phrase words.
Thanks to the upgrade we're able to access new functionalities, e.g. UI Mode.
@michalinacienciala michalinacienciala self-assigned this Nov 2, 2023
It looks that after upgrading to the latest version of Playwright something
has changed in the way how some locators work. The `.getByRole("button", { name:
"Select token", exact: true })` locator no longer identified the button we
wanted to click on the Swap screen. In this commit we're changing the location
method.
Previously a space character before the 'AM'/'PM' in the date was a character
with `U+202F` unicode (narrow no-break space). Now the space character is coded
as `U+0020` (regular space). We'e updating the e2e tests to reflect this change.
Recently the signing in with Ethereum on login.xyz started to misbehave. The old
behavior was that there was a `Vote for your favorite emoji` poll visible on the
website after confirming SIWE in the wallet. Now the website shows the same
state as before the signing. In website console there's a `Cannot read
properties of undefined (reading 'vote')` error. The issue got reported on
https://discord.com/channels/862419652286218251/886997073650655232/1173226370776694794.
We've been using login.xyz in our automated tests when checking SIWE. Until the
issue is fixed, we're marking the test as expected to fail. That means that the
test will be marked in logs as passed when there's no `Vote for your favorite
emoji` on the page after signing. If the string will be found, Playwright will
log a failure and will print `Expected to fail, but passed.`. Placing the
`test.fail()` expression just before the failing step makes sure that failures
on earlier stages will not be considered as expected and will still be
flagged.
@michalinacienciala michalinacienciala marked this pull request as ready for review November 12, 2023 12:46
@michalinacienciala michalinacienciala requested a review from a team as a code owner November 12, 2023 12:46
jagodarybacka
jagodarybacka previously approved these changes Nov 13, 2023
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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


// If we see this then it means we were able to sign in
// There's a bug on login.xyz that makes the test fail
// (https://discord.com/channels/862419652286218251/886997073650655232/1173226370776694794).
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt storing the discord links in a code is a good way to document it 😅 Is the login.xyz website open source? Can we add an issue somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately not, btw. I checked on Friday :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I also prefer documenting such issues in GH, but did not find a repo for the project, hence I contacted them via Discord and documented that.

Btw, I did get a response - they will look into the issue.

@jagodarybacka
Copy link
Contributor

Ugh I see there is something else failing again on e2e flow, could you take a look?

There is a bug in the wallet which manifests with the snackbar with the action's
status not displaying after that action. This happens sporadically and does not
mean that the action wasn't successful. We want to be notified when the bug
occurs, but we don't want it to block execution of the subsequent steps. That's
why we use the soft assertions.
@michalinacienciala
Copy link
Contributor Author

Ugh I see there is something else failing again on e2e flow, could you take a look?

I wasn't able to reproduce the failure locally, but I think it was caused by the #3675 bug. As we will likely want to fix the #3675 soon, I did not do any changes to the tests in relation to this bug.

I did a different change though (f0441b1), so I need a new review.

Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Sorry for the double update, Github glitched
image

Looks good but we will need @Shadowfiend to approve because I'm not in a code owners group

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Lgtm let’s do it. Sorry to block here!

@jagodarybacka jagodarybacka merged commit 080b4d0 into main Nov 20, 2023
6 checks passed
@jagodarybacka jagodarybacka deleted the fix-e2e-tests branch November 20, 2023 15:29
@jagodarybacka jagodarybacka mentioned this pull request Nov 22, 2023
Shadowfiend added a commit that referenced this pull request Nov 23, 2023
## What's Changed
* Check connecting to dapp on different network after disconnecting by
@michalinacienciala in #3654
* v0.51.0 by @Shadowfiend in
#3652
* Fix typos by @xiaolou86 in
#3668
* Alchemic Logs: Small improvements for eth_getLogs by @Shadowfiend in
#3664
* Fix getting currently connected dapp info by @jagodarybacka in
#3671
* Add static subcape link to the wallet by @jagodarybacka in
#3670
* Fix e2e tests by @michalinacienciala in
#3651
* Add Alchemy endpoint for Arbitrum Sepolia by @jagodarybacka in
#3665
* Do notify: Set up base NotificationService by @Shadowfiend in
#3666

## New Contributors
* @xiaolou86 made their first contribution in
#3668

**Full Changelog**:
v0.51.0...v0.52.0

Latest build:
[extension-builds-3678](https://github.com/tahowallet/extension/suites/18415208655/artifacts/1067210815)
(as of Wed, 22 Nov 2023 14:53:11 GMT).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants