From d2ff5740bab25d2a104fc98771e9475a5b58e185 Mon Sep 17 00:00:00 2001 From: Austin Devine Date: Sat, 20 Sep 2025 01:12:01 +0100 Subject: [PATCH 1/5] feat: integrate logger utility across the fiori-mcp-server package --- packages/fiori-mcp-server/package.json | 1 + packages/fiori-mcp-server/src/index.ts | 3 +- .../parser/model/PageEditModel.ts | 3 +- .../src/page-editor-api/parser/tree.ts | 3 +- .../src/page-editor-api/sapuxFtfsFileIO.ts | 3 +- packages/fiori-mcp-server/src/server.ts | 5 +- .../fiori-mcp-server/src/telemetry/index.ts | 3 +- .../generate-fiori-ui-app/command.ts | 7 +- .../src/tools/hybrid-search.ts | 3 +- .../src/tools/services/filestore.ts | 4 +- .../src/tools/services/indexer-simple.ts | 14 +- .../src/tools/services/utils/logger.ts | 61 ------ .../src/tools/services/vector-simple.ts | 8 +- packages/fiori-mcp-server/src/tools/utils.ts | 5 +- .../src/utils/embeddings-path.ts | 2 +- packages/fiori-mcp-server/src/utils/logger.ts | 45 ++++ .../test/unit/tools/hybrid-search.test.ts | 14 +- .../unit/tools/services/filestore.test.ts | 6 +- .../tools/services/indexer-simple.test.ts | 15 +- .../unit/tools/services/utils/logger.test.ts | 197 ------------------ .../unit/tools/services/vector-simple.test.ts | 10 +- .../test/unit/utils/embeddings-path.test.ts | 4 +- .../test/unit/utils/logger.test.ts | 39 ++++ packages/fiori-mcp-server/tsconfig.json | 3 + pnpm-lock.yaml | 3 + 25 files changed, 150 insertions(+), 311 deletions(-) delete mode 100644 packages/fiori-mcp-server/src/tools/services/utils/logger.ts create mode 100644 packages/fiori-mcp-server/src/utils/logger.ts delete mode 100644 packages/fiori-mcp-server/test/unit/tools/services/utils/logger.test.ts create mode 100644 packages/fiori-mcp-server/test/unit/utils/logger.test.ts diff --git a/packages/fiori-mcp-server/package.json b/packages/fiori-mcp-server/package.json index bf83c0da3bc..346a1588791 100644 --- a/packages/fiori-mcp-server/package.json +++ b/packages/fiori-mcp-server/package.json @@ -54,6 +54,7 @@ "@sap-ux/annotation-converter": "0.10.3", "@sap-ux/fiori-docs-embeddings": "workspace:*", "@sap-ux/i18n": "workspace:*", + "@sap-ux/logger": "workspace:*", "@sap-ux/odata-annotation-core-types": "workspace:*", "@sap-ux/odata-entity-model": "workspace:*", "@sap-ux/text-document-utils": "workspace:*", diff --git a/packages/fiori-mcp-server/src/index.ts b/packages/fiori-mcp-server/src/index.ts index 5c424569d66..ecc977fba1f 100644 --- a/packages/fiori-mcp-server/src/index.ts +++ b/packages/fiori-mcp-server/src/index.ts @@ -1,6 +1,7 @@ #!/usr/bin/env node import { FioriFunctionalityServer } from './server'; +import { logger } from './utils/logger'; const server = new FioriFunctionalityServer(); -server.run().catch(console.error); +server.run().catch((error) => logger.error(`Server error: ${error}`)); diff --git a/packages/fiori-mcp-server/src/page-editor-api/parser/model/PageEditModel.ts b/packages/fiori-mcp-server/src/page-editor-api/parser/model/PageEditModel.ts index 05944e816f9..a3945ffbc94 100644 --- a/packages/fiori-mcp-server/src/page-editor-api/parser/model/PageEditModel.ts +++ b/packages/fiori-mcp-server/src/page-editor-api/parser/model/PageEditModel.ts @@ -22,6 +22,7 @@ import { FieldsAggregation, ConnectedFieldsAggregation } from './fields'; import { isArrayEndsWith, getMatchingNode } from './utils'; import { ActionsAggregation } from './actions'; import i18next from 'i18next'; +import { logger } from '../../../utils/logger'; import { FilterFieldsAggregation } from './filter-fields'; import { VisualFiltersAggregation } from './visual-filters'; import { SectionAggregation, HeaderSectionsAggregation, SubSectionsAggregation } from './sections'; @@ -932,7 +933,7 @@ export class PageEditModel { if (currentNode[property] !== undefined || (property === 'i18nClassification' && property in currentNode)) { if (!this.isCorrectSchemaProperty(property, currentNode[property])) { // schema property is not correct - throw error and do not store value, because it can give unpredictable behavior - console.error( + logger.error( i18next.t('SCHEMA_PARSING_ERROR_UNEXPECTED_VALUE', { name: property, value: JSON.stringify(currentNode[property]) diff --git a/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts b/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts index 6fc968ff5a1..7dd2149a68d 100644 --- a/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts +++ b/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts @@ -16,6 +16,7 @@ import { DATA_FIELD_FOR_ACTION_GROUP } from './model'; import type { JSONSchema4, JSONSchema4Type } from 'json-schema'; +import { logger } from '../../utils/logger'; interface TraverseNodeData { text: string; @@ -273,7 +274,7 @@ export function getPropertyData( if (property.schema.enum) { getEnumOptions(property.schema.enum); } else { - console.warn('Unhandled property', property); + logger.warn(`Unhandled property: ${property}`); } break; } diff --git a/packages/fiori-mcp-server/src/page-editor-api/sapuxFtfsFileIO.ts b/packages/fiori-mcp-server/src/page-editor-api/sapuxFtfsFileIO.ts index 0db2f030b52..6bf94dd395d 100644 --- a/packages/fiori-mcp-server/src/page-editor-api/sapuxFtfsFileIO.ts +++ b/packages/fiori-mcp-server/src/page-editor-api/sapuxFtfsFileIO.ts @@ -14,6 +14,7 @@ import { basename, join } from 'path'; import type { ApplicationAccess, Manifest } from '@sap-ux/project-access'; import type { Store } from 'mem-fs'; import { getManifest, getUI5Version, readAnnotationFiles } from './project'; +import { logger } from '../utils/logger'; export interface PageData { pageId: string; @@ -155,7 +156,7 @@ export class SapuxFtfsFileIO { } } } catch (error) { - console.log(String(error)); + logger.error(String(error)); } return undefined; } diff --git a/packages/fiori-mcp-server/src/server.ts b/packages/fiori-mcp-server/src/server.ts index 377d1a75bf2..ed86d15aadd 100644 --- a/packages/fiori-mcp-server/src/server.ts +++ b/packages/fiori-mcp-server/src/server.ts @@ -20,6 +20,7 @@ import type { ListFioriAppsInput, ListFunctionalitiesInput } from './types'; +import { logger } from './utils/logger'; type ToolArgs = | DocSearchInput @@ -61,7 +62,7 @@ export class FioriFunctionalityServer { * Logs MCP errors and handles the SIGINT signal for graceful shutdown. */ private setupErrorHandling(): void { - this.server.onerror = (error) => console.error('[MCP Error]', error); + this.server.onerror = (error) => logger.error(`[MCP Error] ${error}`); process.on('SIGINT', async () => { await this.server.close(); process.exit(0); @@ -166,6 +167,6 @@ export class FioriFunctionalityServer { const transport = new StdioServerTransport(); await this.server.connect(transport); await this.setupTelemetry(); - console.error('Fiori Functionality MCP Server running on stdio'); + logger.info('Fiori Functionality MCP Server running on stdio'); } } diff --git a/packages/fiori-mcp-server/src/telemetry/index.ts b/packages/fiori-mcp-server/src/telemetry/index.ts index 2bb6c89adf1..b00c3e0f90f 100644 --- a/packages/fiori-mcp-server/src/telemetry/index.ts +++ b/packages/fiori-mcp-server/src/telemetry/index.ts @@ -12,6 +12,7 @@ import { isAppStudio } from '@sap-ux/btp-utils'; import osName from 'os-name'; import i18next from 'i18next'; import { version } from '../../package.json'; +import { logger } from '../utils/logger'; export const mcpServerName = '@sap-ux/fiori-mcp-server'; export const unknownTool = 'unknown-tool'; @@ -57,7 +58,7 @@ export abstract class TelemetryHelper { try { await initTelemetrySettings(telemetryOptions); } catch (error) { - console.error('Error initializing telemetry settings:', error); + logger.error(`Error initializing telemetry settings: ${error}`); } } diff --git a/packages/fiori-mcp-server/src/tools/functionalities/generate-fiori-ui-app/command.ts b/packages/fiori-mcp-server/src/tools/functionalities/generate-fiori-ui-app/command.ts index e0050921a2e..297af2d92f3 100644 --- a/packages/fiori-mcp-server/src/tools/functionalities/generate-fiori-ui-app/command.ts +++ b/packages/fiori-mcp-server/src/tools/functionalities/generate-fiori-ui-app/command.ts @@ -7,6 +7,7 @@ import { GENERATE_FIORI_UI_APP_ID } from '../../../constant'; import { findInstalledPackages, type PackageInfo } from '@sap-ux/nodejs-utils'; import * as z from 'zod'; import packageJson from '../../../../package.json'; +import { logger } from '../../../utils/logger'; const GeneratorConfigSchemaCAP = z.object({ floorplan: z.literal(['FE_FPM', 'FE_LROP', 'FE_OVP', 'FE_ALP', 'FE_FEOP', 'FE_WORKLIST', 'FF_SIMPLE']), @@ -115,12 +116,12 @@ export async function command(params: ExecuteFunctionalityInput): Promise arg.startsWith('--log-level='))?.split('=')[1]; - - const logLevelString = globalLogLevel || envLogLevel || argsLogLevel; - - if (logLevelString) { - const level = logLevelString.toUpperCase(); - if (level in LogLevel && typeof LogLevel[level as keyof typeof LogLevel] === 'number') { - return LogLevel[level as keyof typeof LogLevel] as LogLevel; - } - } - - // Default to NONE - return LogLevel.NONE; -} - -const currentLogLevel = getLogLevel(); - -export const logger = { - log: (message: string, ...args: any[]) => { - if (currentLogLevel >= LogLevel.DEBUG) { - console.log(message, ...args); - } - }, - - error: (message: string, ...args: any[]) => { - if (currentLogLevel >= LogLevel.ERROR) { - console.error(message, ...args); - } - }, - - warn: (message: string, ...args: any[]) => { - if (currentLogLevel >= LogLevel.WARN) { - console.warn(message, ...args); - } - }, - - info: (message: string, ...args: any[]) => { - if (currentLogLevel >= LogLevel.INFO) { - console.info(message, ...args); - } - } -}; diff --git a/packages/fiori-mcp-server/src/tools/services/vector-simple.ts b/packages/fiori-mcp-server/src/tools/services/vector-simple.ts index 8e8beb425b5..f5d120f66ca 100644 --- a/packages/fiori-mcp-server/src/tools/services/vector-simple.ts +++ b/packages/fiori-mcp-server/src/tools/services/vector-simple.ts @@ -1,7 +1,7 @@ import type { Connection, Table } from '@lancedb/lancedb'; import { connect } from '@lancedb/lancedb'; import type { VectorSearchResult } from './types/vector'; -import { logger } from './utils/logger'; +import { logger } from '../../utils/logger'; import { resolveEmbeddingsPath } from '../../utils/embeddings-path'; import fs from 'fs/promises'; import path from 'path'; @@ -165,7 +165,7 @@ export class SimpleVectorService { distance: result._distance || 0 })); } catch (error) { - logger.error('Semantic search failed:', error); + logger.error(`Semantic search failed: ${error}`); throw new Error(`Semantic search failed: ${error}`); } } @@ -274,7 +274,7 @@ export class SimpleVectorService { return similarDocs; } catch (error) { - logger.error('Find similar documents failed:', error); + logger.error(`Find similar documents failed: ${error}`); return []; } } @@ -329,7 +329,7 @@ export class SimpleVectorService { distance: 0 })); } catch (error) { - logger.error('Get documents by category failed:', error); + logger.error(`Get documents by category failed: ${error}`); return []; } } diff --git a/packages/fiori-mcp-server/src/tools/utils.ts b/packages/fiori-mcp-server/src/tools/utils.ts index 0561d20b3df..666deb5ea6a 100644 --- a/packages/fiori-mcp-server/src/tools/utils.ts +++ b/packages/fiori-mcp-server/src/tools/utils.ts @@ -2,6 +2,7 @@ import { findProjectRoot, createApplicationAccess, getProject, DirName } from '@ import { join } from 'path'; import * as zod from 'zod'; import type { Appdetails } from '../types'; +import { logger } from '../utils/logger'; /** * Resolves the application details from a given path. @@ -22,7 +23,7 @@ export async function resolveApplication(path: string): Promise arg.startsWith('--log-level='))?.split('=')[1]; + + const logLevelString = globalLogLevel || envLogLevel || argsLogLevel; + + if (logLevelString) { + const level = logLevelString.toUpperCase(); + switch (level) { + case 'ERROR': + return LogLevel.Error; + case 'WARN': + return LogLevel.Warn; + case 'INFO': + return LogLevel.Info; + case 'DEBUG': + return LogLevel.Debug; + case 'VERBOSE': + return LogLevel.Verbose; + case 'SILLY': + return LogLevel.Silly; + default: + break; + } + } + + // Default to Error (minimal logging) + return LogLevel.Error; +} + +const currentLogLevel = getLogLevel(); + +// Create a global logger instance to be reused throughout the tool +export const logger = new ToolsLogger({ + logLevel: currentLogLevel, + transports: [new ConsoleTransport()], + logPrefix: 'fiori-mcp' +}); diff --git a/packages/fiori-mcp-server/test/unit/tools/hybrid-search.test.ts b/packages/fiori-mcp-server/test/unit/tools/hybrid-search.test.ts index 7f3a75d2589..992e5fae8af 100644 --- a/packages/fiori-mcp-server/test/unit/tools/hybrid-search.test.ts +++ b/packages/fiori-mcp-server/test/unit/tools/hybrid-search.test.ts @@ -1,11 +1,14 @@ import type { DocSearchInput } from '../../../src/tools/hybrid-search'; import { DocSearchService, docSearch } from '../../../src/tools/hybrid-search'; import { SimpleDocumentIndexer } from '../../../src/tools/services/indexer-simple'; +import { logger } from '../../../src/utils/logger'; -// Mock the SimpleDocumentIndexer +// Mock the SimpleDocumentIndexer and logger jest.mock('../../../src/tools/services/indexer-simple'); +jest.mock('../../../src/utils/logger'); const mockIndexer = SimpleDocumentIndexer as jest.MockedClass; +const mockLogger = logger as jest.Mocked; describe('hybrid-search', () => { let indexerInstance: jest.Mocked; @@ -127,8 +130,8 @@ describe('hybrid-search', () => { describe('docSearch function', () => { beforeEach(() => { - // Mock console.warn to avoid test output noise - jest.spyOn(console, 'warn').mockImplementation(() => {}); + // Clear logger mock calls + jest.clearAllMocks(); }); afterEach(() => { @@ -192,9 +195,8 @@ describe('hybrid-search', () => { expect(result.suggestion).toContain('npm install -g @sap-ux/fiori-docs-embeddings'); expect(result.results).toEqual([]); expect(result.total).toBe(0); - expect(console.warn).toHaveBeenCalledWith( - 'Embeddings data not available, providing limited search capability:', - expect.any(Error) + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Embeddings data not available, providing limited search capability:') ); }); diff --git a/packages/fiori-mcp-server/test/unit/tools/services/filestore.test.ts b/packages/fiori-mcp-server/test/unit/tools/services/filestore.test.ts index 5bf20f69773..d25e2fe811a 100644 --- a/packages/fiori-mcp-server/test/unit/tools/services/filestore.test.ts +++ b/packages/fiori-mcp-server/test/unit/tools/services/filestore.test.ts @@ -3,12 +3,12 @@ import path from 'path'; import type { FileStoreIndex } from '../../../../src/tools/services/filestore'; import { FileStoreService } from '../../../../src/tools/services/filestore'; import type { DocumentMeta } from '../../../../src/tools/services/types/index'; -import { logger } from '../../../../src/tools/services/utils/logger'; +import { logger } from '../../../../src/utils/logger'; import { resolveEmbeddingsPath } from '../../../../src/utils/embeddings-path'; // Mock dependencies jest.mock('fs/promises'); -jest.mock('../../../../src/tools/services/utils/logger'); +jest.mock('../../../../src/utils/logger'); jest.mock('../../../../src/utils/embeddings-path'); const mockFs = fs as jest.Mocked; @@ -218,7 +218,7 @@ describe('FileStoreService', () => { const result = await fileStore.getDocument('doc1'); expect(result).toBeNull(); - expect(mockLogger.warn).toHaveBeenCalledWith('Failed to load document doc1:', readError); + expect(mockLogger.warn).toHaveBeenCalledWith(`Failed to load document doc1: ${readError}`); }); it('should parse lastModified date correctly', async () => { diff --git a/packages/fiori-mcp-server/test/unit/tools/services/indexer-simple.test.ts b/packages/fiori-mcp-server/test/unit/tools/services/indexer-simple.test.ts index 4a849034751..584df3d1ab5 100644 --- a/packages/fiori-mcp-server/test/unit/tools/services/indexer-simple.test.ts +++ b/packages/fiori-mcp-server/test/unit/tools/services/indexer-simple.test.ts @@ -2,7 +2,7 @@ import fs from 'fs/promises'; import { SimpleDocumentIndexer } from '../../../../src/tools/services/indexer-simple'; import { FileStoreService } from '../../../../src/tools/services/filestore'; import { SimpleVectorService } from '../../../../src/tools/services/vector-simple'; -import { logger } from '../../../../src/tools/services/utils/logger'; +import { logger } from '../../../../src/utils/logger'; import { resolveEmbeddingsPath } from '../../../../src/utils/embeddings-path'; import type { DocumentMeta } from '../../../../src/tools/services/types/index'; @@ -10,7 +10,7 @@ import type { DocumentMeta } from '../../../../src/tools/services/types/index'; jest.mock('fs/promises'); jest.mock('../../../../src/tools/services/filestore'); jest.mock('../../../../src/tools/services/vector-simple'); -jest.mock('../../../../src/tools/services/utils/logger'); +jest.mock('../../../../src/utils/logger'); jest.mock('../../../../src/utils/embeddings-path'); const mockFs = fs as jest.Mocked; @@ -161,8 +161,7 @@ describe('SimpleDocumentIndexer', () => { await expect(indexer.initialize()).rejects.toThrow('Failed to initialize indexer: Error: Filestore failed'); expect(mockLogger.warn).toHaveBeenCalledWith( - 'Filestore initialization failed. Please install @sap-ux/fiori-docs-embeddings for full documentation search capabilities:', - fileStoreError + `Filestore initialization failed. Please install @sap-ux/fiori-docs-embeddings for full documentation search capabilities: ${fileStoreError}` ); }); @@ -182,8 +181,7 @@ describe('SimpleDocumentIndexer', () => { await indexer.initialize(); expect(mockLogger.warn).toHaveBeenCalledWith( - 'Vector service initialization failed, disabling vector search. Install @sap-ux/fiori-docs-embeddings for full capabilities:', - vectorError + `Vector service initialization failed, disabling vector search. Install @sap-ux/fiori-docs-embeddings for full capabilities: ${vectorError}` ); expect(mockLogger.log).toHaveBeenCalledWith('✓ Document indexer initialized'); }); @@ -204,8 +202,7 @@ describe('SimpleDocumentIndexer', () => { await indexer.initialize(); expect(mockLogger.warn).toHaveBeenCalledWith( - 'Failed to load keyword index, keyword search will be limited:', - keywordError + `Failed to load keyword index, keyword search will be limited: ${keywordError}` ); expect(mockLogger.log).toHaveBeenCalledWith('✓ Document indexer initialized'); }); @@ -450,7 +447,7 @@ describe('SimpleDocumentIndexer', () => { const results = await indexer.findSimilarDocuments('doc1'); expect(results).toEqual([]); - expect(mockLogger.error).toHaveBeenCalledWith('Find similar documents failed:', expect.any(Error)); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('Find similar documents failed:')); }); it('should skip documents that cannot be retrieved', async () => { diff --git a/packages/fiori-mcp-server/test/unit/tools/services/utils/logger.test.ts b/packages/fiori-mcp-server/test/unit/tools/services/utils/logger.test.ts deleted file mode 100644 index 66d8857d8c3..00000000000 --- a/packages/fiori-mcp-server/test/unit/tools/services/utils/logger.test.ts +++ /dev/null @@ -1,197 +0,0 @@ -import { LogLevel, logger } from '../../../../../src/tools/services/utils/logger'; - -describe('logger', () => { - let consoleSpy: { - log: jest.SpyInstance; - error: jest.SpyInstance; - warn: jest.SpyInstance; - info: jest.SpyInstance; - }; - - beforeEach(() => { - consoleSpy = { - log: jest.spyOn(console, 'log').mockImplementation(), - error: jest.spyOn(console, 'error').mockImplementation(), - warn: jest.spyOn(console, 'warn').mockImplementation(), - info: jest.spyOn(console, 'info').mockImplementation() - }; - }); - - afterEach(() => { - consoleSpy.log.mockRestore(); - consoleSpy.error.mockRestore(); - consoleSpy.warn.mockRestore(); - consoleSpy.info.mockRestore(); - }); - - describe('LogLevel enum', () => { - it('should have correct values', () => { - expect(LogLevel.NONE).toBe(0); - expect(LogLevel.ERROR).toBe(1); - expect(LogLevel.WARN).toBe(2); - expect(LogLevel.INFO).toBe(3); - expect(LogLevel.DEBUG).toBe(4); - }); - }); - - describe('logger methods', () => { - describe('with LOG_LEVEL environment variable', () => { - const originalEnv = process.env.LOG_LEVEL; - - afterAll(() => { - if (originalEnv) { - process.env.LOG_LEVEL = originalEnv; - } else { - delete process.env.LOG_LEVEL; - } - }); - - it('should log when level is DEBUG', () => { - process.env.LOG_LEVEL = 'DEBUG'; - - // Re-require the module to pick up new env var - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message', 'arg1', 'arg2'); - newLogger.error('error message'); - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.log).toHaveBeenCalledWith('test message', 'arg1', 'arg2'); - expect(consoleSpy.error).toHaveBeenCalledWith('error message'); - expect(consoleSpy.warn).toHaveBeenCalledWith('warn message'); - expect(consoleSpy.info).toHaveBeenCalledWith('info message'); - }); - - it('should not log when level is NONE', () => { - process.env.LOG_LEVEL = 'NONE'; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message'); - newLogger.error('error message'); - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.log).not.toHaveBeenCalled(); - expect(consoleSpy.error).not.toHaveBeenCalled(); - expect(consoleSpy.warn).not.toHaveBeenCalled(); - expect(consoleSpy.info).not.toHaveBeenCalled(); - }); - - it('should only log error messages when level is ERROR', () => { - process.env.LOG_LEVEL = 'ERROR'; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message'); - newLogger.error('error message'); - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.log).not.toHaveBeenCalled(); - expect(consoleSpy.error).toHaveBeenCalledWith('error message'); - expect(consoleSpy.warn).not.toHaveBeenCalled(); - expect(consoleSpy.info).not.toHaveBeenCalled(); - }); - - it('should log error and warn messages when level is WARN', () => { - process.env.LOG_LEVEL = 'WARN'; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message'); - newLogger.error('error message'); - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.log).not.toHaveBeenCalled(); - expect(consoleSpy.error).toHaveBeenCalledWith('error message'); - expect(consoleSpy.warn).toHaveBeenCalledWith('warn message'); - expect(consoleSpy.info).not.toHaveBeenCalled(); - }); - - it('should log error, warn, and info messages when level is INFO', () => { - process.env.LOG_LEVEL = 'INFO'; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message'); - newLogger.error('error message'); - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.log).not.toHaveBeenCalled(); - expect(consoleSpy.error).toHaveBeenCalledWith('error message'); - expect(consoleSpy.warn).toHaveBeenCalledWith('warn message'); - expect(consoleSpy.info).toHaveBeenCalledWith('info message'); - }); - - it('should handle invalid log level gracefully', () => { - process.env.LOG_LEVEL = 'INVALID'; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.log('test message'); - newLogger.error('error message'); - - expect(consoleSpy.log).not.toHaveBeenCalled(); - expect(consoleSpy.error).not.toHaveBeenCalled(); - }); - }); - - describe('with command line arguments', () => { - const originalArgv = process.argv; - - afterEach(() => { - process.argv = originalArgv; - }); - - it('should use command line log level', () => { - process.argv = ['node', 'script.js', '--log-level=ERROR']; - delete process.env.LOG_LEVEL; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.error('error message'); - newLogger.warn('warn message'); - - expect(consoleSpy.error).toHaveBeenCalledWith('error message'); - expect(consoleSpy.warn).not.toHaveBeenCalled(); - }); - }); - - describe('with global log level', () => { - it('should use global log level when available', () => { - (global as any).LOG_LEVEL = 'WARN'; - delete process.env.LOG_LEVEL; - - jest.resetModules(); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { logger: newLogger } = require('../../../../../src/tools/services/utils/logger'); - - newLogger.warn('warn message'); - newLogger.info('info message'); - - expect(consoleSpy.warn).toHaveBeenCalledWith('warn message'); - expect(consoleSpy.info).not.toHaveBeenCalled(); - - delete (global as any).LOG_LEVEL; - }); - }); - }); -}); diff --git a/packages/fiori-mcp-server/test/unit/tools/services/vector-simple.test.ts b/packages/fiori-mcp-server/test/unit/tools/services/vector-simple.test.ts index 745797136db..bd08b786613 100644 --- a/packages/fiori-mcp-server/test/unit/tools/services/vector-simple.test.ts +++ b/packages/fiori-mcp-server/test/unit/tools/services/vector-simple.test.ts @@ -3,13 +3,13 @@ import path from 'path'; import { connect } from '@lancedb/lancedb'; import type { EmbeddingMetadata } from '../../../../src/tools/services/vector-simple'; import { SimpleVectorService } from '../../../../src/tools/services/vector-simple'; -import { logger } from '../../../../src/tools/services/utils/logger'; +import { logger } from '../../../../src/utils/logger'; import { resolveEmbeddingsPath } from '../../../../src/utils/embeddings-path'; // Mock dependencies jest.mock('fs/promises'); jest.mock('@lancedb/lancedb'); -jest.mock('../../../../src/tools/services/utils/logger'); +jest.mock('../../../../src/utils/logger'); jest.mock('../../../../src/utils/embeddings-path'); const mockFs = fs as jest.Mocked; @@ -329,7 +329,7 @@ describe('SimpleVectorService', () => { await expect(vectorService.semanticSearch(queryVector)).rejects.toThrow( 'Semantic search failed: Error: Search failed' ); - expect(mockLogger.error).toHaveBeenCalledWith('Semantic search failed:', searchError); + expect(mockLogger.error).toHaveBeenCalledWith(`Semantic search failed: ${searchError}`); }); it('should handle results without distance property', async () => { @@ -429,7 +429,7 @@ describe('SimpleVectorService', () => { const results = await vectorService.findSimilarToDocument('doc1'); expect(results).toEqual([]); - expect(mockLogger.error).toHaveBeenCalledWith('Find similar documents failed:', searchError); + expect(mockLogger.error).toHaveBeenCalledWith(`Find similar documents failed: ${searchError}`); }); it('should use default limit when not specified', async () => { @@ -493,7 +493,7 @@ describe('SimpleVectorService', () => { const results = await vectorService.getDocumentsByCategory('guides'); expect(results).toEqual([]); - expect(mockLogger.error).toHaveBeenCalledWith('Get documents by category failed:', searchError); + expect(mockLogger.error).toHaveBeenCalledWith(`Get documents by category failed: ${searchError}`); }); }); diff --git a/packages/fiori-mcp-server/test/unit/utils/embeddings-path.test.ts b/packages/fiori-mcp-server/test/unit/utils/embeddings-path.test.ts index 5d276059ef0..c012f7d2e9a 100644 --- a/packages/fiori-mcp-server/test/unit/utils/embeddings-path.test.ts +++ b/packages/fiori-mcp-server/test/unit/utils/embeddings-path.test.ts @@ -1,11 +1,11 @@ import path from 'path'; import fs from 'fs/promises'; import { resolveEmbeddingsPath, hasEmbeddingsData } from '../../../src/utils/embeddings-path'; -import { logger } from '../../../src/tools/services/utils/logger'; +import { logger } from '../../../src/utils/logger'; // Mock dependencies jest.mock('fs/promises'); -jest.mock('../../../src/tools/services/utils/logger'); +jest.mock('../../../src/utils/logger'); const mockFs = fs as jest.Mocked; const mockLogger = logger as jest.Mocked; diff --git a/packages/fiori-mcp-server/test/unit/utils/logger.test.ts b/packages/fiori-mcp-server/test/unit/utils/logger.test.ts new file mode 100644 index 00000000000..58daa9d7cfa --- /dev/null +++ b/packages/fiori-mcp-server/test/unit/utils/logger.test.ts @@ -0,0 +1,39 @@ +import { logger } from '../../../src/utils/logger'; +import { LogLevel } from '@sap-ux/logger'; + +describe('logger', () => { + it('should be an instance of ToolsLogger', () => { + expect(logger).toBeDefined(); + expect(typeof logger.info).toBe('function'); + expect(typeof logger.warn).toBe('function'); + expect(typeof logger.error).toBe('function'); + expect(typeof logger.debug).toBe('function'); + }); + + it('should support log level configuration from environment variables', () => { + // Test that the logger configuration function handles various log levels + // This is an indirect test since the logger is already initialized + const originalEnv = process.env.LOG_LEVEL; + + // Clean up + if (originalEnv !== undefined) { + process.env.LOG_LEVEL = originalEnv; + } else { + delete process.env.LOG_LEVEL; + } + + expect(LogLevel.Error).toBe(0); + expect(LogLevel.Warn).toBe(1); + expect(LogLevel.Info).toBe(2); + expect(LogLevel.Debug).toBe(4); + }); + + it('should have logging methods that accept string messages', () => { + // Test that logger methods exist and can be called + // We don't test actual output since that would require mocking the transport + expect(() => logger.info('test info')).not.toThrow(); + expect(() => logger.warn('test warn')).not.toThrow(); + expect(() => logger.error('test error')).not.toThrow(); + expect(() => logger.debug('test debug')).not.toThrow(); + }); +}); diff --git a/packages/fiori-mcp-server/tsconfig.json b/packages/fiori-mcp-server/tsconfig.json index b7629633dc4..6feb1975fb5 100644 --- a/packages/fiori-mcp-server/tsconfig.json +++ b/packages/fiori-mcp-server/tsconfig.json @@ -24,6 +24,9 @@ { "path": "../i18n" }, + { + "path": "../logger" + }, { "path": "../nodejs-utils" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a7cab48982b..3a92ed33de6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2281,6 +2281,9 @@ importers: '@sap-ux/i18n': specifier: workspace:* version: link:../i18n + '@sap-ux/logger': + specifier: workspace:* + version: link:../logger '@sap-ux/nodejs-utils': specifier: workspace:* version: link:../nodejs-utils From 166d2aec6fa3be8382b23d48fa678f560bbaf9aa Mon Sep 17 00:00:00 2001 From: Austin Devine Date: Sat, 20 Sep 2025 01:13:11 +0100 Subject: [PATCH 2/5] add cset --- .changeset/lazy-birds-serve.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lazy-birds-serve.md diff --git a/.changeset/lazy-birds-serve.md b/.changeset/lazy-birds-serve.md new file mode 100644 index 00000000000..80974d8e5df --- /dev/null +++ b/.changeset/lazy-birds-serve.md @@ -0,0 +1,5 @@ +--- +'@sap-ux/fiori-mcp-server': patch +--- + +feat: integrate ux logger utility across the fiori-mcp-server package From 941990fb18b43932b7d6fee6aa8443c64451552f Mon Sep 17 00:00:00 2001 From: Austin Devine Date: Mon, 22 Sep 2025 10:54:56 +0100 Subject: [PATCH 3/5] increase coverage on logger --- .../test/unit/utils/logger.test.ts | 246 ++++++++++++++++-- 1 file changed, 219 insertions(+), 27 deletions(-) diff --git a/packages/fiori-mcp-server/test/unit/utils/logger.test.ts b/packages/fiori-mcp-server/test/unit/utils/logger.test.ts index 58daa9d7cfa..d4f6a4eaf42 100644 --- a/packages/fiori-mcp-server/test/unit/utils/logger.test.ts +++ b/packages/fiori-mcp-server/test/unit/utils/logger.test.ts @@ -1,39 +1,231 @@ -import { logger } from '../../../src/utils/logger'; +import type { ToolsLogger } from '@sap-ux/logger'; import { LogLevel } from '@sap-ux/logger'; -describe('logger', () => { - it('should be an instance of ToolsLogger', () => { - expect(logger).toBeDefined(); - expect(typeof logger.info).toBe('function'); - expect(typeof logger.warn).toBe('function'); - expect(typeof logger.error).toBe('function'); - expect(typeof logger.debug).toBe('function'); - }); +describe('logger module', () => { + const originalEnv = process.env.LOG_LEVEL; + const originalArgv = process.argv; + const originalGlobal = (global as Record).LOG_LEVEL; + + beforeEach(() => { + // Reset environment + delete process.env.LOG_LEVEL; + delete (global as Record).LOG_LEVEL; + process.argv = ['node', 'test']; - it('should support log level configuration from environment variables', () => { - // Test that the logger configuration function handles various log levels - // This is an indirect test since the logger is already initialized - const originalEnv = process.env.LOG_LEVEL; + // Clear module cache to test fresh imports + jest.resetModules(); + }); - // Clean up + afterEach(() => { + // Restore original values if (originalEnv !== undefined) { process.env.LOG_LEVEL = originalEnv; - } else { - delete process.env.LOG_LEVEL; } + if (originalGlobal !== undefined) { + (global as Record).LOG_LEVEL = originalGlobal; + } + process.argv = originalArgv; + }); + + describe('getLogLevel function behavior', () => { + it('should return Error level as default when no configuration is provided', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + expect(logger.constructor.name).toBe('WinstonLogger'); + }); + + it('should use global LOG_LEVEL when set', () => { + (global as Record).LOG_LEVEL = 'INFO'; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should use environment LOG_LEVEL when global is not set', () => { + process.env.LOG_LEVEL = 'DEBUG'; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should use command line argument when global and env are not set', () => { + process.argv.push('--log-level=WARN'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should prioritize global over environment variable', () => { + (global as Record).LOG_LEVEL = 'ERROR'; + process.env.LOG_LEVEL = 'INFO'; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should prioritize global over command line argument', () => { + (global as Record).LOG_LEVEL = 'WARN'; + process.argv.push('--log-level=DEBUG'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should prioritize environment over command line argument', () => { + process.env.LOG_LEVEL = 'VERBOSE'; + process.argv.push('--log-level=SILLY'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + }); + + describe('log level parsing', () => { + const testCases = [ + { input: 'ERROR', expected: 'error level' }, + { input: 'error', expected: 'error level' }, + { input: 'Error', expected: 'error level' }, + { input: 'WARN', expected: 'warn level' }, + { input: 'warn', expected: 'warn level' }, + { input: 'INFO', expected: 'info level' }, + { input: 'info', expected: 'info level' }, + { input: 'DEBUG', expected: 'debug level' }, + { input: 'debug', expected: 'debug level' }, + { input: 'VERBOSE', expected: 'verbose level' }, + { input: 'verbose', expected: 'verbose level' }, + { input: 'SILLY', expected: 'silly level' }, + { input: 'silly', expected: 'silly level' } + ]; + + testCases.forEach(({ input, expected }) => { + it(`should handle ${input} case-insensitively for ${expected}`, () => { + process.env.LOG_LEVEL = input; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + }); + + it('should fall back to Error level for invalid log level strings', () => { + process.env.LOG_LEVEL = 'INVALID_LEVEL'; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should fall back to Error level for empty string', () => { + process.env.LOG_LEVEL = ''; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should fall back to Error level for whitespace-only string', () => { + process.env.LOG_LEVEL = ' '; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + }); + + describe('command line argument parsing', () => { + it('should parse --log-level=VALUE format correctly', () => { + process.argv.push('--log-level=INFO'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should handle command line argument without equals sign gracefully', () => { + process.argv.push('--log-level'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should handle multiple --log-level arguments by using the first one', () => { + process.argv.push('--log-level=DEBUG'); + process.argv.push('--log-level=ERROR'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should handle malformed command line arguments', () => { + process.argv.push('--log-level='); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + }); + + describe('logger instance configuration', () => { + it('should create logger with correct prefix', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + + it('should create logger with ConsoleTransport', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { logger } = require('../../../src/utils/logger') as { logger: ToolsLogger }; + expect(logger).toBeDefined(); + }); + }); + + describe('logger functionality', () => { + let logger: ToolsLogger; + + beforeEach(() => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const loggerModule = require('../../../src/utils/logger') as { logger: ToolsLogger }; + logger = loggerModule.logger; + }); + + it('should be an instance of ToolsLogger', () => { + expect(logger).toBeDefined(); + expect(logger.constructor.name).toBe('WinstonLogger'); + expect(typeof logger.info).toBe('function'); + expect(typeof logger.warn).toBe('function'); + expect(typeof logger.error).toBe('function'); + expect(typeof logger.debug).toBe('function'); + }); + + it('should have logging methods that accept string messages', () => { + expect(() => logger.info('test info')).not.toThrow(); + expect(() => logger.warn('test warn')).not.toThrow(); + expect(() => logger.error('test error')).not.toThrow(); + expect(() => logger.debug('test debug')).not.toThrow(); + }); + + it('should handle complex message formatting', () => { + const testObj = { key: 'value', nested: { prop: 123 } }; + const testError = new Error('Test error message'); + + expect(() => logger.info(`Test object: ${JSON.stringify(testObj)}`)).not.toThrow(); + expect(() => logger.error(`Error object: ${JSON.stringify(testObj)}`)).not.toThrow(); + expect(() => logger.error(`Error occurred: ${testError.message}`)).not.toThrow(); + expect(() => logger.info(`Multiple values: ${JSON.stringify(['arguments', 'test', 123])}`)).not.toThrow(); + expect(() => + logger.warn(`Warning with object: ${JSON.stringify({ object: true })} and number: 456`) + ).not.toThrow(); + }); - expect(LogLevel.Error).toBe(0); - expect(LogLevel.Warn).toBe(1); - expect(LogLevel.Info).toBe(2); - expect(LogLevel.Debug).toBe(4); + it('should handle edge case values', () => { + expect(() => logger.info('Undefined value: undefined')).not.toThrow(); + expect(() => logger.warn('Null value: null')).not.toThrow(); + expect(() => logger.error('')).not.toThrow(); + expect(() => logger.debug(' ')).not.toThrow(); + }); }); - it('should have logging methods that accept string messages', () => { - // Test that logger methods exist and can be called - // We don't test actual output since that would require mocking the transport - expect(() => logger.info('test info')).not.toThrow(); - expect(() => logger.warn('test warn')).not.toThrow(); - expect(() => logger.error('test error')).not.toThrow(); - expect(() => logger.debug('test debug')).not.toThrow(); + describe('LogLevel constants verification', () => { + it('should have correct LogLevel enum values', () => { + expect(LogLevel.Error).toBe(0); + expect(LogLevel.Warn).toBe(1); + expect(LogLevel.Info).toBe(2); + expect(LogLevel.Debug).toBe(4); + }); }); }); From 4f3b50b20b44bcb6818fbd7123bb77658b5a3686 Mon Sep 17 00:00:00 2001 From: Austin Devine Date: Mon, 22 Sep 2025 12:41:15 +0100 Subject: [PATCH 4/5] sonar issue --- packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts b/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts index 7dd2149a68d..79035046603 100644 --- a/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts +++ b/packages/fiori-mcp-server/src/page-editor-api/parser/tree.ts @@ -274,7 +274,7 @@ export function getPropertyData( if (property.schema.enum) { getEnumOptions(property.schema.enum); } else { - logger.warn(`Unhandled property: ${property}`); + logger.warn(`Unhandled property: ${JSON.stringify(property)}`); } break; } From 9216ac1c1b0a74b552299624edbe74177e6efbde Mon Sep 17 00:00:00 2001 From: Austin Devine Date: Mon, 22 Sep 2025 15:52:25 +0100 Subject: [PATCH 5/5] fix sonar issue --- packages/fiori-mcp-server/src/utils/logger.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/fiori-mcp-server/src/utils/logger.ts b/packages/fiori-mcp-server/src/utils/logger.ts index a45c24772cb..3f94ddabddd 100644 --- a/packages/fiori-mcp-server/src/utils/logger.ts +++ b/packages/fiori-mcp-server/src/utils/logger.ts @@ -5,7 +5,7 @@ import { ToolsLogger, LogLevel, ConsoleTransport } from '@sap-ux/logger'; */ function getLogLevel(): LogLevel { // Check multiple possible sources for the log level - const globalLogLevel = (global as any).LOG_LEVEL; + const globalLogLevel = (globalThis as any).LOG_LEVEL; const envLogLevel = process.env.LOG_LEVEL; const argsLogLevel = process.argv.find((arg) => arg.startsWith('--log-level='))?.split('=')[1];