refactor(logging) cleanup

- deduplicate tests
- define setTopic on `BaseLogger`, not `ToolkitLogger`
  - it's not specific to ToolkitLogger, so it should live in the base interface.
- define `LogTopic` in `logger.ts`, not `ToolkitLogger`
  - it's not specific to ToolkitLogger, so it should live in the base interface.

Author: justinmk3

packages/core/src/shared/logger/logger.ts

@@ -4,7 +4,8 @@
  */
 
 import * as vscode from 'vscode'
-import { LogTopic, type ToolkitLogger } from './toolkitLogger'
+
+export type LogTopic = 'crashReport' | 'notifications' | 'test' | 'unknown'
 
 const toolkitLoggers: {
     main: Logger | undefined
@@ -33,6 +34,11 @@ export interface Logger {
 
 export abstract class BaseLogger implements Logger {
     logFile?: vscode.Uri
+    topic: LogTopic = 'unknown'
+
+    setTopic(topic: LogTopic = 'unknown') {
+        this.topic = topic
+    }
 
     debug(message: string | Error, ...meta: any[]): number {
         return this.sendToLog('debug', message, ...meta)
@@ -121,37 +127,20 @@ export function compareLogLevel(l1: LogLevel, l2: LogLevel): number {
  * @param topic: topic to be appended in front of the message.
  */
 export function getLogger(topic?: LogTopic): Logger {
-    const logger = toolkitLoggers['main']
+    const logger = structuredClone(toolkitLoggers['main'])
     if (!logger) {
         return new ConsoleLogger()
     }
-    /**
-     * need this check so that setTopic can be recognized
-     * using instanceof would lead to dependency loop error
-     */
-    if (isToolkitLogger(logger)) {
-        logger.setTopic(topic)
-    }
+    ;(logger as BaseLogger).setTopic(topic)
     return logger
 }
 
-/**
- * check if the logger is of type `ToolkitLogger`. This avoids Circular Dependencies, but `instanceof ToolkitLogger` is preferred.
- * @param logger
- * @returns bool, true if is `ToolkitLogger`
- */
-function isToolkitLogger(logger: Logger): logger is ToolkitLogger {
-    return 'setTopic' in logger && typeof logger.setTopic === 'function'
-}
-
 export function getDebugConsoleLogger(topic?: LogTopic): Logger {
-    const logger = toolkitLoggers['debugConsole']
+    const logger = structuredClone(toolkitLoggers['debugConsole'])
     if (!logger) {
         return new ConsoleLogger()
     }
-    if (isToolkitLogger(logger)) {
-        logger.setTopic(topic)
-    }
+    ;(logger as BaseLogger).setTopic(topic)
     return logger
 }
 
@@ -169,6 +158,9 @@ export class NullLogger extends BaseLogger {
         message: string | Error,
         ...meta: any[]
     ): number {
+        void logLevel
+        void message
+        void meta
         return 0
     }
 }
@@ -190,10 +182,7 @@ export class ConsoleLogger extends BaseLogger {
         message: string | Error,
         ...meta: any[]
     ): number {
-        /**
-         * This is here because we pipe verbose to debug currentlly
-         * TODO: remove in the next stage
-         */
+        // TODO: we alias "verbose" to "debug" currently. Will be revisited: IDE-14839
         if (logLevel === 'verbose') {
             // eslint-disable-next-line aws-toolkits/no-console-log
             console.debug(message, ...meta)
@@ -208,6 +197,7 @@ export class ConsoleLogger extends BaseLogger {
 export function getNullLogger(type?: 'debugConsole' | 'main'): Logger {
     return new NullLogger()
 }
+
 /**
  * Sets (or clears) the logger that is accessible to code.
  * The Extension is expected to call this only once per log type.

packages/core/src/shared/logger/toolkitLogger.ts

@@ -14,9 +14,6 @@ import { SharedFileTransport } from './sharedFileTransport'
 import { ConsoleLogTransport } from './consoleLogTransport'
 import { isWeb } from '../extensionGlobals'
 
-/* define log topics */
-export type LogTopic = 'unknown' | 'test' | 'crashReport' | 'notifications'
-
 class ErrorLog {
     constructor(
         public topic: string,
@@ -30,7 +27,6 @@ const logmapSize: number = 1000
 export class ToolkitLogger extends BaseLogger implements vscode.Disposable {
     private readonly logger: winston.Logger
     /* topic is used for header in log messages, default is 'Unknown' */
-    private topic: LogTopic = 'unknown'
     private disposed: boolean = false
     private idCounter: number = 0
     private logMap: { [logID: number]: { [filePath: string]: string } } = {}
@@ -115,16 +111,10 @@ export class ToolkitLogger extends BaseLogger implements vscode.Disposable {
               })
     }
 
-    public setTopic(topic: LogTopic = 'unknown') {
-        this.topic = topic
-    }
-
     /* Format the message with topic header */
     private addTopicToMessage(message: string | Error): string | ErrorLog {
         if (typeof message === 'string') {
-            /*We shouldn't print unknow before current logging calls are migrated
-             * TODO: remove this once migration of current calls is completed
-             */
+            // TODO: remove this after all calls are migrated and topic is a required param.
             if (this.topic === 'unknown') {
                 return message
             }

packages/core/src/test/shared/logger/toolkitLogger.test.ts

@@ -239,32 +239,26 @@ describe('ToolkitLogger', function () {
             assert.ok(!(await waitForMessage).includes(nonLoggedVerboseEntry), 'unexpected message in log')
         })
 
-        it('logs append topic header in message', async function () {
-            const testMessage = 'This is a test message'
-            const testMessageWithHeader = 'test: This is a test message'
-
+        it('prepends topic to message', async function () {
             testLogger = new ToolkitLogger('info')
             testLogger.logToOutputChannel(outputChannel, false)
             testLogger.setTopic('test')
             testLogger.setLogLevel('verbose')
-            testLogger.verbose(testMessage)
-
-            const waitForMessage = waitForLoggedTextByContents(testMessageWithHeader)
-            assert.ok((await waitForMessage).includes(testMessageWithHeader), 'Expected header added')
-        })
 
-        it('logs append topic header in errors', async function () {
-            const testError = new ToolkitError('root error', { code: 'something went wrong' })
-            const testErrorWithHeader = "topic: 'test'"
+            const testMessageWithHeader = 'test: This is a test message'
+            testLogger.verbose('This is a test message')
+            assert.ok(
+                (await waitForLoggedTextByContents(testMessageWithHeader)).includes(testMessageWithHeader),
+                'Expected header added'
+            )
 
-            testLogger = new ToolkitLogger('info')
-            testLogger.logToOutputChannel(outputChannel, false)
-            testLogger.setTopic('test')
-            testLogger.setLogLevel('verbose')
-            testLogger.verbose(testError)
+            const msg = "topic: 'test'"
+            testLogger.verbose(new ToolkitError('root error', { code: 'something went wrong' }))
+            assert.ok((await waitForLoggedTextByContents(msg)).includes(msg), 'Expected header added')
 
-            const waitForMessage = waitForLoggedTextByContents(testErrorWithHeader)
-            assert.ok((await waitForMessage).includes(testErrorWithHeader), 'Expected header added')
+            const msg2 = "topic: 'test'"
+            testLogger.verbose(new ToolkitError('root error', { code: 'something went wrong' }))
+            assert.ok((await waitForLoggedTextByContents(msg2)).includes(msg2), 'Expected header added')
         })
 
         it('unknown topic header ignored in message', async function () {
@@ -282,7 +276,7 @@ describe('ToolkitLogger', function () {
             assert.ok(!(await waitForMessage).includes(unknowntestMessage), 'unexpected header in log')
         })
 
-        it('switch topic within same logger', async function () {
+        it('switch topic on same logger', async function () {
             const testMessage = 'This is a test message'
             const testMessageWithHeader = 'test: This is a test message'