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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4341ce4
add console.error for async TranscriptLogger.logActivity() impl in Tr…
stevengum Dec 2, 2018
68bc4a6
apply PR #669 feedback, add new tests
stevengum Dec 5, 2018
fc4ddb7
Merge branch 'master' into stgum/fix383
johnataylor Dec 5, 2018
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
17 changes: 16 additions & 1 deletion libraries/botbuilder-core/src/transcriptLogger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<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 => {
// tslint:disable-next-line:no-console
console.error('TranscriptLoggerMiddleware logActivity failed', 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);
Expand Down
44 changes: 44 additions & 0 deletions libraries/botbuilder-core/tests/transcriptMiddleware.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);

Expand Down Expand Up @@ -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');
});
});
});


Expand Down