feat: added notification snap to methods-snap #137

Lykhoyda commented 2 years ago

Short description of work done

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

PR Checklist

[x] I have run linter locally
[x] 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

invoke Notification

mark the notification as read

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

ChainSafe / dappeteer