Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat: added notification snap to methods-snap #137 #166

Merged
merged 8 commits into from
Nov 10, 2022

Conversation

Lykhoyda
Copy link
Contributor

@Lykhoyda Lykhoyda commented Nov 2, 2022

Short description of work done

Added example of usage Notification snaps; added test for inApp snap method

PR Checklist

  • I have run linter locally
  • I have run unit and integration tests locally
    • Update configuration the newest version (readme and const)
  • Rebased to master branch / merged master

Changes

  • EXAMPLE - Removed basic auth from the controller

Issues

Closes #137

@Lykhoyda Lykhoyda force-pushed the lykhoyda/snap-notifications_137 branch from b4b3604 to a0edd88 Compare November 2, 2022 10:03
@Lykhoyda Lykhoyda changed the title Lykhoyda/snap notifications 137 feat: added notification snap to methods-snap Nov 2, 2022
@Lykhoyda Lykhoyda changed the title feat: added notification snap to methods-snap feat: added notification snap to methods-snap #137 Nov 2, 2022
@mpetrunic mpetrunic linked an issue Nov 2, 2022 that may be closed by this pull request
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

I don't think api is right.
There should be method on dappeteer with following interface:

dappeteer.snaps.waitForInAppNotification(opts?: {
  markRead?: boolean, //default true
  title?: string //resolves only with notification with given title
timeout?: number
}): Promise<{title: string, content: string}>

@Lykhoyda
Copy link
Contributor Author

Lykhoyda commented Nov 2, 2022

I don't think API is right. There should be method on dappeteer with following interface:

dappeteer.snaps.waitForInAppNotification(opts?: {
  markRead?: boolean, //default true
  title?: string //resolves only with notification with the given title
}): Promise<{title: string, content: string}>

@mpetrunic what do you mean by waiting for notification? I'm confused as it will not "freeze" the screen like in the dialog case where you need to accept or reject. It will appear in the notification menu and continue to execute the rest. And I'm not completely getting about the title, as we can pass only the "message". So the method will probably look like this:

dappeteer.snaps.waitForInAppNotification( 
      page: Page,
      snapId: string,
      method: string,
      opts?: {
     markRead?: boolean, //default true
}): Promise<{title: string, content: string}>

@mpetrunic
Copy link
Member

When user tests snap we want him to write something like this:

await dappeteer.snap.invokeMethod()
const notificationPromise = dappeteer.snap.waitForNotifocation() //this is missing
await dappeteer.snap.acceptDialog()
const notification = await notificationPromise;
expect(notification.content) .to.equal("something")

@Lykhoyda
Copy link
Contributor Author

Lykhoyda commented Nov 3, 2022

When user tests snap we want him to write something like this:

await dappeteer.snap.invokeMethod()
const notificationPromise = dappeteer.snap.waitForNotifocation() //this is missing
await dappeteer.snap.acceptDialog()
const notification = await notificationPromise;
expect(notification.content) .to.equal("something")

@mpetrunic I see the idea of improving the Developer experience, but we probably need to come up with a different way. As soon as we will invokeMethod() the notification will appear instantly in the menu bar in the Notifications menu. So technically there is nothing to wait for as in a dialog case where we wait for user action. And also acceptDialog in this case sounds confusing as the only thing we can do is read the notification. "acceptDialog" will lead the user to think that some modal window should appear where it can be "accepted". I assume the next things will have value for a developer:

  1. invoke Notification
  2. mark the notification as read
  3. check notification text through UI as e2e tests are user-centric. We should check for a text shown in the metamask UI and not compare it with the text from invokingPromise.

What do you think about such methods?
await dappeteer.snap.invokeMethod() // or await dappeteer.snap.invokeNotification()
await dappeteer.snap.markLastNotificationAsRead()

expect(await dappeteer.snap.getLastNotificationText()).to.equal("text");

@mpetrunic
Copy link
Member

mpetrunic commented Nov 3, 2022

When user tests snap we want him to write something like this:

await dappeteer.snap.invokeMethod()
const notificationPromise = dappeteer.snap.waitForNotifocation() //this is missing
await dappeteer.snap.acceptDialog()
const notification = await notificationPromise;
expect(notification.content) .to.equal("something")

