From 4341ce4bf14d460626b7a3c0d7a996c87bec5008 Mon Sep 17 00:00:00 2001 From: stevengum <14935595+stevengum@users.noreply.github.com> Date: Sun, 2 Dec 2018 00:20:03 -0800 Subject: [PATCH 1/2] add console.error for async TranscriptLogger.logActivity() impl in TranscriptLoggerMiddleware --- .../botbuilder-core/src/transcriptLogger.ts | 17 ++++++- .../tests/transcriptMiddleware.test.js | 44 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/libraries/botbuilder-core/src/transcriptLogger.ts b/libraries/botbuilder-core/src/transcriptLogger.ts index 1e89d4ad13..08a00196bb 100644 --- a/libraries/botbuilder-core/src/transcriptLogger.ts +++ b/libraries/botbuilder-core/src/transcriptLogger.ts @@ -92,7 +92,22 @@ export class TranscriptLoggerMiddleware implements Middleware { while (this.transcript.length > 0) { try { const activity: Activity = this.transcript.shift(); - this.logger.logActivity(activity); + // If the implementation of this.logger.logActivity() is asynchronous, we don't + // await it as to not block processing of activities. + // Because TranscriptLogger.logActivity() returns void or Promise, we capture + // the result and see if it is a Promise. + const logActivityResult = this.logger.logActivity(activity); + + // If this.logger.logActivity() returns a Promise, a catch is added in case there + // is no innate error handling in the method. This catch prevents + // UnhandledPromiseRejectionWarnings from being thrown and prints the error to the + // console. + if (logActivityResult instanceof Promise) { + logActivityResult.catch(err => { + // tslint:disable-next-line:no-console + console.error('TranscriptLoggerMiddleware logActivity failed', err); + }); + } } catch (err) { // tslint:disable-next-line:no-console console.error('TranscriptLoggerMiddleware logActivity failed', err); diff --git a/libraries/botbuilder-core/tests/transcriptMiddleware.test.js b/libraries/botbuilder-core/tests/transcriptMiddleware.test.js index dad368ea8f..8ed4fe22a8 100644 --- a/libraries/botbuilder-core/tests/transcriptMiddleware.test.js +++ b/libraries/botbuilder-core/tests/transcriptMiddleware.test.js @@ -1,6 +1,18 @@ const assert = require('assert'); const { TestAdapter, TranscriptLoggerMiddleware, MemoryTranscriptStore, ActivityTypes } = require('../'); +class SyncErrorLogger { + logActivity(activity) { + throw new Error(); + } +} + +class AsyncErrorLogger { + async logActivity(activity) { + throw new Error(); + } +} + describe(`TranscriptLoggerMiddleware`, function () { this.timeout(5000); @@ -110,6 +122,38 @@ describe(`TranscriptLoggerMiddleware`, function () { }); }) }); + + describe('\'s error handling', function () { + const originalConsoleError = console.error; + + this.afterEach(function () { + console.error = originalConsoleError; + }) + + function stubConsoleError(done) { + console.error = function (error) { + assert(typeof error === 'string', `unexpected typeof arg passed to console.error: "typeof ${ typeof error }"`); + assert(error.startsWith('TranscriptLoggerMiddleware logActivity failed'), `unexpected error message received: ${ error }`); + done(); + } + } + + it(`should print to console errors from synchronous logActivity()`, function (done) { + stubConsoleError(done); + var adapter = new TestAdapter(async (context) => { + }).use(new TranscriptLoggerMiddleware(new SyncErrorLogger())); + + adapter.send('test'); + }); + + it(`should print to console errors from asynchronous logActivity().`, function (done) { + stubConsoleError(done); + var adapter = new TestAdapter(async (context) => { + }).use(new TranscriptLoggerMiddleware(new AsyncErrorLogger())); + + adapter.send('test'); + }); + }); }); From 68bc4a6f3108b090b7437d7b2f390e50d864274d Mon Sep 17 00:00:00 2001 From: stevengum <14935595+stevengum@users.noreply.github.com> Date: Wed, 5 Dec 2018 10:31:17 -0800 Subject: [PATCH 2/2] apply PR #669 feedback, add new tests --- .../botbuilder-core/src/transcriptLogger.ts | 21 ++++++-- .../tests/transcriptMiddleware.test.js | 54 ++++++++++++++++--- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/libraries/botbuilder-core/src/transcriptLogger.ts b/libraries/botbuilder-core/src/transcriptLogger.ts index 08a00196bb..196230deec 100644 --- a/libraries/botbuilder-core/src/transcriptLogger.ts +++ b/libraries/botbuilder-core/src/transcriptLogger.ts @@ -104,13 +104,11 @@ export class TranscriptLoggerMiddleware implements Middleware { // console. if (logActivityResult instanceof Promise) { logActivityResult.catch(err => { - // tslint:disable-next-line:no-console - console.error('TranscriptLoggerMiddleware logActivity failed', err); + this.transcriptLoggerErrorHandler(err); }); } } catch (err) { - // tslint:disable-next-line:no-console - console.error('TranscriptLoggerMiddleware logActivity failed', err); + this.transcriptLoggerErrorHandler(err); } } } @@ -134,6 +132,21 @@ export class TranscriptLoggerMiddleware implements Middleware { private cloneActivity(activity: Partial): Activity { return Object.assign({}, activity); } + + /** + * Error logging helper function. + * @param err Error or object to console.error out. + */ + private transcriptLoggerErrorHandler(err: Error|any): void { + // tslint:disable:no-console + if (err instanceof Error) { + console.error(`TranscriptLoggerMiddleware logActivity failed: "${ err.message }"`); + console.error(err.stack); + } else { + console.error(`TranscriptLoggerMiddleware logActivity failed: "${ JSON.stringify(err) }"`); + } + // tslint:enable:no-console + } } /** diff --git a/libraries/botbuilder-core/tests/transcriptMiddleware.test.js b/libraries/botbuilder-core/tests/transcriptMiddleware.test.js index 8ed4fe22a8..d6e84b6f89 100644 --- a/libraries/botbuilder-core/tests/transcriptMiddleware.test.js +++ b/libraries/botbuilder-core/tests/transcriptMiddleware.test.js @@ -13,6 +13,19 @@ class AsyncErrorLogger { } } +class SyncThrowerLogger { + logActivity(activity) { + throw 1; + } +} + +class AsyncThrowerLogger { + async logActivity(activity) { + throw 2; + } +} + + describe(`TranscriptLoggerMiddleware`, function () { this.timeout(5000); @@ -130,16 +143,29 @@ describe(`TranscriptLoggerMiddleware`, function () { console.error = originalConsoleError; }) - function stubConsoleError(done) { + function stubConsoleError(done, expectedErrorMessage, expectedErrors = 1) { + let errorCounter = 0; console.error = function (error) { - assert(typeof error === 'string', `unexpected typeof arg passed to console.error: "typeof ${ typeof error }"`); - assert(error.startsWith('TranscriptLoggerMiddleware logActivity failed'), `unexpected error message received: ${ error }`); - done(); + // We only care about the error message on the first error. + if (errorCounter === 0) { + if (expectedErrorMessage) { + assert(error.startsWith(expectedErrorMessage), `unexpected error message received: ${ error }`); + } else { + assert(error.startsWith('TranscriptLoggerMiddleware logActivity failed'), `unexpected error message received: ${ error }`); + } + } + errorCounter++; + + if (expectedErrors === errorCounter) { + done(); + } if (errorCounter > expectedErrors) { + throw new Error(`console.error() called too many times.`); + } } } it(`should print to console errors from synchronous logActivity()`, function (done) { - stubConsoleError(done); + stubConsoleError(done, undefined, 2); var adapter = new TestAdapter(async (context) => { }).use(new TranscriptLoggerMiddleware(new SyncErrorLogger())); @@ -147,12 +173,28 @@ describe(`TranscriptLoggerMiddleware`, function () { }); it(`should print to console errors from asynchronous logActivity().`, function (done) { - stubConsoleError(done); + stubConsoleError(done, undefined, 2); var adapter = new TestAdapter(async (context) => { }).use(new TranscriptLoggerMiddleware(new AsyncErrorLogger())); adapter.send('test'); }); + + it(`should print to console thrown info from synchronous logActivity()`, function (done) { + stubConsoleError(done, 'TranscriptLoggerMiddleware logActivity failed: "1"'); + var adapter = new TestAdapter(async (context) => { + }).use(new TranscriptLoggerMiddleware(new SyncThrowerLogger())); + + adapter.send('test'); + }); + + it(`should print to console thrown info from asynchronous logActivity().`, function (done) { + stubConsoleError(done, 'TranscriptLoggerMiddleware logActivity failed: "2"'); + var adapter = new TestAdapter(async (context) => { + }).use(new TranscriptLoggerMiddleware(new AsyncThrowerLogger())); + + adapter.send('test'); + }); }); });