Skip to content

Commit 0fb52d1

Browse files
committed
Refactor tests and binary/worker path logic
Add centralized binary/worker path resolution and strengthen tests. Documentation (CLAUDE.md) now references findBinary() and runtime detection helpers; FLAC default compression and test counts updated. Many tests were modernized: typed mocks, more reliable path/resolve behavior, explicit findBinary stubs, improved worker mocking (workerMock vs Worker), typed timeout helper, and added self-overwrite unit tests. converterWorker expectations changed to report errors (instead of success codes) for directory failures. finalize() tests updated to return booleans and properly exercise rl behavior. Misc: new selfOverwrite.test.ts, deleted legacy config files, and added helper commands to .claude/settings.local.json for targeted test runs. These changes improve cross-platform path handling, test stability, and error reporting.
1 parent 38706b7 commit 0fb52d1

29 files changed

+1141
-919
lines changed

.claude/settings.local.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,12 @@
1717
"Bash(npm test:*)",
1818
"Bash(xargs:*)",
1919
"Bash(grep:*)",
20-
"Bash(npx jest:*)"
20+
"Bash(npx jest:*)",
21+
"Bash(node --experimental-vm-modules node_modules/jest/bin/jest.js src/__tests__/finalize.test.ts --runInBand 2>&1 | tail -30)",
22+
"Bash(node --experimental-vm-modules node_modules/jest/bin/jest.js src/__tests__/selfOverwrite.test.ts --runInBand --verbose 2>&1 | grep -E '\\(✓|✕|PASS|FAIL|Tests:|✓|×|√|●|RUNS|✗|passed|failed| \\)')",
23+
"Bash(node --experimental-vm-modules node_modules/jest/bin/jest.js src/__tests__/selfOverwrite.test.ts --runInBand --verbose 2>&1 | grep -E '\\(PASS|FAIL|Tests:|✓|✕|✗|×|√\\)' | tail -5)",
24+
"Bash(node --experimental-vm-modules node_modules/jest/bin/jest.js \"src/__tests__/\" --runInBand 2>&1 | grep -E '\\(FAIL |● \\)' | head -20)",
25+
"Bash(node --experimental-vm-modules node_modules/jest/bin/jest.js src/__tests__/metadataFormatting.integration.test.ts src/__tests__/converterWorker.paths.test.ts --runInBand 2>&1 | grep -B1 -A5 '●.*should escape\\\\|●.*uses runtime\\\\|●.*throws' | head -40)"
2126
],
2227
"deny": [],
2328
"ask": []

CLAUDE.md

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,13 @@ The conversion process (`src/converterManager.ts`) uses Node.js worker threads:
9494
- Each worker spawns ffmpeg processes for individual conversions
9595
- Parent thread collects results and aggregates success/failure counts
9696

97-
**Critical Detail**: Worker uses ESM import.meta.url for path resolution:
97+
**Critical Detail**: Binary and worker path resolution is centralised in `src/utils.ts` via `findBinary()`. The ESM/CJS `__dirname` detection is computed once there, and all modules call:
9898

9999
```typescript
100-
import { fileURLToPath } from 'url';
101-
import { dirname, join } from 'path';
100+
import { findBinary, isPackagedRuntime } from './utils.js';
102101

103-
const __filename = fileURLToPath(import.meta.url);
104-
const __dirname = dirname(__filename);
105-
const workerPath = join(__dirname, 'converterWorker.js');
102+
const workerExt = isPackagedRuntime ? 'cjs' : 'js';
103+
const workerPath = findBinary(`converterWorker.${workerExt}`, ['dist']);
106104
```
107105

108106
### Key Architectural Components
@@ -111,14 +109,17 @@ const workerPath = join(__dirname, 'converterWorker.js');
111109
2. **getUserInput.ts** - Interactive CLI prompts for input/output paths and formats
112110
3. **searchFiles.ts** - Recursive file system search for audio files
113111
4. **createConversionList.ts** - Builds conversion jobs with conflict resolution (overwrite/rename/skip)
114-
5. **converterManager.ts** - Spawns and manages worker thread pool
112+
5. **converterManager.ts** - Spawns and manages worker thread pool, fatal ffmpeg error detection (disk space, etc.)
115113
6. **converterWorker.ts** - Worker thread that executes ffmpeg commands and extracts metadata
116114
7. **metadataService.ts** - Handles metadata extraction, loop point conversion, and formatting
117-
8. **utils.ts** - CSV logging, disk space checks, readline interface
118-
9. **finalize.ts** - Results display and auto-restart functionality
115+
8. **exitProgramError.ts** - Custom error class for silent program exit (caught by app.ts top-level handler)
116+
9. **utils.ts** - Runtime detection (`isPackagedRuntime`, `isSeaRuntime`, `platformSlug`, `runtimeBaseDir`), `findBinary()` for locating bundled executables/workers, `getErrorMessage()`, CSV logging, file-busy checks, readline interface
117+
10. **finalize.ts** - Results display, CSV summary output, and auto-restart functionality
119118

