Tests now use a Test Logger (#781)

* Start of TestLogger

* WIP - Remove existing TestLogger, tests now use a new test logger not tied to the main (winston) logger

* wip - logger test migration

* wip - test migration

* wip - test convergence

* wip - test convergence

* Impl TestLogger

* Fix tests

* Comment removal

* Code clenaup

* Added test messages

* Remove initialize call from test

* lint fix

Author: awschristou

src/shared/logger/loggableType.ts

@@ -7,3 +7,7 @@
  * Loggable types: Error, string
  */
 export type Loggable = Error | string
+
+export function isLoggableError(loggable: Loggable): boolean {
+    return loggable instanceof Error
+}

src/shared/logger/logger.ts

@@ -5,7 +5,7 @@
 
 import { Loggable } from './loggableType'
 
-let toolkitLogger: Logger
+let toolkitLogger: Logger | undefined
 
 export interface Logger {
     debug(...message: Loggable[]): void
@@ -18,15 +18,23 @@ export interface Logger {
 export type LogLevel = keyof Logger
 
 /**
- * Gets the default logger if it has been initialized with the initialize() function
+ * Gets the logger if it has been initialized
  */
 export function getLogger(): Logger {
-    if (toolkitLogger) {
-        return toolkitLogger
+    if (!toolkitLogger) {
+        throw new Error(
+            'Logger not initialized. Extension code should call initialize() from shared/logger/activation, test code should call setLogger().'
+        )
     }
-    throw new Error('Default Logger not initialized. Call logger.initialize() first.')
+
+    return toolkitLogger
 }
 
-export function setLogger(logger: Logger) {
+/**
+ * Sets (or clears) the logger that is accessible to code.
+ * The Extension is expected to call this only once.
+ * Tests should call this to set up a logger prior to executing code that accesses a logger.
+ */
+export function setLogger(logger: Logger | undefined) {
     toolkitLogger = logger
 }

src/shared/loggerUtils.ts

@@ -6,7 +6,6 @@
 import * as assert from 'assert'
 import { AwsExplorer } from '../../awsexplorer/awsExplorer'
 import { AwsContextTreeCollection } from '../../shared/awsContextTreeCollection'
-import { TestLogger } from '../../shared/loggerUtils'
 import { RegionNode } from '../../shared/treeview/nodes/regionNode'
 import { MockOutputChannel } from '../mockOutputChannel'
 import {
@@ -18,16 +17,6 @@ import {
 } from '../utilities/fakeAwsContext'
 
 describe('AwsExplorer', () => {
-    let logger: TestLogger
-
-    before(async () => {
-        logger = await TestLogger.createTestLogger()
-    })
-
-    after(async () => {
-        await logger.cleanupLogger()
-    })
-
     it('displays region nodes with user-friendly region names', async () => {
         const awsContext = new FakeAwsContext()
         const regionProvider = new FakeRegionProvider()

src/test/awsExplorer/awsExplorer.test.ts

@@ -0,0 +1,50 @@
+/*!
+ * Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * Before/After hooks for all "unit" tests
+ */
+
+import * as assert from 'assert'
+import { getLogger } from '../shared/logger'
+import { setLogger } from '../shared/logger/logger'
+import { TestLogger } from './testLogger'
+
+// Expectation: Tests are not run concurrently
+let testLogger: TestLogger | undefined
+
+beforeEach(async () => {
+    // Set every test up so that TestLogger is the logger used by toolkit code
+    testLogger = setupTestLogger()
+})
+
+afterEach(async () => {
+    // Prevent other tests from using the same TestLogger instance
+    teardownTestLogger()
+    testLogger = undefined
+})
+
+/**
+ * Provides the TestLogger to tests that want to access it.
+ * Verifies that the TestLogger instance is still the one set as the toolkit's logger.
+ */
+export function getTestLogger(): TestLogger {
+    const logger = getLogger()
+    assert.strictEqual(logger, testLogger, 'The expected test logger is not the current logger')
+    assert.ok(testLogger, 'TestLogger was expected to exist')
+
+    return testLogger!
+}
+
+function setupTestLogger(): TestLogger {
+    const logger = new TestLogger()
+    setLogger(logger)
+
+    return logger
+}
+
+function teardownTestLogger() {
+    setLogger(undefined)
+}

src/test/globalSetup.test.ts

@@ -16,7 +16,6 @@ import {
 import { SamDeployWizardResponse } from '../../../lambda/wizards/samDeployWizard'
 import { AwsContext } from '../../../shared/awsContext'
 import { makeTemporaryToolkitFolder } from '../../../shared/filesystemUtilities'
-import { TestLogger } from '../../../shared/loggerUtils'
 import { RegionInfo } from '../../../shared/regions/regionInfo'
 import { RegionProvider } from '../../../shared/regions/regionProvider'
 import { SamCliContext } from '../../../shared/sam/cli/samCliContext'
@@ -136,10 +135,8 @@ describe('deploySamApplication', async () => {
 
     const extContext = new FakeExtensionContext()
 
-    let logger: TestLogger
     let tempToolkitFolder: string
     beforeEach(async () => {
-        logger = await TestLogger.createTestLogger()
         channelLogger = new FakeChannelLogger()
 
         tempToolkitFolder = await makeTemporaryToolkitFolder()
@@ -160,7 +157,6 @@ describe('deploySamApplication', async () => {
     })
 
     afterEach(async () => {
-        await logger.cleanupLogger()
         await del([tempToolkitFolder], { force: true })
     })
 
@@ -358,9 +354,9 @@ function assertGeneralErrorLogged(channelLogger: FakeChannelLogger) {
 
 function assertErrorLogsContain(text: string, channelLogger: FakeChannelLogger, exactMatch: boolean) {
     assert.ok(
-        channelLogger.logger.errorEntries.some(
-            e => e instanceof Error && (exactMatch ? e.message === text : e.message.indexOf(text) !== -1)
-        ),
+        channelLogger.logger
+            .getLoggedEntries('error')
+            .some(e => e instanceof Error && (exactMatch ? e.message === text : e.message.indexOf(text) !== -1)),
         `Expected to find ${text} in the error logs`
     )
 }

src/test/lambda/commands/deploySamApplication.test.ts

@@ -18,7 +18,6 @@ import { EcsClient } from '../../../shared/clients/ecsClient'
 import { LambdaClient } from '../../../shared/clients/lambdaClient'
 import { StsClient } from '../../../shared/clients/stsClient'
 import { ext } from '../../../shared/extensionGlobals'
-import { TestLogger } from '../../../shared/loggerUtils'
 import { RegionInfo } from '../../../shared/regions/regionInfo'
 import { ErrorNode } from '../../../shared/treeview/nodes/errorNode'
 import { PlaceholderNode } from '../../../shared/treeview/nodes/placeholderNode'
@@ -31,11 +30,9 @@ async function* asyncGenerator<T>(items: T[]): AsyncIterableIterator<T> {
 
 describe('DefaultCloudFormationStackNode', () => {
     let fakeStackSummary: CloudFormation.StackSummary
-    let logger: TestLogger
 
     before(async () => {
         setupTestIconPaths()
-        logger = await TestLogger.createTestLogger()
         fakeStackSummary = {
             CreationTime: new Date(),
             StackId: '1',
@@ -46,7 +43,6 @@ describe('DefaultCloudFormationStackNode', () => {
 
     after(async () => {
         clearTestIconPaths()
-        await logger.cleanupLogger()
     })
 
     // Validates we tagged the node correctly.
@@ -213,16 +209,6 @@ describe('DefaultCloudFormationStackNode', () => {
 })
 
 describe('DefaultCloudFormationNode', () => {
-    let logger: TestLogger
-
-    before(async () => {
-        logger = await TestLogger.createTestLogger()
-    })
-
-    after(async () => {
-        await logger.cleanupLogger()
-    })
-
     class StackNamesMockCloudFormationClient extends MockCloudFormationClient {
         public constructor(
             public readonly stackNames: string[] = [],

src/test/lambda/explorer/cloudFormationNodes.test.ts

@@ -17,7 +17,6 @@ import { EcsClient } from '../../../shared/clients/ecsClient'
 import { LambdaClient } from '../../../shared/clients/lambdaClient'
 import { StsClient } from '../../../shared/clients/stsClient'
 import { ext } from '../../../shared/extensionGlobals'
-import { TestLogger } from '../../../shared/loggerUtils'
 import { RegionInfo } from '../../../shared/regions/regionInfo'
 import { ErrorNode } from '../../../shared/treeview/nodes/errorNode'
 import { MockLambdaClient } from '../../shared/clients/mockClients'
@@ -30,11 +29,9 @@ async function* asyncGenerator<T>(items: T[]): AsyncIterableIterator<T> {
 
 describe('DefaultLambdaFunctionNode', () => {
     let fakeFunctionConfig: Lambda.FunctionConfiguration
-    let logger: TestLogger
 
     before(async () => {
         setupTestIconPaths()
-        logger = await TestLogger.createTestLogger()
         fakeFunctionConfig = {
             FunctionName: 'testFunctionName',
             FunctionArn: 'testFunctionARN'
@@ -43,7 +40,6 @@ describe('DefaultLambdaFunctionNode', () => {
 
     after(async () => {
         clearTestIconPaths()
-        await logger.cleanupLogger()
     })
 
     // Validates we tagged the node correctly
@@ -92,16 +88,6 @@ describe('DefaultLambdaFunctionNode', () => {
 })
 
 describe('DefaultLambdaFunctionGroupNode', () => {
-    let logger: TestLogger
-
-    before(async () => {
-        logger = await TestLogger.createTestLogger()
-    })
-
-    after(async () => {
-        await logger.cleanupLogger()
-    })
-
     class FunctionNamesMockLambdaClient extends MockLambdaClient {
         public constructor(
             public readonly functionNames: string[] = [],

src/test/lambda/explorer/lambdaNodes.test.ts

@@ -11,57 +11,15 @@ import { DebugConfiguration } from '../../../lambda/local/debugConfiguration'
 import * as localLambdaRunner from '../../../shared/codelens/localLambdaRunner'
 import * as fs from '../../../shared/filesystem'
 import * as fsUtils from '../../../shared/filesystemUtilities'
-import { Loggable, Logger } from '../../../shared/logger'
-import { TestLogger } from '../../../shared/loggerUtils'
 import { ChildProcessResult } from '../../../shared/utilities/childProcess'
 import { ExtensionDisposableFiles } from '../../../shared/utilities/disposableFiles'
-import { ChannelLogger } from '../../../shared/utilities/vsCodeUtils'
 import { FakeExtensionContext } from '../../fakeExtensionContext'
+import { FakeChannelLogger } from '../fakeChannelLogger'
 import { assertRejects } from '../utilities/assertUtils'
 
-class FakeChannelLogger implements Pick<ChannelLogger, 'info' | 'error' | 'logger'> {
-    public readonly loggedInfoKeys: Set<string> = new Set<string>()
-    public readonly loggedErrorKeys: Set<string> = new Set<string>()
-    public readonly logger: FakeLogger = new FakeLogger()
-
-    public info(nlsKey: string, nlsTemplate: string, ...templateTokens: Loggable[]): void {
-        this.loggedInfoKeys.add(nlsKey)
-    }
-
-    public error(nlsKey: string, nlsTemplate: string, ...templateTokens: Loggable[]): void {
-        this.loggedErrorKeys.add(nlsKey)
-    }
-}
-
-class FakeLogger implements Logger {
-    public readonly loggedDebugEntries: Loggable[] = []
-
-    public debug(...message: Loggable[]): void {
-        this.loggedDebugEntries.push(...message)
-    }
-
-    public verbose(...message: Loggable[]): void {
-        throw new Error('verbose() not used')
-    }
-
-    public info(...message: Loggable[]): void {
-        throw new Error('info() not used')
-    }
-
-    public warn(...message: Loggable[]): void {
-        throw new Error('warn() not used')
-    }
-
-    public error(...message: Loggable[]): void {
-        throw new Error('error() not used')
-    }
-}
-
 describe('localLambdaRunner', async () => {
-    let logger: TestLogger
     let tempDir: string
     before(async () => {
-        logger = await TestLogger.createTestLogger()
         await ExtensionDisposableFiles.initialize(new FakeExtensionContext())
     })
 
@@ -73,10 +31,6 @@ describe('localLambdaRunner', async () => {
         await del(tempDir, { force: true })
     })
 
-    after(async () => {
-        await logger.cleanupLogger()
-    })
-
     describe('attachDebugger', async () => {
         let actualRetries: number = 0
         let channelLogger: FakeChannelLogger