Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add console.error for async TranscriptLogger.logActivity() impl in TranscriptLoggerMiddleware #669

Merged
merged 3 commits into from
Dec 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions libraries/botbuilder-core/src/transcriptLogger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,23 @@ 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<void>, 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 => {
this.transcriptLoggerErrorHandler(err);
});
}
} catch (err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something of a nit.

If this err object is of type Error (which it should be but isn't necessarily) then this line will do a toString which will write only a subset of the information being carried to the console.

Wouldn't it therefore be more correct to explicitly check if it was of type Error and if it was then print out more of the fields, otherwise just toString()? (Incidentally, if I remember correctly I don't think JSON.stringify will work on Error.)

In this particular case I'm not sure this matters to much because its just the console. But something to consider in general.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

try {
  const activity: Activity = this.transcript.shift();
  // Comments comments comments...
  const logActivityResult = this.logger.logActivity(activity);
  // Comments comments comments...

  if (logActivityResult.catch(err => {
    this.transcriptLoggerErrorHandler(err);
  });
} catch (err) {
  this.transcriptLoggerErrorHandler(err);
}

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
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice

// tslint:disable-next-line:no-console
console.error('TranscriptLoggerMiddleware logActivity failed', err);
this.transcriptLoggerErrorHandler(err);
}
}
}
Expand All @@ -119,6 +132,21 @@ export class TranscriptLoggerMiddleware implements Middleware {
private cloneActivity(activity: Partial<Activity>): Activity {
return Object.assign(<Activity>{}, 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
}
}

/**
Expand Down
86 changes: 86 additions & 0 deletions libraries/botbuilder-core/tests/transcriptMiddleware.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,31 @@
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();
}
}

class SyncThrowerLogger {
logActivity(activity) {
throw 1;
}
}

class AsyncThrowerLogger {
async logActivity(activity) {
throw 2;
}
}


describe(`TranscriptLoggerMiddleware`, function () {
this.timeout(5000);

Expand Down Expand Up @@ -110,6 +135,67 @@ describe(`TranscriptLoggerMiddleware`, function () {
});
})
});

describe('\'s error handling', function () {
const originalConsoleError = console.error;

this.afterEach(function () {
console.error = originalConsoleError;
})

function stubConsoleError(done, expectedErrorMessage, expectedErrors = 1) {
let errorCounter = 0;
console.error = function (error) {
// 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, undefined, 2);
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, 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');
});
});
});


Expand Down