Skip to content

Conversation

agualis
Copy link

@agualis agualis commented Jul 21, 2025

Fixes #15896 by adding a playwright test for start page to test wallet connection

Description

Adds a new rainbow kit Test group with wagmi's mock connector (only when the app is running in localhost).

Another approach could be using an env var like NEXT_PUBLIC_ALLOW_WALLET_MOCK that must be true when running the app to be tested by playwright but I find it less straight forward (so worse DX).

Mock-Wallet

Copy link

netlify bot commented Jul 21, 2025

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit e3ed01f
🔍 Latest deploy log https://app.netlify.com/projects/ethereumorg/deploys/68c7cbd5201768000833dd85
😎 Deploy Preview https://deploy-preview-15897--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 42 (🔴 down 9 from production)
Accessibility: 96 (🟢 up 1 from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 92 (no change from production)
PWA: 59 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

},
],
wallets: walletGroups,
ssr: isLocalhost,
Copy link
Author

@agualis agualis Jul 21, 2025

Choose a reason for hiding this comment

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

Not using ssr: true casues hydration issues. Is there an explicit reason why you are not using it by default?

Copy link
Member

Choose a reason for hiding this comment

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

True, I don't see why not to enable this.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming we still want this updated?

Suggested change
ssr: isLocalhost,
ssr: true,

@wackerow
Copy link
Member

Thanks @agualis! Will get some eyes on this soon

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Hey @agualis, thanks for adding this test! 💪🏼

The test looks good and is working perfectly for me locally, but I think we could make a small refactor to organize the code better.

To keep things separated and clean, I'd suggest placing all test-related files and code within the /tests/e2e folder. We could:

  • Move mockWallet to the fixtures folder.
  • Create a rainbowkitConfig specifically for the test runtime.

We could export the config from rainbow-kit.ts and then extend it in a test.beforeEach block, for example. This way, we keep the code clean and decoupled from the test logic.

LMK what do you think.

},
],
wallets: walletGroups,
ssr: isLocalhost,
Copy link
Member

Choose a reason for hiding this comment

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

True, I don't see why not to enable this.

@agualis agualis force-pushed the all-contributors/test-start-page branch from 0cf12ba to 05167d2 Compare July 25, 2025 16:11
@agualis
Copy link
Author

agualis commented Jul 25, 2025

Thanks for reviewing @pettinarip

The test looks good and is working perfectly for me locally, but I think we could make a small refactor to organize the code better.

Move mockWallet to the fixtures folder.

Done ✅

  • Create a rainbowkitConfig specifically for the test runtime.
    We could export the config from rainbow-kit.ts and then extend it in a test.beforeEach block, for example. This way, we keep the code clean and decoupled from the test logic.

I understand your concern but this is not so trivial because, in order to update rainbowkitConfig from the tests we should store it in a react useState and expose a method to be called from playwright (which is not so straightforward).

If you want to keep the original rainbow-kit.ts file clean, we could isolate the test config in its own file like I show here.

✅ Test setup would be more isolated.
❌ We introduce one test related line in WalletProviders.tsx to chose between rainbowkitTestConfig and rainbowkitConfig which is not ideal but much simpler that the mentioned useState approach.

Happy to use an alternative simpler approach if you know one.

Copy link
Contributor

This issue is stale because it has been open 30 days with no activity.

@github-actions github-actions bot added the Status: Stale This issue is stale because it has been open 30 days with no activity. label Aug 25, 2025
@wackerow wackerow requested a review from pettinarip September 10, 2025 10:44
@wackerow wackerow removed the Status: Stale This issue is stale because it has been open 30 days with no activity. label Sep 10, 2025
@wackerow
Copy link
Member

wackerow commented Sep 10, 2025

@agualis Thanks for the updates here!—any chance you could help clear up the merge conflicts? 🙏
Pinged @pettinarip for another review

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@agualis you are right. If we add more tests that depends on this in the future we might change it. As of now, current implementation looks good enough.

@agualis
Copy link
Author

agualis commented Sep 11, 2025

@agualis Thanks for the updates here!—any chance you could help clear up the merge conflicts? 🙏 Pinged @pettinarip for another review

I don't have time this week but I will try next one.

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label Sep 15, 2025
@agualis
Copy link
Author

agualis commented Sep 15, 2025

@agualis Thanks for the updates here!—any chance you could help clear up the merge conflicts? 🙏

@pettinarip @wackerow Cleared up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add e2e test for start page to test wallet connection
3 participants