@mpetrunic I see the idea of improving the Developer experience, but we probably need to come up with a different way. As soon as we will invokeMethod() the notification will appear instantly in the menu bar in the Notifications menu. So technically there is nothing to wait for as in a dialog case where we wait for user action. And also acceptDialog in this case sounds confusing as the only thing we can do is read the notification. "acceptDialog" will lead the user to think that some modal window should appear where it can be "accepted". I assume the next things will have value for a developer:

  1. invoke Notification
  2. mark the notification as read
  3. check notification text through UI as e2e tests are user-centric. We should check for a text shown in the metamask UI and not compare it with the text from invokingPromise.

What do you think about such methods? await dappeteer.snap.invokeMethod() // or await dappeteer.snap.invokeNotification() await dappeteer.snap.markLastNotificationAsRead()

expect(await dappeteer.snap.getLastNotificationText()).to.equal("text");

Sorry, I was typing code on mobile, it should be:

const notificationPromise = dappeteer.snap.waitForNotifocation() //this is missing
await dappeteer.snap.invokeMethod()
await dappeteer.snap.acceptDialog()
const notification = await notificationPromise;
expect(notification.content) .to.equal("something")

Marking as read is not really something the user needs when testing snaps but it's useful for some other snap simulations. AS a user testing snap, I wanna make sure a notification appeared after I invoked snap meaning snap followed the expected path. It would be nice if we could also support API to check if a notification wasn't emitted.

Current PR doesn't hold any value for the user as it's just testing we can invoke snap that emits notification

src/snap/getAllNotifications.ts Outdated Show resolved Hide resolved
src/snap/getAllNotifications.ts Outdated Show resolved Hide resolved
src/snap/getAllNotifications.ts Outdated Show resolved Hide resolved
@mpetrunic mpetrunic merged commit 4804b88 into unstable Nov 10, 2022
@mpetrunic mpetrunic deleted the lykhoyda/snap-notifications_137 branch November 10, 2022 10:05
mpetrunic added a commit that referenced this pull request Dec 15, 2022
* chore: update CI for unstable branch