120119
### Global Environment Configuration
121120

121+
Type definitions are in `src/types/`: `global.ts` (runtime env), `settings.ts` (Settings object), `audio.ts` (audio types), `metadata.ts` (AudioMetadata).
122+
122123
The application uses `globalThis.env` for runtime configuration (type defined in `src/types/global.ts`):
123124

124125
- Platform detection (Windows/Mac/Linux)
@@ -234,7 +235,7 @@ Tests are located in `src/__tests__/`:
234235

235236
All tests run sequentially (`--runInBand`) to prevent file system race conditions.
236237

237-
**Test count:** 400+ tests across 25 test files.
238+
**Test count:** 440+ tests across 26 test files.
238239

239240
## TypeScript Configuration
240241

@@ -258,7 +259,7 @@ Codecs are hardcoded in `src/converterWorker.ts`:
258259
- **OGG**: libopus at 64k bitrate, or libvorbis with VBR quality 1.2
259260
- **M4A**: AAC at 256k fixed bitrate
260261
- **WAV/AIFF**: pcm_s16le (16-bit uncompressed)
261-
- **FLAC**: Compression level 9 (minimal compression)
262+
- **FLAC**: Compression level 1 (minimal compression)
262263

263264
To modify bitrates or codecs, edit the `formatConfig` object and `getFormatConfig()` function in `src/converterWorker.ts`.
264265

@@ -289,7 +290,7 @@ The project uses GitHub Actions to automatically build releases for all platform
289290
Runs on every push/PR to `dev` or `main`:
290291
- Linting and formatting
291292
- TypeScript compilation
292-
- Tests with coverage (400+ tests)
293+
- Tests with coverage (440+ tests)
293294
- Build verification
294295

295296
### Creating a Release

src/__tests__/app.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,8 @@ describe('app.js', () => {
8585
successfulFiles: ['file2'],
8686
jobStartTime: 0,
8787
});
88-
(finalize as unknown as Mock<() => Promise<void>>).mockResolvedValue(
89-
undefined
88+
(finalize as unknown as Mock<() => Promise<boolean>>).mockResolvedValue(
89+
false
9090
);
9191

9292
logSpy = jest.spyOn(console, 'log').mockImplementation(() => {});

src/__tests__/convertFiles.test.ts

Lines changed: 60 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
beforeEach,
88
afterEach,
99
} from '@jest/globals';
10+
import type { ConversionItem } from '../types/audio.js';
1011

