test: Adding e2e for SIWE and re-enabling redesign for SIWE #25831
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Looking good! One thing is that I'm not sure if this checks that it presents a Sign-in With Ethereum UI rather than a personal sign UI. To do this we could check the "Signing in with" row exists
another thing that might be good to check here is that domain binding (Domain alert) displays. We should be able to trigger this clicking on the "Bad Account" button. This is unique to SIWE and does not exist for personal sign
|
p.s. adding more tests for SIWE could be done in a future PR and does not block this PR there are also lint errors here. This oftentimes can be fixed using 'yarn lint:fix' |
pnarayanaswamy
dismissed stale reviews from pedronfigueiredo and jpuri
via
9199ebe
Added a test for bad domain @digiwand |
Builds ready [a12218b]
Page Load Metrics (178 ± 224 ms)
Bundle size diffs
|
Builds ready [e3cb93b]
Page Load Metrics (263 ± 265 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25831 +/- ##
===========================================
- Coverage 69.66% 69.66% -0.00%
===========================================
Files 1402 1402
Lines 49659 49656 -3
Branches 13722 13719 -3
===========================================
- Hits 34594 34591 -3
Misses 15065 15065 ☔ View full report in Codecov by Sentry. |
pnarayanaswamy
changed the title
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces end-to-end tests for the Sign-In with Ethereum (SIWE) feature and re-enables the redesigned confirmation UI for SIWE messages.
- New E2E Tests: Added
test/e2e/tests/confirmations/signatures/siwe.spec.tsto cover SIWE request scenarios. - Updated Hook Tests: Modified
ui/pages/confirmations/hooks/useCurrentConfirmation.test.tsto expect specific object structures for SIWE messages. - Hook Logic Simplification: Simplified
ui/pages/confirmations/hooks/useCurrentConfirmation.tsby removing theisSIWEcheck from theshouldUseRedesigncondition. - Potential Pitfalls: Ensure all mocked endpoints and fixtures are correctly set up to avoid false positives/negatives in tests.
3 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings
| @@ -0,0 +1,161 @@ | |||
| import { strict as assert } from 'assert'; | |||
There was a problem hiding this comment.
Info: Imported necessary modules and helper functions for the tests.
| assertAccountDetailsMetrics, | ||
| } from './signature-helpers'; | ||
|
|
||
| describe('Confirmation Signature - SIWE @no-mmi', function (this: Suite) { |
There was a problem hiding this comment.
Info: Test for initiating and confirming a SIWE request.
| ); | ||
| }); | ||
|
|
||
| it('initiates and rejects', async function () { |
There was a problem hiding this comment.
Info: Test for initiating and rejecting a SIWE request.
| ); | ||
| }); | ||
|
|
||
| it('displays alert for domain binding and confirms', async function () { |
There was a problem hiding this comment.
Info: Test for handling domain mismatch alerts and confirming a SIWE request.
| async function assertInfoValues(driver: Driver) { | ||
| const origin = driver.findElement({ text: DAPP_HOST_ADDRESS }); | ||
| const message = driver.findElement({ | ||
| text: 'I accept the MetaMask Terms of Service: https://community.metamask.io/tos', | ||
| }); | ||
|
|
||
| assert.ok(await origin); | ||
| assert.ok(await message); | ||
| } |
There was a problem hiding this comment.
Info: Helper function to assert information values in the SIWE request.
| async function assertVerifiedSiweMessage(driver: Driver, message: string) { | ||
| await driver.waitUntilXWindowHandles(2); | ||
| await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); | ||
|
|
||
| const verifySigUtil = await driver.findElement('#siweResult'); | ||
| assert.equal(await verifySigUtil.getText(), message); | ||
| } |
There was a problem hiding this comment.
Info: Helper function to assert the verified SIWE message.
| @@ -204,7 +204,7 @@ describe('useCurrentConfirmation', () => { | |||
| expect(currentConfirmation).toBeUndefined(); | |||
| }); | |||
|
|
|||
| it('returns undefined if message is SIWE', () => { | |||
| it('returns if message is SIWE', () => { | |||
There was a problem hiding this comment.
Info: Updated test description to reflect the new expected behavior for SIWE messages.
| expect(currentConfirmation).toStrictEqual({ | ||
| id: APPROVAL_MOCK.id, | ||
| msgParams: { siwe: { isSIWEMessage: true } }, | ||
| }); |
There was a problem hiding this comment.
Info: Updated expectation to check for the correct structure of the returned SIWE message.
| const shouldUseRedesign = | ||
| isRedesignedConfirmationsUserSettingEnabled && | ||
| (isCorrectApprovalType || isCorrectTransactionType) && | ||
| !isSIWE; | ||
| (isCorrectApprovalType || isCorrectTransactionType); |
There was a problem hiding this comment.
Info: Removed isSIWE check from shouldUseRedesign condition.
Builds ready [b603beb]
Page Load Metrics (278 ± 292 ms)
Bundle size diffs
|
metamaskbot
added
the
release-12.3.0
Description
Related issues
Fixes: #24679
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist