From cb78770d2012128ec2415859626891d38616c110 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Wed, 22 May 2024 08:36:19 +0200 Subject: [PATCH] fix: data transfer logging (#20335) * fix: only create one log file and only as needed * test: add regression test for multiple logs --- .../src/cli/utils/__tests__/logger.test.ts | 89 +++++++++++++++++++ .../strapi/src/cli/utils/data-transfer.ts | 36 +++++--- .../src/configs/output-file-configuration.ts | 12 ++- 3 files changed, 122 insertions(+), 15 deletions(-) create mode 100644 packages/core/strapi/src/cli/utils/__tests__/logger.test.ts diff --git a/packages/core/strapi/src/cli/utils/__tests__/logger.test.ts b/packages/core/strapi/src/cli/utils/__tests__/logger.test.ts new file mode 100644 index 00000000000..3e2b036b403 --- /dev/null +++ b/packages/core/strapi/src/cli/utils/__tests__/logger.test.ts @@ -0,0 +1,89 @@ +import { type Logger, createLogger } from '@strapi/logger'; +import { formatDiagnostic } from '../data-transfer'; + +jest.mock('@strapi/logger', () => { + const actualWinston = jest.requireActual('winston'); + return { + ...actualWinston, + createLogger: jest.fn(), + configs: { + createOutputFileConfiguration: jest.fn(), + }, + }; +}); + +// Mock console methods to avoid cluttering test output +const consoleMock = { + error: jest.spyOn(console, 'error').mockImplementation(() => {}), + warn: jest.spyOn(console, 'warn').mockImplementation(() => {}), + info: jest.spyOn(console, 'info').mockImplementation(() => {}), +}; + +describe('logger', () => { + let mockLogger: jest.Mocked; + + beforeEach(() => { + jest.clearAllMocks(); + + // Create a mock logger with jest.fn() methods + mockLogger = { + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + } as any; + + // Mock createLogger to return the mock logger + (createLogger as jest.Mock).mockReturnValue(mockLogger); + }); + + afterEach(() => { + // Clear console mocks after each test + consoleMock.error.mockClear(); + consoleMock.warn.mockClear(); + consoleMock.info.mockClear(); + }); + + describe('formatDiagnostic', () => { + it('only creates a single logger', () => { + const diagnosticReporter = formatDiagnostic('export'); + + // Use the diagnostic reporter to log different levels of messages + diagnosticReporter({ + kind: 'error', + details: { + message: 'Test error', + createdAt: new Date(), + name: '', + severity: 'error', + error: new Error(), + }, + }); + diagnosticReporter({ + kind: 'warning', + details: { + message: 'Test warning', + createdAt: new Date(), + }, + }); + diagnosticReporter({ + kind: 'info', + details: { + message: 'Test info', + createdAt: new Date(), + }, + }); + + // Verify createLogger is called only once + expect(createLogger).toHaveBeenCalledTimes(1); + + // Verify the mock logger received the logs + expect(mockLogger.error).toHaveBeenCalledTimes(1); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('Test error')); + expect(mockLogger.warn).toHaveBeenCalledTimes(1); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('Test warning')); + expect(mockLogger.info).toHaveBeenCalledTimes(1); + expect(mockLogger.info).toHaveBeenCalledWith(expect.stringContaining('Test info')); + }); + }); +}); diff --git a/packages/core/strapi/src/cli/utils/data-transfer.ts b/packages/core/strapi/src/cli/utils/data-transfer.ts index 694a747b41e..7ba88c7dda4 100644 --- a/packages/core/strapi/src/cli/utils/data-transfer.ts +++ b/packages/core/strapi/src/cli/utils/data-transfer.ts @@ -1,7 +1,7 @@ import chalk from 'chalk'; import Table from 'cli-table3'; import { Command, Option } from 'commander'; -import { configs, createLogger } from '@strapi/logger'; +import { configs, createLogger, type winston } from '@strapi/logger'; import { createStrapi, compileStrapi } from '@strapi/core'; import ora from 'ora'; import { merge } from 'lodash/fp'; @@ -209,14 +209,23 @@ const errorColors = { silly: chalk.yellow, } as const; -const formatDiagnostic = - ( - operation: string - ): Parameters[0] => - ({ details, kind }) => { - const logger = createLogger( - configs.createOutputFileConfiguration(`${operation}_error_log_${Date.now()}.log`) - ); +const formatDiagnostic = ( + operation: string +): Parameters[0] => { + // Create log file for all incoming diagnostics + let logger: undefined | winston.Logger; + const getLogger = () => { + if (!logger) { + logger = createLogger( + configs.createOutputFileConfiguration(`${operation}_${Date.now()}.log`, { level: 'info' }) + ); + } + return logger; + }; + + // We don't want to write a log file until there is something to be logged + + return ({ details, kind }) => { try { if (kind === 'error') { const { message, severity = 'fatal' } = details; @@ -224,24 +233,25 @@ const formatDiagnostic = const colorizeError = errorColors[severity]; const errorMessage = colorizeError(`[${severity.toUpperCase()}] ${message}`); - logger.error(errorMessage); + getLogger().error(errorMessage); } if (kind === 'info') { const { message, params } = details; const msg = `${message}\n${params ? JSON.stringify(params, null, 2) : ''}`; - logger.info(msg); + getLogger().info(msg); } if (kind === 'warning') { const { origin, message } = details; - logger.warn(`(${origin ?? 'transfer'}) ${message}`); + getLogger().warn(`(${origin ?? 'transfer'}) ${message}`); } } catch (err) { - logger.error(err); + getLogger().error(err); } }; +}; type Loaders = { [key in engineDataTransfer.TransferStage]: ora.Ora; diff --git a/packages/utils/logger/src/configs/output-file-configuration.ts b/packages/utils/logger/src/configs/output-file-configuration.ts index 5c2841bddc9..a8b80b22a53 100644 --- a/packages/utils/logger/src/configs/output-file-configuration.ts +++ b/packages/utils/logger/src/configs/output-file-configuration.ts @@ -3,14 +3,22 @@ import { transports, LoggerOptions } from 'winston'; import { LEVEL_LABEL, LEVELS } from '../constants'; import { prettyPrint, excludeColors } from '../formats'; -export default (filename: string): LoggerOptions => { +export default ( + filename: string, + fileTransportOptions: transports.FileTransportOptions = {} +): LoggerOptions => { return { level: LEVEL_LABEL, levels: LEVELS, format: prettyPrint(), transports: [ new transports.Console(), - new transports.File({ level: 'error', filename, format: excludeColors }), + new transports.File({ + level: 'error', + filename, + format: excludeColors, + ...fileTransportOptions, + }), ], }; };