* fix!: casing of MetaMask (#132)

Fix casing of MetaMask

Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* fix: update ganache, fix test depending on goerli (#144)

* fix: selector issues, useless timeouts, reorganise tests (#145)

* fix: update ganache, fix test depending on goerli

* fix: bugs in selectors, less timeout, reorganise structure

* update yarn

* chore: improve root hooks

* try to increase timeout

* better ci

* add screenshot uploading to ci

* fix screenshot name

* run screenshot on failed test

* fix lint

* change viewport

* fix tests

* fix#123124

* fix tests

* whyyyyyyyyyyyyyyyyyyyyyyyyy

* please

* fix testing dapp

* dunno anymore

* fix lint

* fix typo in selector

* update dapp loading

* refactor script fetching for `index.html`

* fix dam sing test

Co-authored-by: Bernard <bero4net@gmail.com>

* fix: import token flaky (#149)

fix: import-token flakyness

* feat!: update recommended metamask version (#151)

* feat!: update recommended metamask version

* implement screenshot helper

* implement hide portfolio popup action

* implement close what's new modal action

* remove obsolete test

* fix dappeteer methods broken on update

* chore: fix lint

* update metamask version to `10.20.0`

* fix addToken.ts

* address issues

* fix lint

* fix tests

* fix #4

* fix non visible buttons

Co-authored-by: Bernard Stojanović <bero4net@gmail.com>

* chore: change eslint config to chainsafe shared (#152)

* chore: change eslint to chainsafe config

* address PR comments

* address PR comments

* fix tests

* feat: add support for installing metamask flask (#153)

feat: add support for installing metamask flaask

* feat: ability to install snap (#154)

* feat: add support for installing metamask flaask

* feat: ability to install snaps

* fi lint

* fix starting a snap

* fix snap install

Co-authored-by: Bernard <bero4net@gmail.com>

* fix: snap install faster, run all tests (#163)

* fix: run all tests, faster check for installed snap

* fix condition bug

* feat: Add invokeSnap method; update installSnap method parameter; (#159)

* Add invokeSnap method; update installSnap method parameter;

* adjust invokeSnap method after review

* Fix params type in invokeSnap method

* update invokeSnap Return type

* update invokeSnap with generic params

* update test expectation

* fix lint

* feat: add ability to accept dialogs (#138) (#164)

* feat:add ability to accept dialogs (#138)

* fix lint

* move installSnap, invokeSnap to dappeteer api; add bringToFront call to dialog methods

* pair improvements

* fix lint

* add methods snap

* exclude flask tests from global

* exclude flask tests from global

Co-authored-by: Bernard <bero4net@gmail.com>

* feat!:  add playwright support (#167)

* feat!: add playwright support

* fix: lint

* fix: ci matrix

* don't run snap tests on non flask

* address PR comments

* feat: added notification snap to methods-snap #137 (#166)

* pair improvements

* fix lint

* feat: add notify Snap method

* Added getAllNotifications method

* go back after getting all notifications

* remove timeout

* fix post merge errors

Co-authored-by: Bernard <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* feat: method to bootstrap snap env (#180)

* feat: install snap from local files

* fix: installing and invokeing snaps

* disable addToken test

* disable fail fast

* replace binance smart chain

* chore: node engine requirements (#184)

set engines compatibility for 16 and above

* chore: remove metamask dir (#185)

* fix: remove page param from install snap (#188)

* feat!: replace outdated methods (#189)

* feat!: replaced addTokenMethod

* feat: remove add network method

* chore: Ci enhancement (#193)

* cache yarn and have lint and build as separate steps

* add back yarn.lock

* add jobs

* fix name

* feat: allow signing typed data (#191)

* add test to sign typed data

* add a check to see if the sign button is actually disabled

* clean up

* fix and clean

* remove flackyness

* lint

* fix playwright

* add hidden option for pupeteer types waitForSelector

* remove reload between tests

* fix: fix prompt clicking flakiness, fix multiple snap key permissions (#194)

* add retries when confirming prompts with lower timeout

* better snap error handling, ability to accept multiple key permissions

* bail on first fail

* add context to screenshots

* remove various random timeouts, add wait for overlay, more retrys

* fix lint

* increase timeout for loading network and token details

* address PR comments

* increase timeout

* add retry to profile dropdown

* feat: snap notifications 137 (#187)

* chore: node engine requirements (#184) (#186)

set engines compatibility for 16 and above

Co-authored-by: Bernard Stojanović <bero4net@gmail.com>

* pair improvements

* fix lint

* feat: add notify Snap method

* go back after getting all notifications

* remove timeout

* wip

* Add notification observe method

* update test

* remove unused code

* wip notifiction observer

* wip observer based notifications

* wip observer based notifications

* remove unused dependency

* cleanup

* fixes after merge

* revert method names

* clean waitForNotification method

* emitter solution - FP

* emitter solution - ClassBased

* Cleanup class based emitter

* fix lint

* fixes after merge

* remove p-event library

* remove eslint comment

* update test configuration

* return NotificationList from waitForNotification

* add getNotificationEmitter method

* remove back button

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>
Co-authored-by: Bernard Stojanović <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* chore: Deprecate button clicks for tests (#195)

* sharedConst and signature check

* without sharedConst :)

* request accounts

* request accounts

* long typed data

* short typed data

* add token reject/accept

* add network accept/reject

* actually verify the lock/unlock

* contract interactions

* address comment

* web3-utils 1.3.4

* addToken and addNetwork use boolean

* remove useless wait

* chore: Update documentation and Readme (#202)

* docs

* change to metaMask accross the board and use bootstrap in our test

* MetaMMMMMMask

* snaps methods in API.md

* snaps usage in Readme.md

* Update README.md

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* address comment

* add forgotten menu item for initSnapEnv

* removing jsdoc

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* chore: Remove local server for dapp (#203)

* docs

* change to metaMask accross the board and use bootstrap in our test

* MetaMMMMMMask

* snaps methods in API.md

* snaps usage in Readme.md

* init

* lint

* add dev deps for ganache/console.log to make build pass

* Update README.md

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* address comment

* add forgotten menu item for initSnapEnv

* removing jsdoc

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* feat: Simplify `installSnap` and `initSnapEnv` apis (#206)

* simplify api

* remove stray comments

* fix undefined opts

* double question mark back :)

* Update src/snap/utils.ts

* 2s for my use case

* test perf

* elegant race with bad name

* chore: update web3 (#217)

chore: upgrade web3.js

* chore: update metamask version (#209)

* update recommended version

* chore: fix timeout for accept dialog (#218)

increase timeout for accept and reject dialog

* fix: increase viewport size (#219)

* Set viewport to most popular desktop resolution; fix for #208

Co-authored-by: Bernard Stojanović <bero4net@gmail.com>

* fix: viewport typo (#220)

fix viewport typo

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Co-authored-by: Bernard <bero4net@gmail.com>
Co-authored-by: Anton Lykhoyda <lykhoyda@gmail.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
mpetrunic added a commit that referenced this pull request Dec 15, 2022
* pair improvements

* fix lint

* feat: add notify Snap method

* Added getAllNotifications method

* go back after getting all notifications

* remove timeout

* fix post merge errors

Co-authored-by: Bernard <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>
mpetrunic added a commit that referenced this pull request Dec 15, 2022
* pair improvements

* fix lint

* feat: add notify Snap method

* Added getAllNotifications method

* go back after getting all notifications

* remove timeout

* fix post merge errors

Co-authored-by: Bernard <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>
BeroBurny added a commit that referenced this pull request Jan 20, 2023
* chore: update CI for unstable branch

* fix!: casing of MetaMask (#132)

Fix casing of MetaMask

Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* fix: update ganache, fix test depending on goerli (#144)

* fix: selector issues, useless timeouts, reorganise tests (#145)

* fix: update ganache, fix test depending on goerli

* fix: bugs in selectors, less timeout, reorganise structure

* update yarn

* chore: improve root hooks

* try to increase timeout

* better ci

* add screenshot uploading to ci

* fix screenshot name

* run screenshot on failed test

* fix lint

* change viewport

* fix tests

* fix#123124

* fix tests

* whyyyyyyyyyyyyyyyyyyyyyyyyy

* please

* fix testing dapp

* dunno anymore

* fix lint

* fix typo in selector

* update dapp loading

* refactor script fetching for `index.html`

* fix dam sing test

Co-authored-by: Bernard <bero4net@gmail.com>

* fix: import token flaky (#149)

fix: import-token flakyness

* feat!: update recommended metamask version (#151)

* feat!: update recommended metamask version

* implement screenshot helper

* implement hide portfolio popup action

* implement close what's new modal action

* remove obsolete test

* fix dappeteer methods broken on update

* chore: fix lint

* update metamask version to `10.20.0`

* fix addToken.ts

* address issues

* fix lint

* fix tests

* fix #4

* fix non visible buttons

Co-authored-by: Bernard Stojanović <bero4net@gmail.com>

* chore: change eslint config to chainsafe shared (#152)

* chore: change eslint to chainsafe config

* address PR comments

* address PR comments

* fix tests

* feat: add support for installing metamask flask (#153)

feat: add support for installing metamask flaask

* feat: ability to install snap (#154)

* feat: add support for installing metamask flaask

* feat: ability to install snaps

* fi lint

* fix starting a snap

* fix snap install

Co-authored-by: Bernard <bero4net@gmail.com>

* fix: snap install faster, run all tests (#163)

* fix: run all tests, faster check for installed snap

* fix condition bug

* feat: Add invokeSnap method; update installSnap method parameter; (#159)

* Add invokeSnap method; update installSnap method parameter;

* adjust invokeSnap method after review

* Fix params type in invokeSnap method

* update invokeSnap Return type

* update invokeSnap with generic params

* update test expectation

* fix lint

* feat: add ability to accept dialogs (#138) (#164)

* feat:add ability to accept dialogs (#138)

* fix lint

* move installSnap, invokeSnap to dappeteer api; add bringToFront call to dialog methods

* pair improvements

* fix lint

* add methods snap

* exclude flask tests from global

* exclude flask tests from global

Co-authored-by: Bernard <bero4net@gmail.com>

* feat!:  add playwright support (#167)

* feat!: add playwright support

* fix: lint

* fix: ci matrix

* don't run snap tests on non flask

* address PR comments

* feat: added notification snap to methods-snap #137 (#166)

* pair improvements

* fix lint

* feat: add notify Snap method

* Added getAllNotifications method

* go back after getting all notifications

* remove timeout

* fix post merge errors

Co-authored-by: Bernard <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* feat: method to bootstrap snap env (#180)

* feat: install snap from local files

* fix: installing and invokeing snaps

* disable addToken test

* disable fail fast

* replace binance smart chain

* chore: node engine requirements (#184)

set engines compatibility for 16 and above

* chore: remove metamask dir (#185)

* fix: remove page param from install snap (#188)

* feat!: replace outdated methods (#189)

* feat!: replaced addTokenMethod

* feat: remove add network method

* chore: Ci enhancement (#193)

* cache yarn and have lint and build as separate steps

* add back yarn.lock

* add jobs

* fix name

* feat: allow signing typed data (#191)

* add test to sign typed data

* add a check to see if the sign button is actually disabled

* clean up

* fix and clean

* remove flackyness

* lint

* fix playwright

* add hidden option for pupeteer types waitForSelector

* remove reload between tests

* fix: fix prompt clicking flakiness, fix multiple snap key permissions (#194)

* add retries when confirming prompts with lower timeout

* better snap error handling, ability to accept multiple key permissions

* bail on first fail

* add context to screenshots

* remove various random timeouts, add wait for overlay, more retrys

* fix lint

* increase timeout for loading network and token details

* address PR comments

* increase timeout

* add retry to profile dropdown

* feat: snap notifications 137 (#187)

* chore: node engine requirements (#184) (#186)

set engines compatibility for 16 and above

Co-authored-by: Bernard Stojanović <bero4net@gmail.com>

* pair improvements

* fix lint

* feat: add notify Snap method

* go back after getting all notifications

* remove timeout

* wip

* Add notification observe method

* update test

* remove unused code

* wip notifiction observer

* wip observer based notifications

* wip observer based notifications

* remove unused dependency

* cleanup

* fixes after merge

* revert method names

* clean waitForNotification method

* emitter solution - FP

* emitter solution - ClassBased

* Cleanup class based emitter

* fix lint

* fixes after merge

* remove p-event library

* remove eslint comment

* update test configuration

* return NotificationList from waitForNotification

* add getNotificationEmitter method

* remove back button

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>
Co-authored-by: Bernard Stojanović <bero4net@gmail.com>
Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>

* chore: Deprecate button clicks for tests (#195)

* sharedConst and signature check

* without sharedConst :)

* request accounts

* request accounts

* long typed data

* short typed data

* add token reject/accept

* add network accept/reject

* actually verify the lock/unlock

* contract interactions

* address comment

* web3-utils 1.3.4

* addToken and addNetwork use boolean

* remove useless wait

* chore: Update documentation and Readme (#202)

* docs

* change to metaMask accross the board and use bootstrap in our test

* MetaMMMMMMask

* snaps methods in API.md

* snaps usage in Readme.md

* Update README.md

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* address comment

* add forgotten menu item for initSnapEnv

* removing jsdoc

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* chore: Remove local server for dapp (#203)

* docs

* change to metaMask accross the board and use bootstrap in our test

* MetaMMMMMMask

* snaps methods in API.md

* snaps usage in Readme.md

* init

* lint

* add dev deps for ganache/console.log to make build pass

* Update README.md

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* address comment

* add forgotten menu item for initSnapEnv

* removing jsdoc

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

* move temporary user data dir in upper scope

* implement exporting state and running from a state

* implement userDataDir for puppeteer

* implement tests

* fix setupBootstrappedMetaMask for flask

* fix flask

* implement addKeyToMetaMaskManifest

* small fixes

* improve userData testing

* fix jest config

* implement default user profile

* small quality of life improvements

* improve documentation

* fix headless issues whit handling files

* improve ci

* fix yml

* fix ci

* improve ci

* fix export

* address comments

Co-authored-by: Marin Petrunic <marin.petrunic@gmail.com>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>
Co-authored-by: Anton Lykhoyda <lykhoyda@gmail.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add method to handle snap notifications
4 participants