refactor(logging): cleanup #5937

- 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

CONTRIBUTING.md

@@ -216,6 +216,16 @@ To run tests against a specific folder in VSCode, do any one of:
         $Env:TEST_DIR = "../core/src/test/foo"; npm run test
         ```
 
+#### Run jscpd ("Copy-Paste Detection")
+
+If the "Copy-Paste Detection" CI job fails, you will find it useful to check things locally. To
+check a specific file:
+
+    npx jscpd --config .github/workflows/jscpd.json --pattern packages/…/src/foo.ts
+
+See the [jscpd cli documentation](https://github.com/kucherenko/jscpd/tree/master/apps/jscpd) for
+more options.
+
 ### Coverage report
 
 You can find the coverage report at `./coverage/amazonq/lcov-report/index.html` and `./coverage/toolkit/lcov-report/index.html` after running the tests. Tests ran from the workspace launch config won't generate a coverage report automatically because it can break file watching.

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)
@@ -125,36 +131,20 @@ export function getLogger(topic?: LogTopic): Logger {
     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']
     if (!logger) {
         return new ConsoleLogger()
     }
-    if (isToolkitLogger(logger)) {
-        logger.setTopic(topic)
-    }
+    ;(logger as BaseLogger).setTopic?.(topic)
     return logger
 }
 
+// jscpd:ignore-start
 export class NullLogger extends BaseLogger {
     public setLogLevel(logLevel: LogLevel) {}
     public logLevelEnabled(logLevel: LogLevel): boolean {
@@ -169,6 +159,9 @@ export class NullLogger extends BaseLogger {
         message: string | Error,
         ...meta: any[]
     ): number {
+        void logLevel
+        void message
+        void meta
         return 0
     }
 }
@@ -190,10 +183,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)
@@ -204,10 +194,12 @@ export class ConsoleLogger extends BaseLogger {
         return 0
     }
 }
+// jscpd:ignore-end
 
 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'