From 88dd0c8eb285113fbd900ab34b952080426872c4 Mon Sep 17 00:00:00 2001 From: Kazuki Yamada Date: Mon, 12 Aug 2024 16:12:46 +0900 Subject: [PATCH 1/4] refact: Refactor logging and improve test coverage --- eslint.config.js | 2 + src/cli/cliPrinter.ts | 43 +++---- src/cli/commands/defaultCommandRunner.ts | 10 +- src/cli/commands/initCommandRunner.ts | 12 +- src/cli/commands/versionCommandRunner.ts | 3 +- src/core/file/fileSanitizer.ts | 11 +- src/core/file/packageJsonParser.ts | 5 +- src/shared/errorHandler.ts | 2 - src/shared/logger.ts | 26 ++-- .../cli/commands/defaultCommandRunner.test.ts | 120 ++++++++++++++++++ tests/cli/commands/initCommandRunner.test.ts | 73 +++++++++++ .../cli/commands/versionCommandRunner.test.ts | 26 ++++ tests/config/configLoader.test.ts | 5 + tests/core/file/fileSanitizer.test.ts | 12 +- tests/core/file/packageJsonParser.test.ts | 5 + tests/shared/logger.test.ts | 89 +++++++++++++ vite.config.mts => vite.config.ts | 0 17 files changed, 387 insertions(+), 57 deletions(-) create mode 100644 tests/cli/commands/defaultCommandRunner.test.ts create mode 100644 tests/cli/commands/initCommandRunner.test.ts create mode 100644 tests/cli/commands/versionCommandRunner.test.ts create mode 100644 tests/shared/logger.test.ts rename vite.config.mts => vite.config.ts (100%) diff --git a/eslint.config.js b/eslint.config.js index f7f28da7..d595303a 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -58,6 +58,8 @@ export default tseslint.config( 'import-x/no-duplicates': 'error', 'import-x/order': 'error', + 'no-process-exit': 'error', + "prefer-arrow-functions/prefer-arrow-functions": [ "warn", { diff --git a/src/cli/cliPrinter.ts b/src/cli/cliPrinter.ts index 835bfa72..eb5db1a0 100644 --- a/src/cli/cliPrinter.ts +++ b/src/cli/cliPrinter.ts @@ -1,6 +1,7 @@ import path from 'node:path'; import pc from 'picocolors'; import type { SecretLintCoreResult } from '@secretlint/types'; +import { logger } from '../shared/logger.js'; export const printSummary = ( rootDir: string, @@ -19,32 +20,30 @@ export const printSummary = ( securityCheckMessage = pc.white('āœ” No suspicious files detected'); } - console.log(pc.white('šŸ“Š Pack Summary:')); - console.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); - console.log(`${pc.white(' Total Files:')} ${pc.white(totalFiles.toString())}`); - console.log(`${pc.white(' Total Chars:')} ${pc.white(totalCharacters.toString())}`); - console.log(`${pc.white(' Total Tokens:')} ${pc.white(totalTokens.toString())}`); - console.log(`${pc.white(' Output:')} ${pc.white(relativeOutputPath)}`); - console.log(`${pc.white(' Security:')} ${pc.white(securityCheckMessage)}`); + logger.log(pc.white('šŸ“Š Pack Summary:')); + logger.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); + logger.log(`${pc.white(' Total Files:')} ${pc.white(totalFiles.toString())}`); + logger.log(`${pc.white(' Total Chars:')} ${pc.white(totalCharacters.toString())}`); + logger.log(`${pc.white(' Total Tokens:')} ${pc.white(totalTokens.toString())}`); + logger.log(`${pc.white(' Output:')} ${pc.white(relativeOutputPath)}`); + logger.log(`${pc.white(' Security:')} ${pc.white(securityCheckMessage)}`); }; export const printSecurityCheck = (rootDir: string, suspiciousFilesResults: SecretLintCoreResult[]) => { - console.log(pc.white('šŸ”Ž Security Check:')); - console.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); + logger.log(pc.white('šŸ”Ž Security Check:')); + logger.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); if (suspiciousFilesResults.length === 0) { - console.log(pc.green('āœ”') + ' ' + pc.white('No suspicious files detected.')); + logger.log(pc.green('āœ”') + ' ' + pc.white('No suspicious files detected.')); } else { - console.log( - pc.yellow(`${suspiciousFilesResults.length} suspicious file(s) detected and excluded from the output:`), - ); + logger.log(pc.yellow(`${suspiciousFilesResults.length} suspicious file(s) detected and excluded from the output:`)); suspiciousFilesResults.forEach((suspiciousFilesResult, index) => { const relativeFilePath = path.relative(rootDir, suspiciousFilesResult.filePath); - console.log(`${pc.white(`${index + 1}.`)} ${pc.white(relativeFilePath)}`); - console.log(pc.dim(' - ' + suspiciousFilesResult.messages.map((message) => message.message).join('\n - '))); + logger.log(`${pc.white(`${index + 1}.`)} ${pc.white(relativeFilePath)}`); + logger.log(pc.dim(' - ' + suspiciousFilesResult.messages.map((message) => message.message).join('\n - '))); }); - console.log(pc.yellow('\nThese files have been excluded from the output for security reasons.')); - console.log(pc.yellow('Please review these files for potential sensitive information.')); + logger.log(pc.yellow('\nThese files have been excluded from the output for security reasons.')); + logger.log(pc.yellow('Please review these files for potential sensitive information.')); } }; @@ -53,8 +52,8 @@ export const printTopFiles = ( fileTokenCounts: Record, topFilesLength: number, ) => { - console.log(pc.white(`šŸ“ˆ Top ${topFilesLength} Files by Character Count and Token Count:`)); - console.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); + logger.log(pc.white(`šŸ“ˆ Top ${topFilesLength} Files by Character Count and Token Count:`)); + logger.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); const topFiles = Object.entries(fileCharCounts) .sort((a, b) => b[1] - a[1]) @@ -63,13 +62,13 @@ export const printTopFiles = ( topFiles.forEach(([filePath, charCount], index) => { const tokenCount = fileTokenCounts[filePath]; const indexString = `${index + 1}.`.padEnd(3, ' '); - console.log( + logger.log( `${pc.white(`${indexString}`)} ${pc.white(filePath)} ${pc.dim(`(${charCount} chars, ${tokenCount} tokens)`)}`, ); }); }; export const printCompletion = () => { - console.log(pc.green('šŸŽ‰ All Done!')); - console.log(pc.white('Your repository has been successfully packed.')); + logger.log(pc.green('šŸŽ‰ All Done!')); + logger.log(pc.white('Your repository has been successfully packed.')); }; diff --git a/src/cli/commands/defaultCommandRunner.ts b/src/cli/commands/defaultCommandRunner.ts index 55030a2e..49282618 100644 --- a/src/cli/commands/defaultCommandRunner.ts +++ b/src/cli/commands/defaultCommandRunner.ts @@ -17,7 +17,7 @@ import { printSummary, printTopFiles, printCompletion, printSecurityCheck } from export const runDefaultCommand = async (directory: string, cwd: string, options: CliOptions): Promise => { const version = await getVersion(); - console.log(pc.dim(`\nšŸ“¦ Repopack v${version}\n`)); + logger.log(pc.dim(`\nšŸ“¦ Repopack v${version}\n`)); logger.setVerbose(options.verbose || false); logger.trace('Loaded CLI options:', options); @@ -71,15 +71,15 @@ export const runDefaultCommand = async (directory: string, cwd: string, options: } spinner.succeed('Packing completed successfully!'); - console.log(''); + logger.log(''); if (config.output.topFilesLength > 0) { printTopFiles(packResult.fileCharCounts, packResult.fileTokenCounts, config.output.topFilesLength); - console.log(''); + logger.log(''); } printSecurityCheck(cwd, packResult.suspiciousFilesResults); - console.log(''); + logger.log(''); printSummary( cwd, @@ -89,7 +89,7 @@ export const runDefaultCommand = async (directory: string, cwd: string, options: config.output.filePath, packResult.suspiciousFilesResults, ); - console.log(''); + logger.log(''); printCompletion(); }; diff --git a/src/cli/commands/initCommandRunner.ts b/src/cli/commands/initCommandRunner.ts index 1a15ebdf..d9a2effc 100644 --- a/src/cli/commands/initCommandRunner.ts +++ b/src/cli/commands/initCommandRunner.ts @@ -12,8 +12,8 @@ export const runInitCommand = async (rootDir: string): Promise => { try { // Check if the config file already exists await fs.access(configPath); - console.log(pc.yellow('A repopack.config.json file already exists in this directory.')); - console.log(pc.yellow('If you want to create a new one, please delete or rename the existing file first.')); + logger.warn('A repopack.config.json file already exists in this directory.'); + logger.warn('If you want to create a new one, please delete or rename the existing file first.'); return; } catch { // File doesn't exist, so we can proceed @@ -22,6 +22,8 @@ export const runInitCommand = async (rootDir: string): Promise => { intro(pc.bold(pc.cyan('Welcome to Repopack!'))); try { + let isCancelled = false; + const options = await group( { outputFilePath: () => @@ -44,11 +46,15 @@ export const runInitCommand = async (rootDir: string): Promise => { { onCancel: () => { cancel('Configuration cancelled.'); - process.exit(0); + isCancelled = true; }, }, ); + if (isCancelled) { + return; + } + const config: RepopackConfigFile = { ...defaultConfig, output: { diff --git a/src/cli/commands/versionCommandRunner.ts b/src/cli/commands/versionCommandRunner.ts index 4d2aa040..44b3be19 100644 --- a/src/cli/commands/versionCommandRunner.ts +++ b/src/cli/commands/versionCommandRunner.ts @@ -1,6 +1,7 @@ import { getVersion } from '../../core/file/packageJsonParser.js'; +import { logger } from '../../shared/logger.js'; export const runVersionCommand = async (): Promise => { const version = await getVersion(); - console.log(version); + logger.log(version); }; diff --git a/src/core/file/fileSanitizer.ts b/src/core/file/fileSanitizer.ts index 19410a04..5546870c 100644 --- a/src/core/file/fileSanitizer.ts +++ b/src/core/file/fileSanitizer.ts @@ -49,8 +49,15 @@ export const sanitizeFile = async ( return null; } - const encoding = jschardet.detect(buffer).encoding || 'utf-8'; - let content = iconv.decode(buffer, encoding); + let content: string; + + try { + const encoding = jschardet.detect(buffer).encoding || 'utf-8'; + content = iconv.decode(buffer, encoding); + } catch (error) { + logger.warn(`Failed to decode file: ${filePath}`, error); + return null; + } if (config.output.removeComments) { const manipulator = getFileManipulator(filePath); diff --git a/src/core/file/packageJsonParser.ts b/src/core/file/packageJsonParser.ts index 4234a792..b0026c32 100644 --- a/src/core/file/packageJsonParser.ts +++ b/src/core/file/packageJsonParser.ts @@ -1,19 +1,20 @@ import path from 'node:path'; import * as fs from 'node:fs/promises'; import * as url from 'node:url'; +import { logger } from '../../shared/logger.js'; export const getVersion = async (): Promise => { try { const packageJson = await parsePackageJson(); if (!packageJson.version) { - console.warn('No version found in package.json'); + logger.warn('No version found in package.json'); return 'unknown'; } return packageJson.version; } catch (error) { - console.error('Error reading package.json:', error); + logger.error('Error reading package.json:', error); return 'unknown'; } }; diff --git a/src/shared/errorHandler.ts b/src/shared/errorHandler.ts index ae9de165..51282b41 100644 --- a/src/shared/errorHandler.ts +++ b/src/shared/errorHandler.ts @@ -1,4 +1,3 @@ -import process from 'node:process'; import { logger } from './logger.js'; export class RepopackError extends Error { @@ -19,5 +18,4 @@ export const handleError = (error: unknown): void => { } logger.info('For more help, please visit: https://github.com/yamadashy/repopack/issues'); - process.exit(1); }; diff --git a/src/shared/logger.ts b/src/shared/logger.ts index 8cbafe8f..d4b8d9ad 100644 --- a/src/shared/logger.ts +++ b/src/shared/logger.ts @@ -8,47 +8,43 @@ class Logger { this.isVerbose = value; } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - error(...args: any[]) { + error(...args: unknown[]) { console.error(pc.red(this.formatArgs(args))); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - warn(...args: any[]) { + warn(...args: unknown[]) { console.log(pc.yellow(this.formatArgs(args))); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - success(...args: any[]) { + success(...args: unknown[]) { console.log(pc.green(this.formatArgs(args))); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - info(...args: any[]) { + info(...args: unknown[]) { console.log(pc.cyan(this.formatArgs(args))); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - note(...args: string[]) { + note(...args: unknown[]) { console.log(pc.dim(this.formatArgs(args))); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any debug(...args: unknown[]) { if (this.isVerbose) { console.log(pc.blue(this.formatArgs(args))); } } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - trace(...args: any[]) { + trace(...args: unknown[]) { if (this.isVerbose) { console.log(pc.gray(this.formatArgs(args))); } } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - private formatArgs(args: any[]): string { + log(...args: unknown[]) { + console.log(...args); + } + + private formatArgs(args: unknown[]): string { return args .map((arg) => (typeof arg === 'object' ? util.inspect(arg, { depth: null, colors: true }) : arg)) .join(' '); diff --git a/tests/cli/commands/defaultCommandRunner.test.ts b/tests/cli/commands/defaultCommandRunner.test.ts new file mode 100644 index 00000000..98235da1 --- /dev/null +++ b/tests/cli/commands/defaultCommandRunner.test.ts @@ -0,0 +1,120 @@ +import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; +import { runDefaultCommand } from '../../../src/cli/commands/defaultCommandRunner.js'; +import * as packager from '../../../src/core/packager.js'; +import * as configLoader from '../../../src/config/configLoader.js'; +import * as packageJsonParser from '../../../src/core/file/packageJsonParser.js'; +import * as logger from '../../../src/shared/logger.js'; +import { CliOptions } from '../../../src/cli/cliRunner.js'; + +vi.mock('../../../src/core/packager'); +vi.mock('../../../src/config/configLoader'); +vi.mock('../../../src/core/file/packageJsonParser'); +vi.mock('../../../src/shared/logger'); + +describe('defaultCommandRunner', () => { + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(packageJsonParser.getVersion).mockResolvedValue('1.0.0'); + vi.mocked(configLoader.loadFileConfig).mockResolvedValue({}); + vi.mocked(configLoader.mergeConfigs).mockReturnValue({ + output: { + filePath: 'output.txt', + style: 'plain', + topFilesLength: 5, + showLineNumbers: false, + removeComments: false, + removeEmptyLines: false, + }, + ignore: { + useGitignore: true, + useDefaultPatterns: true, + customPatterns: [], + }, + include: [], + }); + vi.mocked(packager.pack).mockResolvedValue({ + totalFiles: 10, + totalCharacters: 1000, + totalTokens: 200, + fileCharCounts: {}, + fileTokenCounts: {}, + suspiciousFilesResults: [], + }); + }); + + afterEach(() => { + vi.resetAllMocks(); + }); + + it('should run the default command successfully', async () => { + const options: CliOptions = { + output: 'custom-output.txt', + verbose: true, + }; + + await runDefaultCommand('.', process.cwd(), options); + + expect(packageJsonParser.getVersion).toHaveBeenCalled(); + expect(logger.logger.setVerbose).toHaveBeenCalledWith(true); + expect(configLoader.loadFileConfig).toHaveBeenCalled(); + expect(configLoader.mergeConfigs).toHaveBeenCalled(); + expect(packager.pack).toHaveBeenCalled(); + }); + + it('should handle custom include patterns', async () => { + const options: CliOptions = { + include: '*.js,*.ts', + }; + + await runDefaultCommand('.', process.cwd(), options); + + expect(configLoader.mergeConfigs).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + include: ['*.js', '*.ts'], + }), + ); + }); + + it('should handle custom ignore patterns', async () => { + const options: CliOptions = { + ignore: 'node_modules,*.log', + }; + + await runDefaultCommand('.', process.cwd(), options); + + expect(configLoader.mergeConfigs).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + ignore: { + customPatterns: ['node_modules', '*.log'], + }, + }), + ); + }); + + it('should handle custom output style', async () => { + const options: CliOptions = { + style: 'xml', + }; + + await runDefaultCommand('.', process.cwd(), options); + + expect(configLoader.mergeConfigs).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + output: expect.objectContaining({ + style: 'xml', + }), + }), + ); + }); + + it('should handle errors gracefully', async () => { + vi.mocked(packager.pack).mockRejectedValue(new Error('Test error')); + + const options: CliOptions = {}; + + await expect(runDefaultCommand('.', process.cwd(), options)).rejects.toThrow('Test error'); + }); +}); diff --git a/tests/cli/commands/initCommandRunner.test.ts b/tests/cli/commands/initCommandRunner.test.ts new file mode 100644 index 00000000..0dc4e4e3 --- /dev/null +++ b/tests/cli/commands/initCommandRunner.test.ts @@ -0,0 +1,73 @@ +import * as fs from 'node:fs/promises'; +import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; +import * as prompts from '@clack/prompts'; +import { runInitCommand } from '../../../src/cli/commands/initCommandRunner.js'; +import { logger } from '../../../src/shared/logger.js'; + +vi.mock('node:fs/promises'); +vi.mock('@clack/prompts'); + +describe('initCommandRunner', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + afterEach(() => { + vi.resetAllMocks(); + }); + + it('should create a new config file when one does not exist', async () => { + vi.mocked(fs.access).mockRejectedValue(new Error('File does not exist')); + vi.mocked(prompts.group).mockResolvedValue({ + outputFilePath: 'custom-output.txt', + outputStyle: 'xml', + }); + + await runInitCommand('/test/dir'); + + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/dir/repopack.config.json', + expect.stringContaining('"filePath": "custom-output.txt"'), + ); + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/dir/repopack.config.json', + expect.stringContaining('"style": "xml"'), + ); + }); + + it('should not create a new config file when one already exists', async () => { + vi.mocked(fs.access).mockResolvedValue(undefined); + + const loggerSpy = vi.spyOn(logger, 'warn').mockImplementation(vi.fn()); + await runInitCommand('/test/dir'); + + expect(fs.writeFile).not.toHaveBeenCalled(); + expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('already exists')); + }); + + it('should handle user cancellation', async () => { + vi.mocked(fs.access).mockRejectedValue(new Error('File does not exist')); + vi.mocked(prompts.group).mockImplementation(() => { + throw new Error('User cancelled'); + }); + + const loggerSpy = vi.spyOn(logger, 'error').mockImplementation(vi.fn()); + + await runInitCommand('/test/dir'); + + expect(fs.writeFile).not.toHaveBeenCalled(); + + expect(loggerSpy).toHaveBeenCalledWith('Failed to create repopack.config.json:', new Error('User cancelled')); + }); + + it('should handle errors when writing the config file', async () => { + vi.mocked(fs.access).mockRejectedValue(new Error('File does not exist')); + vi.mocked(prompts.group).mockResolvedValue({ + outputFilePath: 'custom-output.txt', + outputStyle: 'plain', + }); + vi.mocked(fs.writeFile).mockRejectedValue(new Error('Write error')); + + await runInitCommand('/test/dir'); + }); +}); diff --git a/tests/cli/commands/versionCommandRunner.test.ts b/tests/cli/commands/versionCommandRunner.test.ts new file mode 100644 index 00000000..a748e35f --- /dev/null +++ b/tests/cli/commands/versionCommandRunner.test.ts @@ -0,0 +1,26 @@ +import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; +import { runVersionCommand } from '../../../src/cli/commands/versionCommandRunner.js'; +import * as packageJsonParser from '../../../src/core/file/packageJsonParser.js'; +import { logger } from '../../../src/shared/logger.js'; + +vi.mock('../../../src/core/file/packageJsonParser'); + +describe('versionCommandRunner', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + afterEach(() => { + vi.resetAllMocks(); + }); + + it('should print the correct version', async () => { + vi.mocked(packageJsonParser.getVersion).mockResolvedValue('1.2.3'); + + const loggerSpy = vi.spyOn(logger, 'log').mockImplementation(vi.fn()); + await runVersionCommand(); + + expect(packageJsonParser.getVersion).toHaveBeenCalled(); + expect(loggerSpy).toHaveBeenCalledWith('1.2.3'); + }); +}); diff --git a/tests/config/configLoader.test.ts b/tests/config/configLoader.test.ts index baf1bd17..32e12570 100644 --- a/tests/config/configLoader.test.ts +++ b/tests/config/configLoader.test.ts @@ -3,6 +3,7 @@ import { Stats } from 'node:fs'; import { expect, test, describe, vi, beforeEach } from 'vitest'; import { loadFileConfig, mergeConfigs } from '../../src/config/configLoader.js'; import { RepopackConfigFile, RepopackConfigCli } from '../../src/config/configTypes.js'; +import { logger } from '../../src/shared/logger.js'; vi.mock('fs/promises'); vi.mock('../../src/utils/logger', () => ({ @@ -31,10 +32,14 @@ describe('configLoader', () => { }); test('should return an empty object if no config file is found', async () => { + const loggerSpy = vi.spyOn(logger, 'note').mockImplementation(vi.fn()); + vi.mocked(fs.stat).mockRejectedValue(new Error('File not found')); const result = await loadFileConfig(process.cwd(), null); expect(result).toEqual({}); + + expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('No custom config found')); }); test('should throw an error for invalid JSON', async () => { diff --git a/tests/core/file/fileSanitizer.test.ts b/tests/core/file/fileSanitizer.test.ts index c465839e..58e7f676 100644 --- a/tests/core/file/fileSanitizer.test.ts +++ b/tests/core/file/fileSanitizer.test.ts @@ -15,6 +15,9 @@ describe('fileSanitizer', () => { const mockContent = ' Some file content \n'; vi.mocked(fs.readFile).mockResolvedValue(mockContent); + // Suppress iconv-lite error + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + const mockConfig = createMockConfig(); const result = await sanitizeFile('/path/to/file.txt', mockConfig); @@ -35,7 +38,6 @@ describe('fileSanitizer', () => { }, }); const result = postprocessContent(content, config); - expect(result).toBe('Some content with whitespace'); }); @@ -88,11 +90,11 @@ describe('fileSanitizer', () => { test('sanitizeFiles should process multiple files', async () => { const mockConfig = createMockConfig(); const mockFilePaths = ['file1.txt', 'dir/file2.txt']; - const mockRootDir = '/mock/root'; + const mockCwd = '/mock/root'; vi.mocked(fs.readFile).mockResolvedValueOnce('content1').mockResolvedValueOnce('content2'); - const result = await sanitizeFiles(mockFilePaths, mockRootDir, mockConfig); + const result = await sanitizeFiles(mockFilePaths, mockCwd, mockConfig); expect(result).toEqual([ { path: 'file1.txt', content: 'content1' }, @@ -100,7 +102,7 @@ describe('fileSanitizer', () => { ]); expect(fs.readFile).toHaveBeenCalledTimes(2); - expect(vi.mocked(fs.readFile).mock.calls[0][0]).toBe(path.join(mockRootDir, 'file1.txt')); - expect(vi.mocked(fs.readFile).mock.calls[1][0]).toBe(path.join(mockRootDir, 'dir/file2.txt')); + expect(vi.mocked(fs.readFile).mock.calls[0][0]).toBe(path.join(mockCwd, 'file1.txt')); + expect(vi.mocked(fs.readFile).mock.calls[1][0]).toBe(path.join(mockCwd, 'dir/file2.txt')); }); }); diff --git a/tests/core/file/packageJsonParser.test.ts b/tests/core/file/packageJsonParser.test.ts index 642c931d..eade432f 100644 --- a/tests/core/file/packageJsonParser.test.ts +++ b/tests/core/file/packageJsonParser.test.ts @@ -3,6 +3,7 @@ import * as url from 'node:url'; import path from 'node:path'; import { expect, test, describe, vi, beforeEach, afterEach } from 'vitest'; import { getVersion } from '../../../src/core/file/packageJsonParser.js'; +import { logger } from '../../../src/shared/logger.js'; vi.mock('fs/promises'); vi.mock('url'); @@ -40,11 +41,15 @@ describe('packageJsonParser', () => { name: 'repopack', }; + const loggerSpy = vi.spyOn(logger, 'warn').mockImplementation(vi.fn()); + vi.mocked(url.fileURLToPath).mockReturnValue('/mock/path/to/src/core/file2'); vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(mockPackageJson)); const version = await getVersion(); + expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('No version found in package.json')); + expect(version).toBe('unknown'); }); }); diff --git a/tests/shared/logger.test.ts b/tests/shared/logger.test.ts new file mode 100644 index 00000000..5d95ea25 --- /dev/null +++ b/tests/shared/logger.test.ts @@ -0,0 +1,89 @@ +import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; +import { logger } from '../../src/shared/logger.js'; + +vi.mock('picocolors', () => ({ + default: { + red: vi.fn((str) => `RED:${str}`), + yellow: vi.fn((str) => `YELLOW:${str}`), + green: vi.fn((str) => `GREEN:${str}`), + cyan: vi.fn((str) => `CYAN:${str}`), + dim: vi.fn((str) => `DIM:${str}`), + blue: vi.fn((str) => `BLUE:${str}`), + gray: vi.fn((str) => `GRAY:${str}`), + }, +})); + +describe('logger', () => { + beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + vi.spyOn(console, 'log').mockImplementation(vi.fn()); + logger.setVerbose(false); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should log error messages', () => { + logger.error('Error message'); + expect(console.error).toHaveBeenCalledWith('RED:Error message'); + }); + + it('should log warning messages', () => { + logger.warn('Warning message'); + expect(console.log).toHaveBeenCalledWith('YELLOW:Warning message'); + }); + + it('should log success messages', () => { + logger.success('Success message'); + expect(console.log).toHaveBeenCalledWith('GREEN:Success message'); + }); + + it('should log info messages', () => { + logger.info('Info message'); + expect(console.log).toHaveBeenCalledWith('CYAN:Info message'); + }); + + it('should log note messages', () => { + logger.note('Note message'); + expect(console.log).toHaveBeenCalledWith('DIM:Note message'); + }); + + it('should log log messages', () => { + logger.log('Note message'); + expect(console.log).toHaveBeenCalledWith('Note message'); + }); + + it('should not log debug messages when verbose is false', () => { + logger.debug('Debug message'); + expect(console.log).not.toHaveBeenCalled(); + }); + + it('should log debug messages when verbose is true', () => { + logger.setVerbose(true); + logger.debug('Debug message'); + expect(console.log).toHaveBeenCalledWith('BLUE:Debug message'); + }); + + it('should not log trace messages when verbose is false', () => { + logger.trace('Trace message'); + expect(console.log).not.toHaveBeenCalled(); + }); + + it('should log trace messages when verbose is true', () => { + logger.setVerbose(true); + logger.trace('Trace message'); + expect(console.log).toHaveBeenCalledWith('GRAY:Trace message'); + }); + + it('should format object arguments correctly', () => { + const obj = { key: 'value' }; + logger.info('Object:', obj); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('CYAN:Object: ')); + }); + + it('should handle multiple arguments', () => { + logger.info('Multiple', 'arguments', 123); + expect(console.log).toHaveBeenCalledWith('CYAN:Multiple arguments 123'); + }); +}); diff --git a/vite.config.mts b/vite.config.ts similarity index 100% rename from vite.config.mts rename to vite.config.ts From 9f131078851a8cc20b7cd8436c382c98ff10cc8f Mon Sep 17 00:00:00 2001 From: Kazuki Yamada Date: Mon, 12 Aug 2024 18:28:35 +0900 Subject: [PATCH 2/4] refact: Refactor file processing pipeline and improve security check --- src/cli/cliPrinter.ts | 8 +- src/core/file/fileCollector.ts | 47 ++++++++++ src/core/file/fileManipulater.ts | 22 ++++- src/core/file/fileProcessor.ts | 27 ++++++ src/core/file/fileSanitizer.ts | 88 ------------------ src/core/file/fileTypes.ts | 9 ++ src/core/output/outputGenerator.ts | 10 +- src/core/output/outputGeneratorTypes.ts | 4 +- src/core/output/plainStyleGenerator.ts | 4 +- src/core/output/xmlStyleGenerator.ts | 4 +- src/core/packager.ts | 44 +++++---- src/core/security/securityCheckRunner.ts | 23 +++-- tests/core/file/fileCollector.test.ts | 68 ++++++++++++++ tests/core/file/fileProcessor.test.ts | 90 ++++++++++++++++++ tests/core/file/fileSanitizer.test.ts | 108 ---------------------- tests/core/output/outputGenerator.test.ts | 5 +- tests/core/packager.test.ts | 100 +++++++++++--------- 17 files changed, 375 insertions(+), 286 deletions(-) create mode 100644 src/core/file/fileCollector.ts create mode 100644 src/core/file/fileProcessor.ts delete mode 100644 src/core/file/fileSanitizer.ts create mode 100644 src/core/file/fileTypes.ts create mode 100644 tests/core/file/fileCollector.test.ts create mode 100644 tests/core/file/fileProcessor.test.ts delete mode 100644 tests/core/file/fileSanitizer.test.ts diff --git a/src/cli/cliPrinter.ts b/src/cli/cliPrinter.ts index eb5db1a0..30488a7d 100644 --- a/src/cli/cliPrinter.ts +++ b/src/cli/cliPrinter.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import pc from 'picocolors'; -import type { SecretLintCoreResult } from '@secretlint/types'; import { logger } from '../shared/logger.js'; +import { SuspiciousFileResult } from '../core/security/securityCheckRunner.js'; export const printSummary = ( rootDir: string, @@ -9,7 +9,7 @@ export const printSummary = ( totalCharacters: number, totalTokens: number, outputPath: string, - suspiciousFilesResults: SecretLintCoreResult[], + suspiciousFilesResults: SuspiciousFileResult[], ) => { const relativeOutputPath = path.relative(rootDir, outputPath); @@ -29,7 +29,7 @@ export const printSummary = ( logger.log(`${pc.white(' Security:')} ${pc.white(securityCheckMessage)}`); }; -export const printSecurityCheck = (rootDir: string, suspiciousFilesResults: SecretLintCoreResult[]) => { +export const printSecurityCheck = (rootDir: string, suspiciousFilesResults: SuspiciousFileResult[]) => { logger.log(pc.white('šŸ”Ž Security Check:')); logger.log(pc.dim('ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€')); @@ -40,7 +40,7 @@ export const printSecurityCheck = (rootDir: string, suspiciousFilesResults: Secr suspiciousFilesResults.forEach((suspiciousFilesResult, index) => { const relativeFilePath = path.relative(rootDir, suspiciousFilesResult.filePath); logger.log(`${pc.white(`${index + 1}.`)} ${pc.white(relativeFilePath)}`); - logger.log(pc.dim(' - ' + suspiciousFilesResult.messages.map((message) => message.message).join('\n - '))); + logger.log(pc.dim(' - ' + suspiciousFilesResult.messages.join('\n - '))); }); logger.log(pc.yellow('\nThese files have been excluded from the output for security reasons.')); logger.log(pc.yellow('Please review these files for potential sensitive information.')); diff --git a/src/core/file/fileCollector.ts b/src/core/file/fileCollector.ts new file mode 100644 index 00000000..9736b31e --- /dev/null +++ b/src/core/file/fileCollector.ts @@ -0,0 +1,47 @@ +import * as fs from 'node:fs/promises'; +import path from 'node:path'; +import { isBinary } from 'istextorbinary'; +import jschardet from 'jschardet'; +import iconv from 'iconv-lite'; +import { logger } from '../../shared/logger.js'; +import { RawFile } from './fileTypes.js'; + +export const collectFiles = async (filePaths: string[], rootDir: string): Promise => { + const rawFiles: RawFile[] = []; + + for (const filePath of filePaths) { + const fullPath = path.join(rootDir, filePath); + const content = await readRawFile(fullPath); + if (content) { + rawFiles.push({ path: filePath, content }); + } + } + + return rawFiles; +}; + +const readRawFile = async (filePath: string): Promise => { + if (isBinary(filePath)) { + logger.debug(`Skipping binary file: ${filePath}`); + return null; + } + + logger.trace(`Processing file: ${filePath}`); + + try { + const buffer = await fs.readFile(filePath); + + if (isBinary(null, buffer)) { + logger.debug(`Skipping binary file (content check): ${filePath}`); + return null; + } + + const encoding = jschardet.detect(buffer).encoding || 'utf-8'; + const content = iconv.decode(buffer, encoding); + + return content; + } catch (error) { + logger.warn(`Failed to read file: ${filePath}`, error); + return null; + } +}; diff --git a/src/core/file/fileManipulater.ts b/src/core/file/fileManipulater.ts index 68c60320..11f37368 100644 --- a/src/core/file/fileManipulater.ts +++ b/src/core/file/fileManipulater.ts @@ -3,14 +3,29 @@ import strip from 'strip-comments'; interface FileManipulator { removeComments(content: string): string; + removeEmptyLines(content: string): string; } const rtrimLines = (content: string): string => content.replace(/[ \t]+$/gm, ''); -class StripCommentsManipulator implements FileManipulator { +class BaseManipulator implements FileManipulator { + removeComments(content: string): string { + return content; + } + + removeEmptyLines(content: string): string { + return content + .split('\n') + .filter((line) => line.trim() !== '') + .join('\n'); + } +} + +class StripCommentsManipulator extends BaseManipulator { private language: string; constructor(language: string) { + super(); this.language = language; } @@ -20,7 +35,7 @@ class StripCommentsManipulator implements FileManipulator { } } -class PythonManipulator implements FileManipulator { +class PythonManipulator extends BaseManipulator { removeComments(content: string): string { // First, use strip-comments to remove standard comments let result = strip(content, { language: 'python', preserveNewlines: true }); @@ -36,10 +51,11 @@ class PythonManipulator implements FileManipulator { } } -class CompositeManipulator implements FileManipulator { +class CompositeManipulator extends BaseManipulator { private manipulators: FileManipulator[]; constructor(...manipulators: FileManipulator[]) { + super(); this.manipulators = manipulators; } diff --git a/src/core/file/fileProcessor.ts b/src/core/file/fileProcessor.ts new file mode 100644 index 00000000..dc0d76dc --- /dev/null +++ b/src/core/file/fileProcessor.ts @@ -0,0 +1,27 @@ +import { RepopackConfigMerged } from '../../config/configTypes.js'; +import { getFileManipulator } from './fileManipulater.js'; +import { ProcessedFile, RawFile } from './fileTypes.js'; + +export const processFiles = (rawFiles: RawFile[], config: RepopackConfigMerged): ProcessedFile[] => { + return rawFiles.map((rawFile) => ({ + path: rawFile.path, + content: processContent(rawFile.content, rawFile.path, config), + })); +}; + +export const processContent = (content: string, filePath: string, config: RepopackConfigMerged): string => { + let processedContent = content; + const manipulator = getFileManipulator(filePath); + + if (config.output.removeComments && manipulator) { + processedContent = manipulator.removeComments(processedContent); + } + + if (config.output.removeEmptyLines && manipulator) { + processedContent = manipulator.removeEmptyLines(processedContent); + } + + processedContent = processedContent.trim(); + + return processedContent; +}; diff --git a/src/core/file/fileSanitizer.ts b/src/core/file/fileSanitizer.ts deleted file mode 100644 index 5546870c..00000000 --- a/src/core/file/fileSanitizer.ts +++ /dev/null @@ -1,88 +0,0 @@ -import * as fs from 'node:fs/promises'; -import path from 'node:path'; -import { isBinary } from 'istextorbinary'; -import jschardet from 'jschardet'; -import iconv from 'iconv-lite'; -import { RepopackConfigMerged } from '../../config/configTypes.js'; -import { logger } from '../../shared/logger.js'; -import { getFileManipulator } from './fileManipulater.js'; - -export interface SanitizedFile { - path: string; - content: string; -} - -export const sanitizeFiles = async ( - filePaths: string[], - rootDir: string, - config: RepopackConfigMerged, -): Promise => { - const sanitizedFiles: SanitizedFile[] = []; - - for (const filePath of filePaths) { - const fullPath = path.join(rootDir, filePath); - const content = await sanitizeFile(fullPath, config); - if (content) { - sanitizedFiles.push({ path: filePath, content }); - } - } - - return sanitizedFiles; -}; - -export const sanitizeFile = async ( - filePath: string, - config: RepopackConfigMerged, - fsModule = fs, -): Promise => { - if (isBinary(filePath)) { - logger.debug(`Skipping binary by path. path: ${filePath}`); - return null; - } - - logger.trace(`Processing file: ${filePath}`); - - const buffer = await fsModule.readFile(filePath); - - if (isBinary(null, buffer)) { - logger.debug(`Skipping binary by content. path: ${filePath}`); - return null; - } - - let content: string; - - try { - const encoding = jschardet.detect(buffer).encoding || 'utf-8'; - content = iconv.decode(buffer, encoding); - } catch (error) { - logger.warn(`Failed to decode file: ${filePath}`, error); - return null; - } - - if (config.output.removeComments) { - const manipulator = getFileManipulator(filePath); - if (manipulator) { - content = manipulator.removeComments(content); - } - } - - content = postprocessContent(content, config); - - return content; -}; - -export const postprocessContent = (content: string, config: RepopackConfigMerged): string => { - content = content.trim(); - - if (config.output.removeEmptyLines) { - content = removeEmptyLines(content); - } - - return content; -}; - -const removeEmptyLines = (content: string): string => - content - .split('\n') - .filter((line) => line.trim() !== '') - .join('\n'); diff --git a/src/core/file/fileTypes.ts b/src/core/file/fileTypes.ts new file mode 100644 index 00000000..5bcbbc77 --- /dev/null +++ b/src/core/file/fileTypes.ts @@ -0,0 +1,9 @@ +export interface RawFile { + path: string; + content: string; +} + +export interface ProcessedFile { + path: string; + content: string; +} diff --git a/src/core/output/outputGenerator.ts b/src/core/output/outputGenerator.ts index d5ca8bcc..d74b883f 100644 --- a/src/core/output/outputGenerator.ts +++ b/src/core/output/outputGenerator.ts @@ -1,16 +1,16 @@ import { RepopackConfigMerged } from '../../config/configTypes.js'; -import { SanitizedFile } from '../file/fileSanitizer.js'; import { generateTreeString } from '../file/fileTreeGenerator.js'; +import { ProcessedFile } from '../file/fileTypes.js'; import { generateXmlStyle } from './xmlStyleGenerator.js'; import { generatePlainStyle } from './plainStyleGenerator.js'; import { OutputGeneratorContext } from './outputGeneratorTypes.js'; export const generateOutput = async ( config: RepopackConfigMerged, - sanitizedFiles: SanitizedFile[], + processedFiles: ProcessedFile[], allFilePaths: string[], ): Promise => { - const outputGeneratorContext = buildOutputGeneratorContext(config, allFilePaths, sanitizedFiles); + const outputGeneratorContext = buildOutputGeneratorContext(config, allFilePaths, processedFiles); let output: string; switch (config.output.style) { @@ -27,10 +27,10 @@ export const generateOutput = async ( export const buildOutputGeneratorContext = ( config: RepopackConfigMerged, allFilePaths: string[], - sanitizedFiles: SanitizedFile[], + processedFiles: ProcessedFile[], ): OutputGeneratorContext => ({ generationDate: new Date().toISOString(), treeString: generateTreeString(allFilePaths), - sanitizedFiles, + processedFiles, config, }); diff --git a/src/core/output/outputGeneratorTypes.ts b/src/core/output/outputGeneratorTypes.ts index a4969f27..07d72e84 100644 --- a/src/core/output/outputGeneratorTypes.ts +++ b/src/core/output/outputGeneratorTypes.ts @@ -1,9 +1,9 @@ import { RepopackConfigMerged } from '../../config/configTypes.js'; -import { SanitizedFile } from '../file/fileSanitizer.js'; +import { ProcessedFile } from '../file/fileTypes.js'; export interface OutputGeneratorContext { generationDate: string; treeString: string; - sanitizedFiles: SanitizedFile[]; + processedFiles: ProcessedFile[]; config: RepopackConfigMerged; } diff --git a/src/core/output/plainStyleGenerator.ts b/src/core/output/plainStyleGenerator.ts index b16c3ad9..4506a0d4 100644 --- a/src/core/output/plainStyleGenerator.ts +++ b/src/core/output/plainStyleGenerator.ts @@ -4,7 +4,7 @@ const PLAIN_SEPARATOR = '='.repeat(16); const PLAIN_LONG_SEPARATOR = '='.repeat(64); export const generatePlainStyle = (data: OutputGeneratorContext): string => { - const { generationDate, treeString, sanitizedFiles, config } = data; + const { generationDate, treeString, processedFiles, config } = data; let output = `${PLAIN_LONG_SEPARATOR} Repopack Output File @@ -72,7 +72,7 @@ ${PLAIN_LONG_SEPARATOR} `; - for (const file of sanitizedFiles) { + for (const file of processedFiles) { output += `${PLAIN_SEPARATOR} File: ${file.path} ${PLAIN_SEPARATOR} diff --git a/src/core/output/xmlStyleGenerator.ts b/src/core/output/xmlStyleGenerator.ts index d635a254..c5ff7cc7 100644 --- a/src/core/output/xmlStyleGenerator.ts +++ b/src/core/output/xmlStyleGenerator.ts @@ -1,7 +1,7 @@ import { OutputGeneratorContext } from './outputGeneratorTypes.js'; export const generateXmlStyle = (data: OutputGeneratorContext): string => { - const { generationDate, treeString, sanitizedFiles, config } = data; + const { generationDate, treeString, processedFiles, config } = data; let xml = ` @@ -65,7 +65,7 @@ ${treeString} `; - for (const file of sanitizedFiles) { + for (const file of processedFiles) { xml += ` ${file.content} diff --git a/src/core/packager.ts b/src/core/packager.ts index 7c6c8271..5aedbb7c 100644 --- a/src/core/packager.ts +++ b/src/core/packager.ts @@ -1,17 +1,19 @@ import * as fs from 'node:fs/promises'; import path from 'node:path'; -import type { SecretLintCoreResult } from '@secretlint/types'; import { RepopackConfigMerged } from '../config/configTypes.js'; -import { sanitizeFiles as defaultSanitizeFiles } from './file/fileSanitizer.js'; import { generateOutput as defaultGenerateOutput } from './output/outputGenerator.js'; -import { runSecurityCheck } from './security/securityCheckRunner.js'; +import { SuspiciousFileResult, runSecurityCheck as defaultRunSecurityCheck } from './security/securityCheckRunner.js'; import { searchFiles as defaultSearchFiles } from './file/fileSearcher.js'; import { TokenCounter } from './tokenCounter/tokenCounter.js'; +import { collectFiles as defaultCollectFiles } from './file/fileCollector.js'; +import { processFiles as defaultProcessFiles } from './file/fileProcessor.js'; export interface PackDependencies { searchFiles: typeof defaultSearchFiles; + collectFiles: typeof defaultCollectFiles; + processFiles: typeof defaultProcessFiles; + runSecurityCheck: typeof defaultRunSecurityCheck; generateOutput: typeof defaultGenerateOutput; - sanitizeFiles: typeof defaultSanitizeFiles; } export interface PackResult { @@ -20,7 +22,7 @@ export interface PackResult { totalTokens: number; fileCharCounts: Record; fileTokenCounts: Record; - suspiciousFilesResults: SecretLintCoreResult[]; + suspiciousFilesResults: SuspiciousFileResult[]; } export const pack = async ( @@ -28,22 +30,30 @@ export const pack = async ( config: RepopackConfigMerged, deps: PackDependencies = { searchFiles: defaultSearchFiles, + collectFiles: defaultCollectFiles, + processFiles: defaultProcessFiles, + runSecurityCheck: defaultRunSecurityCheck, generateOutput: defaultGenerateOutput, - sanitizeFiles: defaultSanitizeFiles, }, ): Promise => { - // Get all file paths that should be processed + // Get all file paths considering the config const filePaths = await deps.searchFiles(rootDir, config); + // Collect raw files + const rawFiles = await deps.collectFiles(filePaths, rootDir); + // Perform security check and filter out suspicious files - const suspiciousFilesResults = await runSecurityCheck(filePaths, rootDir); - const safeFilePaths = filePaths.filter( - (filePath) => !suspiciousFilesResults.some((result) => result.filePath === path.join(rootDir, filePath)), + const suspiciousFilesResults = await deps.runSecurityCheck(rawFiles); + const safeRawFiles = rawFiles.filter( + (rawFile) => !suspiciousFilesResults.some((result) => result.filePath === rawFile.path), ); + const safeFilePaths = safeRawFiles.map((file) => file.path); + + // Process files (remove comments, etc.) + const processedFiles = deps.processFiles(safeRawFiles, config); - // Sanitize files and generate output - const sanitizedFiles = await deps.sanitizeFiles(safeFilePaths, rootDir, config); - const output = await deps.generateOutput(config, sanitizedFiles, safeFilePaths); + // Generate output + const output = await deps.generateOutput(config, processedFiles, safeFilePaths); // Write output to file const outputPath = path.resolve(rootDir, config.output.filePath); @@ -53,12 +63,12 @@ export const pack = async ( const tokenCounter = new TokenCounter(); // Metrics - const totalFiles = sanitizedFiles.length; - const totalCharacters = sanitizedFiles.reduce((sum, file) => sum + file.content.length, 0); - const totalTokens = sanitizedFiles.reduce((sum, file) => sum + tokenCounter.countTokens(file.content), 0); + const totalFiles = processedFiles.length; + const totalCharacters = processedFiles.reduce((sum, file) => sum + file.content.length, 0); + const totalTokens = processedFiles.reduce((sum, file) => sum + tokenCounter.countTokens(file.content), 0); const fileCharCounts: Record = {}; const fileTokenCounts: Record = {}; - sanitizedFiles.forEach((file) => { + processedFiles.forEach((file) => { fileCharCounts[file.path] = file.content.length; fileTokenCounts[file.path] = tokenCounter.countTokens(file.content); }); diff --git a/src/core/security/securityCheckRunner.ts b/src/core/security/securityCheckRunner.ts index 4153b51c..a60804dc 100644 --- a/src/core/security/securityCheckRunner.ts +++ b/src/core/security/securityCheckRunner.ts @@ -1,21 +1,26 @@ -import path from 'node:path'; -import fs from 'node:fs/promises'; import type { SecretLintCoreConfig, SecretLintCoreResult } from '@secretlint/types'; import { lintSource } from '@secretlint/core'; import { creator } from '@secretlint/secretlint-rule-preset-recommend'; import { logger } from '../../shared/logger.js'; +import { RawFile } from '../file/fileTypes.js'; -export const runSecurityCheck = async (filePaths: string[], rootDir: string): Promise => { +export interface SuspiciousFileResult { + filePath: string; + messages: string[]; +} + +export const runSecurityCheck = async (rawFiles: RawFile[]): Promise => { const secretLintConfig = createSecretLintConfig(); - const suspiciousFilesResults: SecretLintCoreResult[] = []; + const suspiciousFilesResults: SuspiciousFileResult[] = []; - for (const filePath of filePaths) { - const fullPath = path.join(rootDir, filePath); - const content = await fs.readFile(fullPath, 'utf-8'); - const secretLintResult = await runSecretLint(fullPath, content, secretLintConfig); + for (const rawFile of rawFiles) { + const secretLintResult = await runSecretLint(rawFile.path, rawFile.content, secretLintConfig); const isSuspicious = secretLintResult.messages.length > 0; if (isSuspicious) { - suspiciousFilesResults.push(secretLintResult); + suspiciousFilesResults.push({ + filePath: rawFile.path, + messages: secretLintResult.messages.map((message) => message.message), + }); } } diff --git a/tests/core/file/fileCollector.test.ts b/tests/core/file/fileCollector.test.ts new file mode 100644 index 00000000..e93ffdcc --- /dev/null +++ b/tests/core/file/fileCollector.test.ts @@ -0,0 +1,68 @@ +import * as fs from 'node:fs/promises'; +import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; +import { isBinary } from 'istextorbinary'; +import jschardet from 'jschardet'; +import iconv from 'iconv-lite'; +import { collectFiles } from '../../../src/core/file/fileCollector.js'; +import { logger } from '../../../src/shared/logger.js'; + +vi.mock('node:fs/promises'); +vi.mock('istextorbinary'); +vi.mock('jschardet'); +vi.mock('iconv-lite'); +vi.mock('../../../src/shared/logger'); + +describe('fileCollector', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + afterEach(() => { + vi.resetAllMocks(); + }); + + it('should collect non-binary files', async () => { + const mockFilePaths = ['file1.txt', 'file2.txt']; + const mockRootDir = '/root'; + + vi.mocked(isBinary).mockReturnValue(false); + vi.mocked(fs.readFile).mockResolvedValue(Buffer.from('file content')); + vi.mocked(jschardet.detect).mockReturnValue({ encoding: 'utf-8', confidence: 0.99 }); + vi.mocked(iconv.decode).mockReturnValue('decoded content'); + + const result = await collectFiles(mockFilePaths, mockRootDir); + + expect(result).toEqual([ + { path: 'file1.txt', content: 'decoded content' }, + { path: 'file2.txt', content: 'decoded content' }, + ]); + }); + + it('should skip binary files', async () => { + const mockFilePaths = ['binary.bin', 'text.txt']; + const mockRootDir = '/root'; + + vi.mocked(isBinary).mockReturnValueOnce(true).mockReturnValueOnce(false); + vi.mocked(fs.readFile).mockResolvedValue(Buffer.from('file content')); + vi.mocked(jschardet.detect).mockReturnValue({ encoding: 'utf-8', confidence: 0.99 }); + vi.mocked(iconv.decode).mockReturnValue('decoded content'); + + const result = await collectFiles(mockFilePaths, mockRootDir); + + expect(result).toEqual([{ path: 'text.txt', content: 'decoded content' }]); + expect(logger.debug).toHaveBeenCalledWith('Skipping binary file: /root/binary.bin'); + }); + + it('should handle file read errors', async () => { + const mockFilePaths = ['error.txt']; + const mockRootDir = '/root'; + + vi.mocked(isBinary).mockReturnValue(false); + vi.mocked(fs.readFile).mockRejectedValue(new Error('Read error')); + + const result = await collectFiles(mockFilePaths, mockRootDir); + + expect(result).toEqual([]); + expect(logger.warn).toHaveBeenCalledWith('Failed to read file: /root/error.txt', expect.any(Error)); + }); +}); diff --git a/tests/core/file/fileProcessor.test.ts b/tests/core/file/fileProcessor.test.ts new file mode 100644 index 00000000..8435325a --- /dev/null +++ b/tests/core/file/fileProcessor.test.ts @@ -0,0 +1,90 @@ +import { expect, describe, it, vi } from 'vitest'; +import { processFiles, processContent } from '../../../src/core/file/fileProcessor.js'; +import { getFileManipulator } from '../../../src/core/file/fileManipulater.js'; +import { RepopackConfigMerged } from '../../../src/config/configTypes.js'; +import { RawFile } from '../../../src/core/file/fileTypes.js'; + +vi.mock('../../../src/core/file/fileManipulater'); + +describe('fileProcessor', () => { + describe('processFiles', () => { + it('should process multiple files', () => { + const mockRawFiles: RawFile[] = [ + { path: 'file1.js', content: '// comment\nconst a = 1;' }, + { path: 'file2.js', content: '/* comment */\nconst b = 2;' }, + ]; + const mockConfig: RepopackConfigMerged = { + output: { + removeComments: true, + removeEmptyLines: true, + }, + } as RepopackConfigMerged; + + vi.mocked(getFileManipulator).mockReturnValue({ + removeComments: (content: string) => content.replace(/\/\/.*|\/\*[\s\S]*?\*\//g, ''), + removeEmptyLines: (content: string) => content.replace(/^\s*[\r\n]/gm, ''), + }); + + const result = processFiles(mockRawFiles, mockConfig); + + expect(result).toEqual([ + { path: 'file1.js', content: 'const a = 1;' }, + { path: 'file2.js', content: 'const b = 2;' }, + ]); + }); + }); + + describe('processContent', () => { + it('should remove comments and empty lines when configured', () => { + const content = '// comment\nconst a = 1;\n\n/* multi-line\ncomment */\nconst b = 2;'; + const filePath = 'test.js'; + const config: RepopackConfigMerged = { + output: { + removeComments: true, + removeEmptyLines: true, + }, + } as RepopackConfigMerged; + + vi.mocked(getFileManipulator).mockReturnValue({ + removeComments: (content: string) => content.replace(/\/\/.*|\/\*[\s\S]*?\*\//g, ''), + removeEmptyLines: (content: string) => content.replace(/^\s*[\r\n]/gm, ''), + }); + + const result = processContent(content, filePath, config); + + expect(result).toBe('const a = 1;\nconst b = 2;'); + }); + + it('should not remove comments or empty lines when not configured', () => { + const content = '// comment\nconst a = 1;\n\n/* multi-line\ncomment */\nconst b = 2;'; + const filePath = 'test.js'; + const config: RepopackConfigMerged = { + output: { + removeComments: false, + removeEmptyLines: false, + }, + } as RepopackConfigMerged; + + const result = processContent(content, filePath, config); + + expect(result).toBe(content.trim()); + }); + + it('should handle files without a manipulator', () => { + const content = 'Some content'; + const filePath = 'unknown.ext'; + const config: RepopackConfigMerged = { + output: { + removeComments: true, + removeEmptyLines: true, + }, + } as RepopackConfigMerged; + + vi.mocked(getFileManipulator).mockReturnValue(null); + + const result = processContent(content, filePath, config); + + expect(result).toBe(content); + }); + }); +}); diff --git a/tests/core/file/fileSanitizer.test.ts b/tests/core/file/fileSanitizer.test.ts deleted file mode 100644 index 58e7f676..00000000 --- a/tests/core/file/fileSanitizer.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -import path from 'node:path'; -import * as fs from 'node:fs/promises'; -import { expect, test, vi, describe, beforeEach } from 'vitest'; -import { sanitizeFile, postprocessContent, sanitizeFiles } from '../../../src/core/file/fileSanitizer.js'; -import { createMockConfig } from '../../testing/testUtils.js'; - -vi.mock('fs/promises'); - -describe('fileSanitizer', () => { - beforeEach(() => { - vi.resetAllMocks(); - }); - - test('sanitizeFile should read and preprocess file content', async () => { - const mockContent = ' Some file content \n'; - vi.mocked(fs.readFile).mockResolvedValue(mockContent); - - // Suppress iconv-lite error - vi.spyOn(console, 'error').mockImplementation(vi.fn()); - - const mockConfig = createMockConfig(); - const result = await sanitizeFile('/path/to/file.txt', mockConfig); - - expect(fs.readFile).toHaveBeenCalledWith('/path/to/file.txt'); - expect(result).toBe('Some file content'); - }); - - test('preprocessContent should trim content', () => { - const content = ' Some content with whitespace \n'; - const config = createMockConfig({ - output: { - filePath: 'output.txt', - style: 'plain', - topFilesLength: 2, - showLineNumbers: false, - removeComments: false, - removeEmptyLines: false, - }, - }); - const result = postprocessContent(content, config); - expect(result).toBe('Some content with whitespace'); - }); - - test('preprocessContent should remove empty lines when configured', () => { - const content = ` - Some content - - with empty lines - - in between - `; - const config = createMockConfig({ - output: { - filePath: 'output.txt', - style: 'plain', - topFilesLength: 2, - showLineNumbers: false, - removeComments: false, - removeEmptyLines: true, - }, - }); - const result = postprocessContent(content, config); - - expect(result).toBe('Some content\n with empty lines\n in between'); - }); - - test('preprocessContent should not remove empty lines when not configured', () => { - const content = ` - Some content - - with empty lines - - in between - `; - const config = createMockConfig({ - output: { - filePath: 'output.txt', - style: 'plain', - topFilesLength: 2, - showLineNumbers: false, - removeComments: false, - removeEmptyLines: false, - }, - }); - const result = postprocessContent(content, config); - - expect(result).toBe('Some content\n\n with empty lines\n\n in between'); - }); - - test('sanitizeFiles should process multiple files', async () => { - const mockConfig = createMockConfig(); - const mockFilePaths = ['file1.txt', 'dir/file2.txt']; - const mockCwd = '/mock/root'; - - vi.mocked(fs.readFile).mockResolvedValueOnce('content1').mockResolvedValueOnce('content2'); - - const result = await sanitizeFiles(mockFilePaths, mockCwd, mockConfig); - - expect(result).toEqual([ - { path: 'file1.txt', content: 'content1' }, - { path: 'dir/file2.txt', content: 'content2' }, - ]); - - expect(fs.readFile).toHaveBeenCalledTimes(2); - expect(vi.mocked(fs.readFile).mock.calls[0][0]).toBe(path.join(mockCwd, 'file1.txt')); - expect(vi.mocked(fs.readFile).mock.calls[1][0]).toBe(path.join(mockCwd, 'dir/file2.txt')); - }); -}); diff --git a/tests/core/output/outputGenerator.test.ts b/tests/core/output/outputGenerator.test.ts index 9b02af9a..5c134c67 100644 --- a/tests/core/output/outputGenerator.test.ts +++ b/tests/core/output/outputGenerator.test.ts @@ -1,6 +1,7 @@ import { expect, test, describe } from 'vitest'; import { generateOutput } from '../../../src/core/output/outputGenerator.js'; import { createMockConfig } from '../../testing/testUtils.js'; +import { ProcessedFile } from '../../../src/core/file/fileTypes.js'; describe('outputGenerator', () => { test('generateOutput should write correct content to file', async () => { @@ -14,12 +15,12 @@ describe('outputGenerator', () => { removeEmptyLines: false, }, }); - const mockSanitizedFiles = [ + const mockProcessedFiles: ProcessedFile[] = [ { path: 'file1.txt', content: 'content1' }, { path: 'dir/file2.txt', content: 'content2' }, ]; - const output = await generateOutput(mockConfig, mockSanitizedFiles, []); + const output = await generateOutput(mockConfig, mockProcessedFiles, []); expect(output).toContain('Repopack Output File'); expect(output).toContain('File: file1.txt'); diff --git a/tests/core/packager.test.ts b/tests/core/packager.test.ts index a00c664b..89e3ff5e 100644 --- a/tests/core/packager.test.ts +++ b/tests/core/packager.test.ts @@ -3,8 +3,12 @@ import * as fs from 'node:fs/promises'; import { expect, test, vi, describe, beforeEach } from 'vitest'; import { pack, PackDependencies } from '../../src/core/packager.js'; import { createMockConfig } from '../testing/testUtils.js'; +import { TokenCounter } from '../../src/core/tokenCounter/tokenCounter.js'; +vi.mock('node:fs/promises'); vi.mock('fs/promises'); +vi.mock('../../src/core/security/securityCheckRunner'); +vi.mock('../../src/core/tokenCounter/tokenCounter'); describe('packager', () => { let mockDeps: PackDependencies; @@ -14,56 +18,44 @@ describe('packager', () => { const file2Path = path.join('dir1', 'file2.txt'); mockDeps = { searchFiles: vi.fn().mockResolvedValue(['file1.txt', file2Path]), - generateOutput: vi.fn().mockResolvedValue(undefined), - sanitizeFiles: vi.fn().mockResolvedValue([ + collectFiles: vi.fn().mockResolvedValue([ + { path: 'file1.txt', content: 'raw content 1' }, + { path: file2Path, content: 'raw content 2' }, + ]), + processFiles: vi.fn().mockReturnValue([ { path: 'file1.txt', content: 'processed content 1' }, { path: file2Path, content: 'processed content 2' }, ]), + runSecurityCheck: vi.fn().mockResolvedValue([]), + generateOutput: vi.fn().mockResolvedValue('mock output'), }; + + vi.mocked(TokenCounter.prototype.countTokens).mockReturnValue(10); }); test('pack should process files and generate output', async () => { const mockConfig = createMockConfig(); - const file2Path = path.join('dir1', 'file2.txt'); const result = await pack('root', mockConfig, mockDeps); expect(mockDeps.searchFiles).toHaveBeenCalledWith('root', mockConfig); + expect(mockDeps.collectFiles).toHaveBeenCalledWith(['file1.txt', 'dir1/file2.txt'], 'root'); + expect(mockDeps.runSecurityCheck).toHaveBeenCalled(); + expect(mockDeps.processFiles).toHaveBeenCalled(); + expect(mockDeps.generateOutput).toHaveBeenCalled(); + expect(fs.writeFile).toHaveBeenCalled(); - expect(mockDeps.sanitizeFiles).toHaveBeenCalledWith(['file1.txt', file2Path], 'root', mockConfig); - expect(mockDeps.generateOutput).toHaveBeenCalledWith( - mockConfig, - [ - { path: 'file1.txt', content: 'processed content 1' }, - { path: file2Path, content: 'processed content 2' }, - ], - ['file1.txt', file2Path], - ); - - // Check the result of pack function expect(result.totalFiles).toBe(2); - expect(result.totalCharacters).toBe(38); // 'processed content 1' + 'processed content 2' + expect(result.totalCharacters).toBe(38); + expect(result.totalTokens).toBe(20); expect(result.fileCharCounts).toEqual({ 'file1.txt': 19, - [file2Path]: 19, + 'dir1/file2.txt': 19, + }); + expect(result.fileTokenCounts).toEqual({ + 'file1.txt': 10, + 'dir1/file2.txt': 10, }); - }); - - test('pack should handle empty filtered files list', async () => { - const mockConfig = createMockConfig(); - vi.mocked(mockDeps.searchFiles).mockResolvedValue([]); - - vi.mocked(mockDeps.sanitizeFiles).mockResolvedValue([]); - - const result = await pack('root', mockConfig, mockDeps); - - expect(mockDeps.searchFiles).toHaveBeenCalledWith('root', mockConfig); - expect(mockDeps.sanitizeFiles).toHaveBeenCalledWith([], 'root', mockConfig); - expect(mockDeps.generateOutput).toHaveBeenCalledWith(mockConfig, [], []); - - expect(result.totalFiles).toBe(0); - expect(result.totalCharacters).toBe(0); - expect(result.fileCharCounts).toEqual({}); }); test('pack should handle security check and filter out suspicious files', async () => { @@ -71,23 +63,43 @@ describe('packager', () => { const suspiciousFile = 'suspicious.txt'; const file2Path = path.join('dir1', 'file2.txt'); vi.mocked(mockDeps.searchFiles).mockResolvedValue(['file1.txt', file2Path, suspiciousFile]); - - // Mock fs.readFile to return content for security check - vi.mocked(fs.readFile).mockImplementation((filepath) => { - if (filepath.toString().includes(suspiciousFile)) { - // secretlint-disable - return Promise.resolve('URL: https://user:pass@example.com'); - // secretlint-enable - } - return Promise.resolve('normal content'); - }); + vi.mocked(mockDeps.collectFiles).mockResolvedValue([ + { path: 'file1.txt', content: 'raw content 1' }, + { path: file2Path, content: 'raw content 2' }, + { path: suspiciousFile, content: 'suspicious content' }, + ]); + + // Mock the runSecurityCheck to return a suspicious file result + vi.mocked(mockDeps.runSecurityCheck).mockResolvedValue([ + { + filePath: path.join('root', suspiciousFile), + messages: ['Suspicious content detected'], + }, + ]); const result = await pack('root', mockConfig, mockDeps); expect(mockDeps.searchFiles).toHaveBeenCalledWith('root', mockConfig); - expect(mockDeps.sanitizeFiles).toHaveBeenCalledWith(['file1.txt', file2Path], 'root', mockConfig); + expect(mockDeps.processFiles).toHaveBeenCalledWith( + [ + expect.objectContaining({ + content: 'raw content 1', + path: 'file1.txt', + }), + expect.objectContaining({ + content: 'raw content 2', + path: file2Path, + }), + expect.objectContaining({ + content: 'suspicious content', + path: 'suspicious.txt', + }), + ], + mockConfig, + ); expect(result.suspiciousFilesResults).toHaveLength(1); expect(result.suspiciousFilesResults[0].filePath).toContain(suspiciousFile); + expect(result.totalFiles).toBe(2); // Only safe files should be counted }); }); From 3bc021f8139258dd362a7f2867e14d1bc9560312 Mon Sep 17 00:00:00 2001 From: Kazuki Yamada Date: Mon, 12 Aug 2024 18:34:47 +0900 Subject: [PATCH 3/4] refact: Rename command to action --- .../defaultActionRunner.ts} | 6 +++--- .../initActionRunner.ts} | 2 +- .../versionActionRunner.ts} | 2 +- src/cli/cliRunner.ts | 12 ++++++------ .../defaultActionRunner.test.ts} | 14 +++++++------- .../initActionRunner.test.ts} | 12 ++++++------ .../versionActionRunner.test.ts} | 6 +++--- 7 files changed, 27 insertions(+), 27 deletions(-) rename src/cli/{commands/defaultCommandRunner.ts => actions/defaultActionRunner.ts} (93%) rename src/cli/{commands/initCommandRunner.ts => actions/initActionRunner.ts} (97%) rename src/cli/{commands/versionCommandRunner.ts => actions/versionActionRunner.ts} (74%) rename tests/cli/{commands/defaultCommandRunner.test.ts => actions/defaultActionRunner.test.ts} (87%) rename tests/cli/{commands/initCommandRunner.test.ts => actions/initActionRunner.test.ts} (88%) rename tests/cli/{commands/versionCommandRunner.test.ts => actions/versionActionRunner.test.ts} (81%) diff --git a/src/cli/commands/defaultCommandRunner.ts b/src/cli/actions/defaultActionRunner.ts similarity index 93% rename from src/cli/commands/defaultCommandRunner.ts rename to src/cli/actions/defaultActionRunner.ts index 49282618..7f64e6ee 100644 --- a/src/cli/commands/defaultCommandRunner.ts +++ b/src/cli/actions/defaultActionRunner.ts @@ -11,10 +11,10 @@ import { loadFileConfig, mergeConfigs } from '../../config/configLoader.js'; import { logger } from '../../shared/logger.js'; import { CliOptions } from '../cliRunner.js'; import { getVersion } from '../../core/file/packageJsonParser.js'; -import Spinner from './../cliSpinner.js'; -import { printSummary, printTopFiles, printCompletion, printSecurityCheck } from './../cliPrinter.js'; +import Spinner from '../cliSpinner.js'; +import { printSummary, printTopFiles, printCompletion, printSecurityCheck } from '../cliPrinter.js'; -export const runDefaultCommand = async (directory: string, cwd: string, options: CliOptions): Promise => { +export const runDefaultAction = async (directory: string, cwd: string, options: CliOptions): Promise => { const version = await getVersion(); logger.log(pc.dim(`\nšŸ“¦ Repopack v${version}\n`)); diff --git a/src/cli/commands/initCommandRunner.ts b/src/cli/actions/initActionRunner.ts similarity index 97% rename from src/cli/commands/initCommandRunner.ts rename to src/cli/actions/initActionRunner.ts index d9a2effc..fbb74807 100644 --- a/src/cli/commands/initCommandRunner.ts +++ b/src/cli/actions/initActionRunner.ts @@ -6,7 +6,7 @@ import { logger } from '../../shared/logger.js'; import { RepopackConfigFile, RepopackOutputStyle } from '../../config/configTypes.js'; import { defaultConfig } from '../../config/defaultConfig.js'; -export const runInitCommand = async (rootDir: string): Promise => { +export const runInitAction = async (rootDir: string): Promise => { const configPath = path.join(rootDir, 'repopack.config.json'); try { diff --git a/src/cli/commands/versionCommandRunner.ts b/src/cli/actions/versionActionRunner.ts similarity index 74% rename from src/cli/commands/versionCommandRunner.ts rename to src/cli/actions/versionActionRunner.ts index 44b3be19..2311875e 100644 --- a/src/cli/commands/versionCommandRunner.ts +++ b/src/cli/actions/versionActionRunner.ts @@ -1,7 +1,7 @@ import { getVersion } from '../../core/file/packageJsonParser.js'; import { logger } from '../../shared/logger.js'; -export const runVersionCommand = async (): Promise => { +export const runVersionAction = async (): Promise => { const version = await getVersion(); logger.log(version); }; diff --git a/src/cli/cliRunner.ts b/src/cli/cliRunner.ts index 8f30a018..4debcf5f 100644 --- a/src/cli/cliRunner.ts +++ b/src/cli/cliRunner.ts @@ -3,9 +3,9 @@ import { program, OptionValues } from 'commander'; import { RepopackOutputStyle } from '../config/configTypes.js'; import { getVersion } from '../core/file/packageJsonParser.js'; import { handleError } from '../shared/errorHandler.js'; -import { runInitCommand } from './commands/initCommandRunner.js'; -import { runVersionCommand } from './commands/versionCommandRunner.js'; -import { runDefaultCommand } from './commands/defaultCommandRunner.js'; +import { runInitAction } from './actions/initActionRunner.js'; +import { runVersionAction } from './actions/versionActionRunner.js'; +import { runDefaultAction } from './actions/defaultActionRunner.js'; export interface CliOptions extends OptionValues { version?: boolean; @@ -48,14 +48,14 @@ export async function run() { const executeAction = async (directory: string, cwd: string, options: CliOptions) => { if (options.version) { - await runVersionCommand(); + await runVersionAction(); return; } if (options.init) { - await runInitCommand(cwd); + await runInitAction(cwd); return; } - await runDefaultCommand(directory, cwd, options); + await runDefaultAction(directory, cwd, options); }; diff --git a/tests/cli/commands/defaultCommandRunner.test.ts b/tests/cli/actions/defaultActionRunner.test.ts similarity index 87% rename from tests/cli/commands/defaultCommandRunner.test.ts rename to tests/cli/actions/defaultActionRunner.test.ts index 98235da1..1d4eb8a9 100644 --- a/tests/cli/commands/defaultCommandRunner.test.ts +++ b/tests/cli/actions/defaultActionRunner.test.ts @@ -1,5 +1,5 @@ import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; -import { runDefaultCommand } from '../../../src/cli/commands/defaultCommandRunner.js'; +import { runDefaultAction } from '../../../src/cli/actions/defaultActionRunner.js'; import * as packager from '../../../src/core/packager.js'; import * as configLoader from '../../../src/config/configLoader.js'; import * as packageJsonParser from '../../../src/core/file/packageJsonParser.js'; @@ -11,7 +11,7 @@ vi.mock('../../../src/config/configLoader'); vi.mock('../../../src/core/file/packageJsonParser'); vi.mock('../../../src/shared/logger'); -describe('defaultCommandRunner', () => { +describe('defaultActionRunner', () => { beforeEach(() => { vi.resetAllMocks(); vi.mocked(packageJsonParser.getVersion).mockResolvedValue('1.0.0'); @@ -52,7 +52,7 @@ describe('defaultCommandRunner', () => { verbose: true, }; - await runDefaultCommand('.', process.cwd(), options); + await runDefaultAction('.', process.cwd(), options); expect(packageJsonParser.getVersion).toHaveBeenCalled(); expect(logger.logger.setVerbose).toHaveBeenCalledWith(true); @@ -66,7 +66,7 @@ describe('defaultCommandRunner', () => { include: '*.js,*.ts', }; - await runDefaultCommand('.', process.cwd(), options); + await runDefaultAction('.', process.cwd(), options); expect(configLoader.mergeConfigs).toHaveBeenCalledWith( expect.anything(), @@ -81,7 +81,7 @@ describe('defaultCommandRunner', () => { ignore: 'node_modules,*.log', }; - await runDefaultCommand('.', process.cwd(), options); + await runDefaultAction('.', process.cwd(), options); expect(configLoader.mergeConfigs).toHaveBeenCalledWith( expect.anything(), @@ -98,7 +98,7 @@ describe('defaultCommandRunner', () => { style: 'xml', }; - await runDefaultCommand('.', process.cwd(), options); + await runDefaultAction('.', process.cwd(), options); expect(configLoader.mergeConfigs).toHaveBeenCalledWith( expect.anything(), @@ -115,6 +115,6 @@ describe('defaultCommandRunner', () => { const options: CliOptions = {}; - await expect(runDefaultCommand('.', process.cwd(), options)).rejects.toThrow('Test error'); + await expect(runDefaultAction('.', process.cwd(), options)).rejects.toThrow('Test error'); }); }); diff --git a/tests/cli/commands/initCommandRunner.test.ts b/tests/cli/actions/initActionRunner.test.ts similarity index 88% rename from tests/cli/commands/initCommandRunner.test.ts rename to tests/cli/actions/initActionRunner.test.ts index 0dc4e4e3..5fb06847 100644 --- a/tests/cli/commands/initCommandRunner.test.ts +++ b/tests/cli/actions/initActionRunner.test.ts @@ -1,13 +1,13 @@ import * as fs from 'node:fs/promises'; import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; import * as prompts from '@clack/prompts'; -import { runInitCommand } from '../../../src/cli/commands/initCommandRunner.js'; +import { runInitAction } from '../../../src/cli/actions/initActionRunner.js'; import { logger } from '../../../src/shared/logger.js'; vi.mock('node:fs/promises'); vi.mock('@clack/prompts'); -describe('initCommandRunner', () => { +describe('initActionRunner', () => { beforeEach(() => { vi.resetAllMocks(); }); @@ -23,7 +23,7 @@ describe('initCommandRunner', () => { outputStyle: 'xml', }); - await runInitCommand('/test/dir'); + await runInitAction('/test/dir'); expect(fs.writeFile).toHaveBeenCalledWith( '/test/dir/repopack.config.json', @@ -39,7 +39,7 @@ describe('initCommandRunner', () => { vi.mocked(fs.access).mockResolvedValue(undefined); const loggerSpy = vi.spyOn(logger, 'warn').mockImplementation(vi.fn()); - await runInitCommand('/test/dir'); + await runInitAction('/test/dir'); expect(fs.writeFile).not.toHaveBeenCalled(); expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('already exists')); @@ -53,7 +53,7 @@ describe('initCommandRunner', () => { const loggerSpy = vi.spyOn(logger, 'error').mockImplementation(vi.fn()); - await runInitCommand('/test/dir'); + await runInitAction('/test/dir'); expect(fs.writeFile).not.toHaveBeenCalled(); @@ -68,6 +68,6 @@ describe('initCommandRunner', () => { }); vi.mocked(fs.writeFile).mockRejectedValue(new Error('Write error')); - await runInitCommand('/test/dir'); + await runInitAction('/test/dir'); }); }); diff --git a/tests/cli/commands/versionCommandRunner.test.ts b/tests/cli/actions/versionActionRunner.test.ts similarity index 81% rename from tests/cli/commands/versionCommandRunner.test.ts rename to tests/cli/actions/versionActionRunner.test.ts index a748e35f..615a0cb8 100644 --- a/tests/cli/commands/versionCommandRunner.test.ts +++ b/tests/cli/actions/versionActionRunner.test.ts @@ -1,11 +1,11 @@ import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; -import { runVersionCommand } from '../../../src/cli/commands/versionCommandRunner.js'; +import { runVersionAction } from '../../../src/cli/actions/versionActionRunner.js'; import * as packageJsonParser from '../../../src/core/file/packageJsonParser.js'; import { logger } from '../../../src/shared/logger.js'; vi.mock('../../../src/core/file/packageJsonParser'); -describe('versionCommandRunner', () => { +describe('versionActionRunner', () => { beforeEach(() => { vi.resetAllMocks(); }); @@ -18,7 +18,7 @@ describe('versionCommandRunner', () => { vi.mocked(packageJsonParser.getVersion).mockResolvedValue('1.2.3'); const loggerSpy = vi.spyOn(logger, 'log').mockImplementation(vi.fn()); - await runVersionCommand(); + await runVersionAction(); expect(packageJsonParser.getVersion).toHaveBeenCalled(); expect(loggerSpy).toHaveBeenCalledWith('1.2.3'); From 4502083dfdf3a0b3a888f7f54a23c0801b091785 Mon Sep 17 00:00:00 2001 From: Kazuki Yamada Date: Mon, 12 Aug 2024 18:38:32 +0900 Subject: [PATCH 4/4] fix(test): Fix tests for windows --- src/cli/actions/initActionRunner.ts | 2 +- src/core/file/fileCollector.ts | 2 +- tests/cli/actions/initActionRunner.test.ts | 13 +++++-------- tests/core/file/fileCollector.test.ts | 8 ++++++-- tests/core/packager.test.ts | 8 +++++--- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/cli/actions/initActionRunner.ts b/src/cli/actions/initActionRunner.ts index fbb74807..ee98551b 100644 --- a/src/cli/actions/initActionRunner.ts +++ b/src/cli/actions/initActionRunner.ts @@ -7,7 +7,7 @@ import { RepopackConfigFile, RepopackOutputStyle } from '../../config/configType import { defaultConfig } from '../../config/defaultConfig.js'; export const runInitAction = async (rootDir: string): Promise => { - const configPath = path.join(rootDir, 'repopack.config.json'); + const configPath = path.resolve(rootDir, 'repopack.config.json'); try { // Check if the config file already exists diff --git a/src/core/file/fileCollector.ts b/src/core/file/fileCollector.ts index 9736b31e..072e7c48 100644 --- a/src/core/file/fileCollector.ts +++ b/src/core/file/fileCollector.ts @@ -10,7 +10,7 @@ export const collectFiles = async (filePaths: string[], rootDir: string): Promis const rawFiles: RawFile[] = []; for (const filePath of filePaths) { - const fullPath = path.join(rootDir, filePath); + const fullPath = path.resolve(rootDir, filePath); const content = await readRawFile(fullPath); if (content) { rawFiles.push({ path: filePath, content }); diff --git a/tests/cli/actions/initActionRunner.test.ts b/tests/cli/actions/initActionRunner.test.ts index 5fb06847..718e030b 100644 --- a/tests/cli/actions/initActionRunner.test.ts +++ b/tests/cli/actions/initActionRunner.test.ts @@ -1,4 +1,5 @@ import * as fs from 'node:fs/promises'; +import path from 'node:path'; import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; import * as prompts from '@clack/prompts'; import { runInitAction } from '../../../src/cli/actions/initActionRunner.js'; @@ -25,14 +26,10 @@ describe('initActionRunner', () => { await runInitAction('/test/dir'); - expect(fs.writeFile).toHaveBeenCalledWith( - '/test/dir/repopack.config.json', - expect.stringContaining('"filePath": "custom-output.txt"'), - ); - expect(fs.writeFile).toHaveBeenCalledWith( - '/test/dir/repopack.config.json', - expect.stringContaining('"style": "xml"'), - ); + const configPath = path.resolve('/test/dir/repopack.config.json'); + + expect(fs.writeFile).toHaveBeenCalledWith(configPath, expect.stringContaining('"filePath": "custom-output.txt"')); + expect(fs.writeFile).toHaveBeenCalledWith(configPath, expect.stringContaining('"style": "xml"')); }); it('should not create a new config file when one already exists', async () => { diff --git a/tests/core/file/fileCollector.test.ts b/tests/core/file/fileCollector.test.ts index e93ffdcc..27d68d61 100644 --- a/tests/core/file/fileCollector.test.ts +++ b/tests/core/file/fileCollector.test.ts @@ -1,4 +1,5 @@ import * as fs from 'node:fs/promises'; +import path from 'node:path'; import { expect, describe, it, vi, beforeEach, afterEach } from 'vitest'; import { isBinary } from 'istextorbinary'; import jschardet from 'jschardet'; @@ -50,7 +51,7 @@ describe('fileCollector', () => { const result = await collectFiles(mockFilePaths, mockRootDir); expect(result).toEqual([{ path: 'text.txt', content: 'decoded content' }]); - expect(logger.debug).toHaveBeenCalledWith('Skipping binary file: /root/binary.bin'); + expect(logger.debug).toHaveBeenCalledWith('Skipping binary file: ' + path.resolve('/root/binary.bin')); }); it('should handle file read errors', async () => { @@ -63,6 +64,9 @@ describe('fileCollector', () => { const result = await collectFiles(mockFilePaths, mockRootDir); expect(result).toEqual([]); - expect(logger.warn).toHaveBeenCalledWith('Failed to read file: /root/error.txt', expect.any(Error)); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to read file: ' + path.resolve('/root/error.txt'), + expect.any(Error), + ); }); }); diff --git a/tests/core/packager.test.ts b/tests/core/packager.test.ts index 89e3ff5e..2d65dc17 100644 --- a/tests/core/packager.test.ts +++ b/tests/core/packager.test.ts @@ -38,8 +38,10 @@ describe('packager', () => { const result = await pack('root', mockConfig, mockDeps); + const file2Path = path.join('dir1', 'file2.txt'); + expect(mockDeps.searchFiles).toHaveBeenCalledWith('root', mockConfig); - expect(mockDeps.collectFiles).toHaveBeenCalledWith(['file1.txt', 'dir1/file2.txt'], 'root'); + expect(mockDeps.collectFiles).toHaveBeenCalledWith(['file1.txt', file2Path], 'root'); expect(mockDeps.runSecurityCheck).toHaveBeenCalled(); expect(mockDeps.processFiles).toHaveBeenCalled(); expect(mockDeps.generateOutput).toHaveBeenCalled(); @@ -50,11 +52,11 @@ describe('packager', () => { expect(result.totalTokens).toBe(20); expect(result.fileCharCounts).toEqual({ 'file1.txt': 19, - 'dir1/file2.txt': 19, + [file2Path]: 19, }); expect(result.fileTokenCounts).toEqual({ 'file1.txt': 10, - 'dir1/file2.txt': 10, + [file2Path]: 10, }); });