From b4631b0dd164ee34de9f5dff43a1bf559880ebd0 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Tue, 30 Apr 2024 12:58:16 -0400 Subject: [PATCH] Ps/improve-log-service (#8989) * Match console method signatures in logService abstraction * Add a few usages of improved signature * Remove reality check test * Improve electron logging --- .../src/background/runtime.background.ts | 3 +- .../offscreen-document.spec.ts | 4 +- .../offscreen-document/offscreen-document.ts | 2 +- .../services/console-log.service.spec.ts | 36 ++++++------ .../platform/services/console-log.service.ts | 6 +- apps/desktop/src/platform/preload.ts | 3 +- .../services/electron-log.main.service.ts | 14 ++--- .../services/electron-log.renderer.service.ts | 14 +++-- .../services/logging-error-handler.ts | 2 +- libs/common/spec/intercept-console.ts | 23 +++----- .../src/platform/abstractions/log.service.ts | 10 ++-- .../services/console-log.service.spec.ts | 57 ++++++++++--------- .../platform/services/console-log.service.ts | 26 ++++----- 13 files changed, 104 insertions(+), 96 deletions(-) diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index 98b1df9c80..d8f3cf840f 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -76,7 +76,8 @@ export default class RuntimeBackground { void this.processMessageWithSender(msg, sender).catch((err) => this.logService.error( - `Error while processing message in RuntimeBackground '${msg?.command}'. Error: ${err?.message ?? "Unknown Error"}`, + `Error while processing message in RuntimeBackground '${msg?.command}'.`, + err, ), ); return false; diff --git a/apps/browser/src/platform/offscreen-document/offscreen-document.spec.ts b/apps/browser/src/platform/offscreen-document/offscreen-document.spec.ts index 1cbcc7a94c..933cd08c2e 100644 --- a/apps/browser/src/platform/offscreen-document/offscreen-document.spec.ts +++ b/apps/browser/src/platform/offscreen-document/offscreen-document.spec.ts @@ -28,6 +28,7 @@ describe("OffscreenDocument", () => { }); it("shows a console message if the handler throws an error", async () => { + const error = new Error("test error"); browserClipboardServiceCopySpy.mockRejectedValueOnce(new Error("test error")); sendExtensionRuntimeMessage({ command: "offscreenCopyToClipboard", text: "test" }); @@ -35,7 +36,8 @@ describe("OffscreenDocument", () => { expect(browserClipboardServiceCopySpy).toHaveBeenCalled(); expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error resolving extension message response: Error: test error", + "Error resolving extension message response", + error, ); }); diff --git a/apps/browser/src/platform/offscreen-document/offscreen-document.ts b/apps/browser/src/platform/offscreen-document/offscreen-document.ts index 627036b80b..4994a6e9ba 100644 --- a/apps/browser/src/platform/offscreen-document/offscreen-document.ts +++ b/apps/browser/src/platform/offscreen-document/offscreen-document.ts @@ -71,7 +71,7 @@ class OffscreenDocument implements OffscreenDocumentInterface { Promise.resolve(messageResponse) .then((response) => sendResponse(response)) .catch((error) => - this.consoleLogService.error(`Error resolving extension message response: ${error}`), + this.consoleLogService.error("Error resolving extension message response", error), ); return true; }; diff --git a/apps/cli/src/platform/services/console-log.service.spec.ts b/apps/cli/src/platform/services/console-log.service.spec.ts index 10a0ad8cca..03598b16e6 100644 --- a/apps/cli/src/platform/services/console-log.service.spec.ts +++ b/apps/cli/src/platform/services/console-log.service.spec.ts @@ -2,13 +2,18 @@ import { interceptConsole, restoreConsole } from "@bitwarden/common/spec"; import { ConsoleLogService } from "./console-log.service"; -let caughtMessage: any = {}; - describe("CLI Console log service", () => { + const error = new Error("this is an error"); + const obj = { a: 1, b: 2 }; let logService: ConsoleLogService; + let consoleSpy: { + log: jest.Mock; + warn: jest.Mock; + error: jest.Mock; + }; + beforeEach(() => { - caughtMessage = {}; - interceptConsole(caughtMessage); + consoleSpy = interceptConsole(); logService = new ConsoleLogService(true); }); @@ -19,24 +24,21 @@ describe("CLI Console log service", () => { it("should redirect all console to error if BW_RESPONSE env is true", () => { process.env.BW_RESPONSE = "true"; - logService.debug("this is a debug message"); - expect(caughtMessage).toMatchObject({ - error: { 0: "this is a debug message" }, - }); + logService.debug("this is a debug message", error, obj); + expect(consoleSpy.error).toHaveBeenCalledWith("this is a debug message", error, obj); }); it("should not redirect console to error if BW_RESPONSE != true", () => { process.env.BW_RESPONSE = "false"; - logService.debug("debug"); - logService.info("info"); - logService.warning("warning"); - logService.error("error"); + logService.debug("debug", error, obj); + logService.info("info", error, obj); + logService.warning("warning", error, obj); + logService.error("error", error, obj); - expect(caughtMessage).toMatchObject({ - log: { 0: "info" }, - warn: { 0: "warning" }, - error: { 0: "error" }, - }); + expect(consoleSpy.log).toHaveBeenCalledWith("debug", error, obj); + expect(consoleSpy.log).toHaveBeenCalledWith("info", error, obj); + expect(consoleSpy.warn).toHaveBeenCalledWith("warning", error, obj); + expect(consoleSpy.error).toHaveBeenCalledWith("error", error, obj); }); }); diff --git a/apps/cli/src/platform/services/console-log.service.ts b/apps/cli/src/platform/services/console-log.service.ts index a35dae71fc..5bdc0b4015 100644 --- a/apps/cli/src/platform/services/console-log.service.ts +++ b/apps/cli/src/platform/services/console-log.service.ts @@ -6,17 +6,17 @@ export class ConsoleLogService extends BaseConsoleLogService { super(isDev, filter); } - write(level: LogLevelType, message: string) { + write(level: LogLevelType, message?: any, ...optionalParams: any[]) { if (this.filter != null && this.filter(level)) { return; } if (process.env.BW_RESPONSE === "true") { // eslint-disable-next-line - console.error(message); + console.error(message, ...optionalParams); return; } - super.write(level, message); + super.write(level, message, ...optionalParams); } } diff --git a/apps/desktop/src/platform/preload.ts b/apps/desktop/src/platform/preload.ts index 771d25ef0a..d81d647652 100644 --- a/apps/desktop/src/platform/preload.ts +++ b/apps/desktop/src/platform/preload.ts @@ -103,7 +103,8 @@ export default { isMacAppStore: isMacAppStore(), isWindowsStore: isWindowsStore(), reloadProcess: () => ipcRenderer.send("reload-process"), - log: (level: LogLevelType, message: string) => ipcRenderer.invoke("ipc.log", { level, message }), + log: (level: LogLevelType, message?: any, ...optionalParams: any[]) => + ipcRenderer.invoke("ipc.log", { level, message, optionalParams }), openContextMenu: ( menu: { diff --git a/apps/desktop/src/platform/services/electron-log.main.service.ts b/apps/desktop/src/platform/services/electron-log.main.service.ts index 832365785c..0725de3dc9 100644 --- a/apps/desktop/src/platform/services/electron-log.main.service.ts +++ b/apps/desktop/src/platform/services/electron-log.main.service.ts @@ -25,28 +25,28 @@ export class ElectronLogMainService extends BaseLogService { } log.initialize(); - ipcMain.handle("ipc.log", (_event, { level, message }) => { - this.write(level, message); + ipcMain.handle("ipc.log", (_event, { level, message, optionalParams }) => { + this.write(level, message, ...optionalParams); }); } - write(level: LogLevelType, message: string) { + write(level: LogLevelType, message?: any, ...optionalParams: any[]) { if (this.filter != null && this.filter(level)) { return; } switch (level) { case LogLevelType.Debug: - log.debug(message); + log.debug(message, ...optionalParams); break; case LogLevelType.Info: - log.info(message); + log.info(message, ...optionalParams); break; case LogLevelType.Warning: - log.warn(message); + log.warn(message, ...optionalParams); break; case LogLevelType.Error: - log.error(message); + log.error(message, ...optionalParams); break; default: break; diff --git a/apps/desktop/src/platform/services/electron-log.renderer.service.ts b/apps/desktop/src/platform/services/electron-log.renderer.service.ts index e0e0757e6a..cea939f160 100644 --- a/apps/desktop/src/platform/services/electron-log.renderer.service.ts +++ b/apps/desktop/src/platform/services/electron-log.renderer.service.ts @@ -6,27 +6,29 @@ export class ElectronLogRendererService extends BaseLogService { super(ipc.platform.isDev, filter); } - write(level: LogLevelType, message: string) { + write(level: LogLevelType, message?: any, ...optionalParams: any[]) { if (this.filter != null && this.filter(level)) { return; } /* eslint-disable no-console */ - ipc.platform.log(level, message).catch((e) => console.log("Error logging", e)); + ipc.platform + .log(level, message, ...optionalParams) + .catch((e) => console.log("Error logging", e)); /* eslint-disable no-console */ switch (level) { case LogLevelType.Debug: - console.debug(message); + console.debug(message, ...optionalParams); break; case LogLevelType.Info: - console.info(message); + console.info(message, ...optionalParams); break; case LogLevelType.Warning: - console.warn(message); + console.warn(message, ...optionalParams); break; case LogLevelType.Error: - console.error(message); + console.error(message, ...optionalParams); break; default: break; diff --git a/libs/angular/src/platform/services/logging-error-handler.ts b/libs/angular/src/platform/services/logging-error-handler.ts index 522412dd28..5644272d35 100644 --- a/libs/angular/src/platform/services/logging-error-handler.ts +++ b/libs/angular/src/platform/services/logging-error-handler.ts @@ -14,7 +14,7 @@ export class LoggingErrorHandler extends ErrorHandler { override handleError(error: any): void { try { const logService = this.injector.get(LogService, null); - logService.error(error); + logService.error("Unhandled error in angular", error); } catch { super.handleError(error); } diff --git a/libs/common/spec/intercept-console.ts b/libs/common/spec/intercept-console.ts index 01c4063e7a..565d475cae 100644 --- a/libs/common/spec/intercept-console.ts +++ b/libs/common/spec/intercept-console.ts @@ -2,22 +2,17 @@ const originalConsole = console; declare let console: any; -export function interceptConsole(interceptions: any): object { +export function interceptConsole(): { + log: jest.Mock; + warn: jest.Mock; + error: jest.Mock; +} { console = { - log: function () { - // eslint-disable-next-line - interceptions.log = arguments; - }, - warn: function () { - // eslint-disable-next-line - interceptions.warn = arguments; - }, - error: function () { - // eslint-disable-next-line - interceptions.error = arguments; - }, + log: jest.fn(), + warn: jest.fn(), + error: jest.fn(), }; - return interceptions; + return console; } export function restoreConsole() { diff --git a/libs/common/src/platform/abstractions/log.service.ts b/libs/common/src/platform/abstractions/log.service.ts index dffa3ca8d3..d77a4f6990 100644 --- a/libs/common/src/platform/abstractions/log.service.ts +++ b/libs/common/src/platform/abstractions/log.service.ts @@ -1,9 +1,9 @@ import { LogLevelType } from "../enums/log-level-type.enum"; export abstract class LogService { - abstract debug(message: string): void; - abstract info(message: string): void; - abstract warning(message: string): void; - abstract error(message: string): void; - abstract write(level: LogLevelType, message: string): void; + abstract debug(message?: any, ...optionalParams: any[]): void; + abstract info(message?: any, ...optionalParams: any[]): void; + abstract warning(message?: any, ...optionalParams: any[]): void; + abstract error(message?: any, ...optionalParams: any[]): void; + abstract write(level: LogLevelType, message?: any, ...optionalParams: any[]): void; } diff --git a/libs/common/src/platform/services/console-log.service.spec.ts b/libs/common/src/platform/services/console-log.service.spec.ts index 129969bbc4..508ca4eb32 100644 --- a/libs/common/src/platform/services/console-log.service.spec.ts +++ b/libs/common/src/platform/services/console-log.service.spec.ts @@ -2,13 +2,18 @@ import { interceptConsole, restoreConsole } from "../../../spec"; import { ConsoleLogService } from "./console-log.service"; -let caughtMessage: any; - describe("ConsoleLogService", () => { + const error = new Error("this is an error"); + const obj = { a: 1, b: 2 }; + let consoleSpy: { + log: jest.Mock; + warn: jest.Mock; + error: jest.Mock; + }; let logService: ConsoleLogService; + beforeEach(() => { - caughtMessage = {}; - interceptConsole(caughtMessage); + consoleSpy = interceptConsole(); logService = new ConsoleLogService(true); }); @@ -18,41 +23,41 @@ describe("ConsoleLogService", () => { it("filters messages below the set threshold", () => { logService = new ConsoleLogService(true, () => true); - logService.debug("debug"); - logService.info("info"); - logService.warning("warning"); - logService.error("error"); + logService.debug("debug", error, obj); + logService.info("info", error, obj); + logService.warning("warning", error, obj); + logService.error("error", error, obj); - expect(caughtMessage).toEqual({}); + expect(consoleSpy.log).not.toHaveBeenCalled(); + expect(consoleSpy.warn).not.toHaveBeenCalled(); + expect(consoleSpy.error).not.toHaveBeenCalled(); }); + it("only writes debug messages in dev mode", () => { logService = new ConsoleLogService(false); logService.debug("debug message"); - expect(caughtMessage.log).toBeUndefined(); + expect(consoleSpy.log).not.toHaveBeenCalled(); }); it("writes debug/info messages to console.log", () => { - logService.debug("this is a debug message"); - expect(caughtMessage).toMatchObject({ - log: { "0": "this is a debug message" }, - }); + logService.debug("this is a debug message", error, obj); + logService.info("this is an info message", error, obj); - logService.info("this is an info message"); - expect(caughtMessage).toMatchObject({ - log: { "0": "this is an info message" }, - }); + expect(consoleSpy.log).toHaveBeenCalledTimes(2); + expect(consoleSpy.log).toHaveBeenCalledWith("this is a debug message", error, obj); + expect(consoleSpy.log).toHaveBeenCalledWith("this is an info message", error, obj); }); + it("writes warning messages to console.warn", () => { - logService.warning("this is a warning message"); - expect(caughtMessage).toMatchObject({ - warn: { 0: "this is a warning message" }, - }); + logService.warning("this is a warning message", error, obj); + + expect(consoleSpy.warn).toHaveBeenCalledWith("this is a warning message", error, obj); }); + it("writes error messages to console.error", () => { - logService.error("this is an error message"); - expect(caughtMessage).toMatchObject({ - error: { 0: "this is an error message" }, - }); + logService.error("this is an error message", error, obj); + + expect(consoleSpy.error).toHaveBeenCalledWith("this is an error message", error, obj); }); }); diff --git a/libs/common/src/platform/services/console-log.service.ts b/libs/common/src/platform/services/console-log.service.ts index 3eb3ad1881..a1480a0c26 100644 --- a/libs/common/src/platform/services/console-log.service.ts +++ b/libs/common/src/platform/services/console-log.service.ts @@ -9,26 +9,26 @@ export class ConsoleLogService implements LogServiceAbstraction { protected filter: (level: LogLevelType) => boolean = null, ) {} - debug(message: string) { + debug(message?: any, ...optionalParams: any[]) { if (!this.isDev) { return; } - this.write(LogLevelType.Debug, message); + this.write(LogLevelType.Debug, message, ...optionalParams); } - info(message: string) { - this.write(LogLevelType.Info, message); + info(message?: any, ...optionalParams: any[]) { + this.write(LogLevelType.Info, message, ...optionalParams); } - warning(message: string) { - this.write(LogLevelType.Warning, message); + warning(message?: any, ...optionalParams: any[]) { + this.write(LogLevelType.Warning, message, ...optionalParams); } - error(message: string) { - this.write(LogLevelType.Error, message); + error(message?: any, ...optionalParams: any[]) { + this.write(LogLevelType.Error, message, ...optionalParams); } - write(level: LogLevelType, message: string) { + write(level: LogLevelType, message?: any, ...optionalParams: any[]) { if (this.filter != null && this.filter(level)) { return; } @@ -36,19 +36,19 @@ export class ConsoleLogService implements LogServiceAbstraction { switch (level) { case LogLevelType.Debug: // eslint-disable-next-line - console.log(message); + console.log(message, ...optionalParams); break; case LogLevelType.Info: // eslint-disable-next-line - console.log(message); + console.log(message, ...optionalParams); break; case LogLevelType.Warning: // eslint-disable-next-line - console.warn(message); + console.warn(message, ...optionalParams); break; case LogLevelType.Error: // eslint-disable-next-line - console.error(message); + console.error(message, ...optionalParams); break; default: break;