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: test runner cannot find element when wrapped with another element #2154

Closed
wants to merge 7 commits into from

Conversation

enesozturk
Copy link
Contributor

@enesozturk enesozturk commented Apr 14, 2024

Issue

The Playwright tests are unable to find the element when another DOM element is on top of the queried element. This causes tests to not proceed and fail.

Screenshot 2024-04-14 at 17 31 05

Changes

This PR was introduced to replace the toast components which is rendering the toast at the right-bottom of the screen. These changes are already introduced in #2102 but to be able to fix the test issues first, toast replacement is separated to be sure they are running as expected.

Additional

We are also missing the MAILSAC_API_KEY for the ui-tests Run canary with minimal environment config step.

Copy link

vercel bot commented Apr 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web3modal-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2024 4:53pm
web3modal-gallery ✅ Ready (Inspect) Visit Preview Apr 14, 2024 4:53pm
web3modal-laboratory ✅ Ready (Inspect) Visit Preview Apr 14, 2024 4:53pm

@enesozturk enesozturk requested review from tomiir and svenvoskamp and removed request for tomiir April 14, 2024 14:34
@enesozturk enesozturk changed the title refactor/lab replace toast component fix: test runner cannot find element when wrapped with another element Apr 14, 2024
@@ -13,7 +13,7 @@
"playwright:test:email": "playwright test --grep 'email.spec.ts'",
"playwright:test:siwe": "playwright test --grep siwe.spec.ts",
"playwright:test:sa": "playwright test --grep smart-account.spec.ts",
"playwright:test:canary": "playwright test --retries=0 --grep canary.spec.ts --project='Desktop Chrome/wagmi'",
"playwright:test:canary": "playwright test --retries=0 --grep canary.spec.ts --project='Desktop Firefox/ethers'",
Copy link
Member

@chris13524 chris13524 Apr 14, 2024

Choose a reason for hiding this comment

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

Why are you switching this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for local testing purpose, should have been missed. Will revert back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, why it's testing only on wagmi?

Copy link
Member

Choose a reason for hiding this comment

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

Because the canary is supposed to test if the system is generally working together. E.g. is the relay down, and what is the overall latency of the system over-time. Chromium/Wagmi is the sort of best-case configuration/recommendation and is least likely to fail from test flakes

@@ -95,6 +95,7 @@ jobs:
- name: Run canary with minimal environment config
env:
NEXT_PUBLIC_PROJECT_ID: ${{ secrets.NEXT_PUBLIC_PROJECT_ID }}
MAILSAC_API_KEY: ${{ secrets.TESTS_MAILSEC_API_KEY }}
Copy link
Member

@chris13524 chris13524 Apr 14, 2024

Choose a reason for hiding this comment

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

The whole point of this minimal environment step is to have a minimum configuration. The canary doesn't need this API key and isn't available when the canary is run automatically. So it shouldn't be passed here to ensure the canary works without it.

Copy link
Contributor Author

@enesozturk enesozturk Apr 14, 2024

Choose a reason for hiding this comment

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

I see. Then I think we should remove the requirement of using w3m-email-fixture.ts which is checking MAILSAC_API_KEY and causing tests to fail. Example

I think this is a matter of the PR we introduced minimal test step. I'll take a look how we could do this.

Copy link
Member

Choose a reason for hiding this comment

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

This regression happened before, see this PR which @tomiir helped refactor

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the canary now running new tests? Why was it working before and not anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomiir have no idea tbh. I think we have already open PR about it that I just saw: #2029

Copy link
Member

Choose a reason for hiding this comment

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

The canary has been working, you can see it at the bottom of this Grafana dashboard. Doesn't seem like #2029 affected it

@@ -95,6 +95,7 @@ jobs:
- name: Run canary with minimal environment config
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Run canary with minimal environment config
- name: Run canary with minimal environment config
# Keep in-sync with https://github.com/WalletConnect/rs-relay/blob/master/terraform/canary/main.tf#L48

@enesozturk enesozturk closed this Apr 17, 2024
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