1112
// ESM mocks must be declared BEFORE dynamic imports
1213
jest.unstable_mockModule('perf_hooks', () => ({
@@ -37,10 +38,13 @@ jest.unstable_mockModule('../utils.js', () => ({
3738
addToLog: jest.fn(),
3839
settings: { oggCodec: 'vorbis' },
3940
initializeFileNames: jest.fn(),
40-
rl: { question: jest.fn((question, callback) => callback()) },
41+
rl: {
42+
question: jest.fn((_question: string, callback: () => void) => callback()),
43+
},
4144
getAnswer: jest.fn(),
4245
runtimeBaseDir: '/mock/base',
4346
isPackagedRuntime: false,
47+
findBinary: () => '/mock/base/converterWorker.js',
4448
}));
4549

4650
jest.unstable_mockModule('os', () => ({
@@ -53,13 +57,26 @@ const os = await import('os');
5357
const { convertFiles } = await import('../converterManager.js');
5458
import events from 'events';
5559

60+
type WorkerHandlers = {
61+
message?: (message: unknown) => void;
62+
error?: (error: unknown) => void;
63+
exit?: (code: number) => void;
64+
};
65+
66+
const workerMock = Worker as unknown as jest.MockedFunction<any>;
67+
const cpusMock = os.cpus as unknown as jest.MockedFunction<typeof os.cpus>;
68+
5669
events.defaultMaxListeners = 20;
5770

5871
// Add a timeout helper function
59-
const withTimeout = (promise, timeoutMs, errorMessage) => {
60-
let timeoutId;
61-
62-
const timeoutPromise = new Promise((_, reject) => {
72+
const withTimeout = <T>(
73+
promise: Promise<T>,
74+
timeoutMs: number,
75+
errorMessage?: string
76+
): Promise<T> => {
77+
let timeoutId: ReturnType<typeof setTimeout> | undefined;
78+
79+
const timeoutPromise: Promise<never> = new Promise((_, reject) => {
6380
timeoutId = setTimeout(() => {
6481
console.error(
6582
`TIMEOUT: ${errorMessage || 'Test took too long to complete'}`
@@ -73,7 +90,9 @@ const withTimeout = (promise, timeoutMs, errorMessage) => {
7390
});
7491

7592
return Promise.race([promise, timeoutPromise]).finally(() => {
76-
clearTimeout(timeoutId);
93+
if (timeoutId) {
94+
clearTimeout(timeoutId);
95+
}
7796
});
7897
};
7998

@@ -104,8 +123,8 @@ describe('convertFiles', () => {
104123
});
105124

106125
// Create a factory function for test files to make tests more maintainable
107-
const createTestFiles = (count = 2) => {
108-
const files = [];
126+
const createTestFiles = (count = 2): ConversionItem[] => {
127+
const files: ConversionItem[] = [];
109128
for (let i = 0; i < count; i++) {
110129
files.push({
111130
inputFile: `C:/Music/testfile${i}.mp3`,
@@ -125,12 +144,15 @@ describe('convertFiles', () => {
125144
delayCompletion = false,
126145
} = {}) {
127146
// Mock events object
128-
const handlers = {};
147+
const handlers: WorkerHandlers = {};
129148

130149
// Create a worker with configurable behavior
131150
const worker = {
132-
on: jest.fn((event, handler) => {
133-
handlers[event] = handler;
151+
on: jest.fn((event: string, handler: (...args: unknown[]) => void) => {
152+
if (event === 'message') handlers.message = handler;
153+
if (event === 'error') handlers.error = handler;
154+
if (event === 'exit')
155+
handlers.exit = handler as unknown as (code: number) => void;
134156

135157
// Don't auto-trigger events if we want to delay completion
136158
if (delayCompletion) {
@@ -169,7 +191,7 @@ describe('convertFiles', () => {
169191
it('should process files successfully', async () => {
170192
// Create a simple mock worker that auto-completes successfully
171193
console.log("Starting 'should process files successfully' test");
172-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
194+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
173195

174196
const result = await withTimeout(
175197
convertFiles(createTestFiles(2)),
@@ -178,15 +200,15 @@ describe('convertFiles', () => {
178200
);
179201
console.log("Test 'should process files successfully' completed");
180202

181-
expect(Worker).toHaveBeenCalled();
203+
expect(workerMock).toHaveBeenCalled();
182204
expect(result.failedFiles).toHaveLength(0);
183205
expect(result.successfulFiles.length).toBeGreaterThan(0);
184206
}, 30000);
185207

186208
it('should handle failed conversions (non-zero exit code)', async () => {
187209
// Create a worker that exits with non-zero code
188210
console.log("Starting 'should handle failed conversions' test");
189-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 1 }));
211+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 1 }));
190212

191213
const result = await withTimeout(
192214
convertFiles(createTestFiles(1)),
@@ -195,7 +217,7 @@ describe('convertFiles', () => {
195217
);
196218
console.log("Test 'should handle failed conversions' completed");
197219

198-
expect(Worker).toHaveBeenCalled();
220+
expect(workerMock).toHaveBeenCalled();
199221
expect(result.successfulFiles).toHaveLength(0);
200222
expect(result.failedFiles.length).toBeGreaterThan(0);
201223
expect(console.error).toHaveBeenCalled();
@@ -205,12 +227,15 @@ describe('convertFiles', () => {
205227
// Mock a worker that triggers an error
206228
console.log("Starting 'should handle worker errors' test");
207229

208-
Worker.mockImplementation(() => {
230+
workerMock.mockImplementation(() => {
209231
// Synchronously fire the error event when registered
210-
const handlers = {};
232+
const handlers: WorkerHandlers = {};
211233
const worker = {
212-
on: jest.fn((event, handler) => {
213-
handlers[event] = handler;
234+
on: jest.fn((event: string, handler: (...args: unknown[]) => void) => {
235+
if (event === 'message') handlers.message = handler;
236+
if (event === 'error') handlers.error = handler;
237+
if (event === 'exit')
238+
handlers.exit = handler as unknown as (code: number) => void;
214239
if (event === 'error') {
215240
handler(new Error('Worker thread error'));
216241
}
@@ -238,7 +263,7 @@ describe('convertFiles', () => {
238263
// Mock a worker that sends stderr messages
239264
console.log("Starting 'should handle stderr messages from workers' test");
240265

241-
Worker.mockImplementation(() =>
266+
workerMock.mockImplementation(() =>
242267
createWorkerMock({
243268
triggerStderr: true,
244269
})
@@ -252,7 +277,7 @@ describe('convertFiles', () => {
252277
console.log("Test 'should handle stderr messages from workers' completed");
253278

254279
expect(console.error).toHaveBeenCalledWith(
255-
expect.stringContaining('ERROR MESSAGE FROM FFMPEG'),
280+
expect.stringContaining('FFmpeg stderr'),
256281
expect.any(String),
257282
expect.any(String),
258283
expect.any(String)
@@ -263,7 +288,7 @@ describe('convertFiles', () => {
263288
// Mock a worker that sends a 'no space left' error
264289
console.log("Starting 'should detect disk space errors' test");
265290

266-
Worker.mockImplementation(() =>
291+
workerMock.mockImplementation(() =>
267292
createWorkerMock({
268293
triggerStderr: true,
269294
noSpaceLeft: true,
@@ -272,7 +297,7 @@ describe('convertFiles', () => {
272297

273298
// Mock process.exit
274299
const originalExit = process.exit;
275-
process.exit = jest.fn();
300+
process.exit = jest.fn() as unknown as typeof process.exit;
276301

277302
await withTimeout(
278303
convertFiles(createTestFiles(1)),
@@ -296,7 +321,7 @@ describe('convertFiles', () => {
296321
);
297322

298323
let callCount = 0;
299-
Worker.mockImplementation(() => {
324+
workerMock.mockImplementation(() => {
300325
callCount++;
301326
return createWorkerMock({
302327
exitCode: callCount % 2 === 0 ? 0 : 1,
@@ -312,7 +337,7 @@ describe('convertFiles', () => {
312337
"Test 'should handle multiple workers and files properly' completed"
313338
);
314339

315-
expect(Worker).toHaveBeenCalledTimes(4);
340+
expect(workerMock).toHaveBeenCalledTimes(4);
316341
expect(result.successfulFiles.length).toBeGreaterThan(0);
317342
expect(result.failedFiles.length).toBeGreaterThan(0);
318343
}, 30000);
@@ -322,11 +347,11 @@ describe('convertFiles', () => {
322347
// Mock os.cpus to throw an error
323348
console.log("Starting 'should handle CPU detection failure' test");
324349

325-
os.cpus.mockImplementationOnce(() => {
350+
cpusMock.mockImplementationOnce(() => {
326351
throw new Error('CPU detection failed');
327352
});
328353

329-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
354+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
330355

331356
const result = await withTimeout(
332357
convertFiles(createTestFiles(2)),
@@ -346,8 +371,8 @@ describe('convertFiles', () => {
346371
// Set up 10 CPUs but only 2 files
347372
console.log("Starting 'should limit concurrent workers' test");
348373

349-
os.cpus.mockReturnValueOnce(Array(10).fill({}));
350-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
374+
cpusMock.mockReturnValueOnce(Array(10).fill({}));
375+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
351376

352377
await withTimeout(
353378
convertFiles(createTestFiles(2)),
@@ -356,7 +381,7 @@ describe('convertFiles', () => {
356381
);
357382
console.log("Test 'should limit concurrent workers' completed");
358383

359-
expect(Worker).toHaveBeenCalledTimes(2); // Should create only 2 workers
384+
expect(workerMock).toHaveBeenCalledTimes(2); // Should create only 2 workers
360385
}, 30000);
361386

362387
it('should properly handle worker creation errors', async () => {
@@ -365,7 +390,7 @@ describe('convertFiles', () => {
365390
"Starting 'should properly handle worker creation errors' test"
366391
);
367392

368-
Worker.mockImplementationOnce(() => {
393+
workerMock.mockImplementationOnce(() => {
369394
throw new Error('Failed to create worker');
370395
});
371396

@@ -386,7 +411,7 @@ describe('convertFiles', () => {
386411
}, 30000);
387412

388413
it('should handle an empty file list', async () => {
389-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
414+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
390415
const result = await withTimeout(
391416
convertFiles([]),
392417
5000,
@@ -397,10 +422,10 @@ describe('convertFiles', () => {
397422
});
398423

399424
it('should handle file with missing properties gracefully', async () => {
400-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
425+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 0 }));
401426
const files = [
402427
{ inputFile: undefined, outputFile: undefined, outputFormat: undefined },
403-
];
428+
] as unknown as ConversionItem[];
404429
const result = await withTimeout(
405430
convertFiles(files),
406431
5000,
@@ -410,7 +435,7 @@ describe('convertFiles', () => {
410435
});
411436

412437
it('should put all files in failedFiles if all workers fail', async () => {
413-
Worker.mockImplementation(() => createWorkerMock({ exitCode: 1 }));
438+
workerMock.mockImplementation(() => createWorkerMock({ exitCode: 1 }));
414439
const files = createTestFiles(3);
415440
const result = await withTimeout(
416441
convertFiles(files),

0 commit comments

Comments
 (0)