test(notifications): fix and improve

hayemaxi · hayemaxi · commit a9c743e2eb27 · 2024-11-15T11:06:53.000-05:00
- Fix async tests broken by recent commits.
- Add setContext and telemetry verification
diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts
@@ -40,24 +40,22 @@ import { telemetry } from '../shared/telemetry/telemetry'
 export class NotificationsController {
     public static readonly suggestedPollIntervalMs = 1000 * 60 * 10 // 10 minutes
 
-    public readonly storageKey: globalKey
-
     /** Internal memory state that is written to global state upon modification. */
     private readonly state: NotificationsState
 
     static #instance: NotificationsController | undefined
 
     constructor(
         private readonly notificationsNode: NotificationsNode,
-        private readonly fetcher: NotificationFetcher = new RemoteFetcher()
+        private readonly fetcher: NotificationFetcher = new RemoteFetcher(),
+        public readonly storageKey: globalKey = 'aws.notifications'
     ) {
         if (!NotificationsController.#instance) {
             // Register on first creation only.
             registerDismissCommand()
         }
         NotificationsController.#instance = this
 
-        this.storageKey = 'aws.notifications'
         this.state = globals.globalState.tryGet<NotificationsState>(this.storageKey, NotificationsStateConstructor, {
             startUp: {},
             emergency: {},
diff --git a/packages/core/src/notifications/panelNode.ts b/packages/core/src/notifications/panelNode.ts
@@ -78,24 +78,23 @@ export class NotificationsNode implements TreeNode {
     }
 
     public refresh(): void {
-        if (!this.view) {
-            throw new ToolkitError('NotificationsNode: TreeView accessed without being registered.')
-        }
-
         const totalNotifications = this.notificationCount()
-        if (totalNotifications > 0) {
-            this.view.badge = {
-                tooltip: `${totalNotifications} notification${totalNotifications > 1 ? 's' : ''}`,
-                value: totalNotifications,
+        if (this.view) {
+            if (totalNotifications > 0) {
+                this.view.badge = {
+                    tooltip: `${totalNotifications} notification${totalNotifications > 1 ? 's' : ''}`,
+                    value: totalNotifications,
+                }
+                this.view.title = `${NotificationsNode.title} (${totalNotifications})`
+            } else {
+                this.view.badge = undefined
+                this.view.title = NotificationsNode.title
             }
-            this.view.title = `${NotificationsNode.title} (${totalNotifications})`
-            void setContext(this.showContextStr, true)
         } else {
-            this.view.badge = undefined
-            this.view.title = NotificationsNode.title
-            void setContext(this.showContextStr, false)
+            logger.warn('NotificationsNode was refreshed but the view was not initialized!')
         }
 
+        void setContext(this.showContextStr, totalNotifications > 0)
         this.provider?.refresh()
     }
 
@@ -240,7 +239,7 @@ export class NotificationsNode implements TreeNode {
                                 case 'updateAndReload':
                                     // Give things time to finish executing.
                                     globals.clock.setTimeout(() => {
-                                        void this.updateAndReload(notification.displayIf.extensionId)
+                                        return this.updateAndReload(notification.displayIf.extensionId)
                                     }, 1000)
                                     break
                                 case 'openUrl':
diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts
@@ -9,6 +9,7 @@ import assert from 'assert'
 import sinon from 'sinon'
 import globals from '../../shared/extensionGlobals'
 import { randomUUID } from '../../shared'
+import * as setContext from '../../shared/vscode/setContext'
 import { assertTelemetry, installFakeClock } from '../testUtil'
 import {
     NotificationFetcher,
@@ -45,8 +46,9 @@ describe('Notifications Controller', function () {
     let controller: NotificationsController
     let fetcher: TestFetcher
 
-    let ruleEngineSpy: sinon.SinonSpy
-    let focusPanelSpy: sinon.SinonSpy
+    const ruleEngineSpy = sinon.spy(ruleEngine, 'shouldDisplayNotification')
+    const focusPanelSpy = sinon.spy(panelNode, 'focusPanel')
+    const setContextSpy = sinon.spy(setContext, 'setContext')
 
     function dismissNotification(notification: ToolkitNotification) {
         // We could call `controller.dismissNotification()`, but this emulates a call
@@ -89,10 +91,7 @@ describe('Notifications Controller', function () {
     beforeEach(async function () {
         panelNode.setNotifications([], [])
         fetcher = new TestFetcher()
-        controller = new NotificationsController(panelNode, fetcher)
-
-        ruleEngineSpy = sinon.spy(ruleEngine, 'shouldDisplayNotification')
-        focusPanelSpy = sinon.spy(panelNode, 'focusPanel')
+        controller = new NotificationsController(panelNode, fetcher, '_aws.test.notification' as any)
 
         await globals.globalState.update(controller.storageKey, {
             startUp: {} as NotificationData,
@@ -102,8 +101,14 @@ describe('Notifications Controller', function () {
     })
 
     afterEach(function () {
-        ruleEngineSpy.restore()
+        ruleEngineSpy.resetHistory()
+        focusPanelSpy.resetHistory()
+        setContextSpy.resetHistory()
+    })
+
+    after(function () {
         focusPanelSpy.restore()
+        setContextSpy.restore()
     })
 
     it('can fetch and store startup notifications', async function () {
@@ -135,6 +140,7 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.startUpNotifications, [content.notifications[0]])
         assert.equal(panelNode.getChildren().length, 1)
         assert.equal(focusPanelSpy.callCount, 0)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
     })
 
     it('can fetch and store emergency notifications', async function () {
@@ -166,6 +172,7 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.emergencyNotifications, [content.notifications[0]])
         assert.equal(panelNode.getChildren().length, 1)
         assert.equal(focusPanelSpy.callCount, 1)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
     })
 
     it('can fetch and store both startup and emergency notifications', async function () {
@@ -224,6 +231,7 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.emergencyNotifications, [emergencyContent.notifications[0]])
         assert.equal(panelNode.getChildren().length, 2)
         assert.equal(focusPanelSpy.callCount, 1)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
     })
 
     it('dismisses a startup notification', async function () {
@@ -241,6 +249,7 @@ describe('Notifications Controller', function () {
 
         assert.equal(panelNode.getChildren().length, 2)
         assert.equal(panelNode.startUpNotifications.length, 2)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
 
         assert.deepStrictEqual(await globals.globalState.get(controller.storageKey), {
             startUp: {
@@ -281,9 +290,11 @@ describe('Notifications Controller', function () {
 
         await controller.pollForStartUp(ruleEngine)
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
 
         await dismissNotification(content.notifications[0])
         assert.equal(panelNode.getChildren().length, 0)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', false])
 
         content.notifications.push(getValidTestNotification('id:startup2'))
         fetcher.setStartUpContent({
@@ -305,6 +316,7 @@ describe('Notifications Controller', function () {
         })
 
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
     })
 
     it('does not refocus emergency notifications', async function () {
@@ -434,6 +446,7 @@ describe('Notifications Controller', function () {
             newlyReceived: [],
         })
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
 
         fetcher.setEmergencyContent({
             eTag: '1',
@@ -455,6 +468,7 @@ describe('Notifications Controller', function () {
         })
 
         assert.equal(panelNode.getChildren().length, 0)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', false])
     })
 
     it('does not rethrow errors when fetching', async function () {
diff --git a/packages/core/src/test/notifications/rendering.test.ts b/packages/core/src/test/notifications/rendering.test.ts
@@ -12,28 +12,20 @@ import { panelNode } from './controller.test'
 import { getTestWindow } from '../shared/vscode/window'
 import * as VsCodeUtils from '../../shared/utilities/vsCodeUtils'
 import { assertTextEditorContains, installFakeClock } from '../testUtil'
+import { waitUntil } from '../../shared/utilities/timeoutUtils'
+import globals from '../../shared/extensionGlobals'
 
 describe('Notifications Rendering', function () {
     let sandbox: sinon.SinonSandbox
-    let clock: FakeTimers.InstalledClock
-
-    before(function () {
-        clock = installFakeClock()
-    })
 
     beforeEach(function () {
         sandbox = sinon.createSandbox()
     })
 
     afterEach(function () {
-        clock.reset()
         sandbox.restore()
     })
 
-    after(function () {
-        clock.uninstall()
-    })
-
     // util to test txt pop-up under different senarios
     async function verifyTxtNotification(notification: ToolkitNotification) {
         const expectedContent = notification.uiRenderInstructions.content['en-US'].description
@@ -103,14 +95,21 @@ describe('Notifications Rendering', function () {
             message.selectItem('Update and Reload')
         })
         const excuteCommandStub = sandbox.stub(vscode.commands, 'executeCommand').resolves()
+        const telemetrySpy = sandbox.spy(globals.telemetry, 'flushRecords')
         const notification = getModalNotification()
-        await panelNode.openNotification(notification)
 
         // Update and Reload is put on a timer so that other methods (e.g. telemetry) can finish.
-        await clock.tickAsync(2000)
+        const clock: FakeTimers.InstalledClock = installFakeClock()
+        await panelNode.openNotification(notification)
+
+        await clock.tickAsync(1000)
+        clock.uninstall()
+
+        await waitUntil(async () => excuteCommandStub.called, { interval: 5, timeout: 5000 })
 
         assert.ok(excuteCommandStub.calledWith('workbench.extensions.installExtension', 'aws.toolkit.fake.extension'))
         assert.ok(excuteCommandStub.calledWith('workbench.action.reloadWindow'))
+        assert.ok(telemetrySpy.calledOnce)
     })
 
     it('executes openURL type button', async function () {
