refactor(debug): replace ConsolePatcher with debugLogger and update error reporting

- Replace ConsolePatcher with centralized debugLogger utility
- Refactor errorReporting to use debugLogger instead of file-based reporting
- Remove user-facing console message components:
  - Delete ConsolePatcher.ts, useConsoleMessages.ts/hook
  - Delete ConsoleSummaryDisplay.tsx, DetailedMessagesDisplay.tsx
- Update all tests in packages/core and packages/cli:
  - Mock debugLogger where needed
  - Remove assertions for console output on non-critical errors
  - Keep debugLogger assertions for fatal/network errors
  - Use HOME directory mocking for hermetic file system tests

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This commit is contained in:
tanzhenxin 2026-02-02 17:37:54 +08:00
parent 135df54f27
commit 89e3c2cd7a
64 changed files with 1240 additions and 2416 deletions

View file

@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import * as fs from 'node:fs';
import * as path from 'node:path';
import * as os from 'node:os';
@ -158,28 +158,14 @@ describe('commentJson', () => {
fs.writeFileSync(testFilePath, corruptedContent, 'utf-8');
const consoleSpy = vi
.spyOn(console, 'error')
.mockImplementation(() => {});
expect(() => {
updateSettingsFilePreservingFormat(testFilePath, {
model: 'gemini-2.5-flash',
});
}).not.toThrow();
expect(consoleSpy).toHaveBeenCalledWith(
'Error parsing settings file:',
expect.any(Error),
);
expect(consoleSpy).toHaveBeenCalledWith(
'Settings file may be corrupted. Please check the JSON syntax.',
);
const unchangedContent = fs.readFileSync(testFilePath, 'utf-8');
expect(unchangedContent).toBe(corruptedContent);
consoleSpy.mockRestore();
});
});
});

View file

@ -19,6 +19,14 @@ import {
handleMaxTurnsExceededError,
} from './errors.js';
const mockWriteStderrLine = vi.hoisted(() => vi.fn());
const debugLoggerSpy = vi.hoisted(() => ({
debug: vi.fn(),
info: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
}));
// Mock the core modules
vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const original =
@ -26,6 +34,12 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
return {
...original,
createDebugLogger: () => ({
debug: debugLoggerSpy.debug,
info: debugLoggerSpy.info,
warn: debugLoggerSpy.warn,
error: debugLoggerSpy.error,
}),
parseAndFormatApiError: vi.fn((error: unknown) => {
if (error instanceof Error) {
return `API Error: ${error.message}`;
@ -66,18 +80,25 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
};
});
vi.mock('./stdioHelpers.js', () => ({
writeStderrLine: mockWriteStderrLine,
writeStdoutLine: vi.fn(),
clearScreen: vi.fn(),
}));
describe('errors', () => {
let mockConfig: Config;
let processExitSpy: MockInstance;
let processStderrWriteSpy: MockInstance;
let consoleErrorSpy: MockInstance;
beforeEach(() => {
// Reset mocks
vi.clearAllMocks();
// Mock console.error
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
mockWriteStderrLine.mockClear();
debugLoggerSpy.debug.mockClear();
debugLoggerSpy.info.mockClear();
debugLoggerSpy.warn.mockClear();
debugLoggerSpy.error.mockClear();
// Mock process.stderr.write
processStderrWriteSpy = vi
@ -99,7 +120,6 @@ describe('errors', () => {
});
afterEach(() => {
consoleErrorSpy.mockRestore();
processStderrWriteSpy.mockRestore();
processExitSpy.mockRestore();
});
@ -163,7 +183,9 @@ describe('errors', () => {
handleError(testError, mockConfig);
}).toThrow(testError);
expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: Test error');
expect(mockWriteStderrLine).toHaveBeenCalledWith(
'API Error: Test error',
);
});
it('should handle non-Error objects', () => {
@ -173,7 +195,9 @@ describe('errors', () => {
handleError(testError, mockConfig);
}).toThrow(testError);
expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: String error');
expect(mockWriteStderrLine).toHaveBeenCalledWith(
'API Error: String error',
);
});
});
@ -191,7 +215,7 @@ describe('errors', () => {
handleError(testError, mockConfig);
}).toThrow('process.exit called with code: 1');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {
@ -213,7 +237,7 @@ describe('errors', () => {
handleError(testError, mockConfig, 42);
}).toThrow('process.exit called with code: 42');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {
@ -235,7 +259,7 @@ describe('errors', () => {
handleError(fatalError, mockConfig);
}).toThrow('process.exit called with code: 42');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {
@ -271,7 +295,7 @@ describe('errors', () => {
handleError(errorWithStatus, mockConfig);
}).toThrow('process.exit called with code: 1'); // string codes become 1
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {
@ -307,7 +331,7 @@ describe('errors', () => {
it('should log error message to stderr and not exit', () => {
handleToolError(toolName, toolError, mockConfig);
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -322,7 +346,7 @@ describe('errors', () => {
'Custom display message',
);
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Custom display message',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -340,7 +364,7 @@ describe('errors', () => {
handleToolError(toolName, toolError, mockConfig);
// In JSON mode, should not exit (just log to stderr when debug mode is on)
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -350,7 +374,7 @@ describe('errors', () => {
handleToolError(toolName, toolError, mockConfig, 'CUSTOM_TOOL_ERROR');
// In JSON mode, should not exit (just log to stderr when debug mode is on)
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -360,7 +384,7 @@ describe('errors', () => {
handleToolError(toolName, toolError, mockConfig, 500);
// In JSON mode, should not exit (just log to stderr when debug mode is on)
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -376,7 +400,7 @@ describe('errors', () => {
);
// In JSON mode, should not exit (just log to stderr when debug mode is on)
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Display message',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -394,7 +418,7 @@ describe('errors', () => {
handleToolError(toolName, toolError, mockConfig);
// Should not exit in STREAM_JSON mode (just log to stderr when debug mode is on)
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
@ -407,36 +431,42 @@ describe('errors', () => {
(mockConfig.getDebugMode as Mock).mockReturnValue(false);
});
it('should not log and not exit in text mode', () => {
it('should log error and not exit in text mode', () => {
(
mockConfig.getOutputFormat as ReturnType<typeof vi.fn>
).mockReturnValue(OutputFormat.TEXT);
handleToolError(toolName, toolError, mockConfig);
expect(consoleErrorSpy).not.toHaveBeenCalled();
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
});
it('should not log and not exit in JSON mode', () => {
it('should log error and not exit in JSON mode', () => {
(
mockConfig.getOutputFormat as ReturnType<typeof vi.fn>
).mockReturnValue(OutputFormat.JSON);
handleToolError(toolName, toolError, mockConfig);
expect(consoleErrorSpy).not.toHaveBeenCalled();
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
});
it('should not log and not exit in STREAM_JSON mode', () => {
it('should log error and not exit in STREAM_JSON mode', () => {
(
mockConfig.getOutputFormat as ReturnType<typeof vi.fn>
).mockReturnValue(OutputFormat.STREAM_JSON);
handleToolError(toolName, toolError, mockConfig);
expect(consoleErrorSpy).not.toHaveBeenCalled();
expect(debugLoggerSpy.error).toHaveBeenCalledWith(
'Error executing tool test-tool: Tool failed',
);
expect(processExitSpy).not.toHaveBeenCalled();
});
});
@ -565,7 +595,9 @@ describe('errors', () => {
handleCancellationError(mockConfig);
}).toThrow('process.exit called with code: 130');
expect(consoleErrorSpy).toHaveBeenCalledWith('Operation cancelled.');
expect(mockWriteStderrLine).toHaveBeenCalledWith(
'Operation cancelled.',
);
});
});
@ -581,7 +613,7 @@ describe('errors', () => {
handleCancellationError(mockConfig);
}).toThrow('process.exit called with code: 130');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {
@ -611,7 +643,7 @@ describe('errors', () => {
handleMaxTurnsExceededError(mockConfig);
}).toThrow('process.exit called with code: 53');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.',
);
});
@ -629,7 +661,7 @@ describe('errors', () => {
handleMaxTurnsExceededError(mockConfig);
}).toThrow('process.exit called with code: 53');
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect(mockWriteStderrLine).toHaveBeenCalledWith(
JSON.stringify(
{
error: {

View file

@ -11,9 +11,14 @@ import * as path from 'node:path';
import * as childProcess from 'node:child_process';
import { isGitRepository } from '@qwen-code/qwen-code-core';
vi.mock('@qwen-code/qwen-code-core', () => ({
isGitRepository: vi.fn(),
}));
vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const actual =
await importOriginal<typeof import('@qwen-code/qwen-code-core')>();
return {
...actual,
isGitRepository: vi.fn(),
};
});
vi.mock('fs', async (importOriginal) => {
const actualFs = await importOriginal<typeof fs>();
@ -58,8 +63,7 @@ describe('getInstallationInfo', () => {
expect(info.packageManager).toBe(PackageManager.UNKNOWN);
});
it('should return UNKNOWN and log error if realpathSync fails', () => {
const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
it('should return UNKNOWN if realpathSync fails', () => {
process.argv[1] = '/path/to/cli';
const error = new Error('realpath failed');
mockedRealPathSync.mockImplementation(() => {
@ -69,8 +73,6 @@ describe('getInstallationInfo', () => {
const info = getInstallationInfo(projectRoot, false);
expect(info.packageManager).toBe(PackageManager.UNKNOWN);
expect(consoleSpy).toHaveBeenCalledWith(error);
consoleSpy.mockRestore();
});
it('should detect running from a local git clone', () => {

View file

@ -16,6 +16,8 @@ import {
} from './modelConfigUtils.js';
import type { Settings } from '../config/settings.js';
const mockWriteStderrLine = vi.hoisted(() => vi.fn());
vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const original =
await importOriginal<typeof import('@qwen-code/qwen-code-core')>();
@ -25,6 +27,12 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
};
});
vi.mock('./stdioHelpers.js', () => ({
writeStderrLine: mockWriteStderrLine,
writeStdoutLine: vi.fn(),
clearScreen: vi.fn(),
}));
describe('modelConfigUtils', () => {
describe('getAuthTypeFromEnv', () => {
const originalEnv = process.env;
@ -122,17 +130,15 @@ describe('modelConfigUtils', () => {
describe('resolveCliGenerationConfig', () => {
const originalEnv = process.env;
const originalConsoleWarn = console.warn;
beforeEach(() => {
vi.resetModules();
process.env = { ...originalEnv };
console.warn = vi.fn();
mockWriteStderrLine.mockClear();
});
afterEach(() => {
process.env = originalEnv;
console.warn = originalConsoleWarn;
vi.clearAllMocks();
});
@ -521,8 +527,8 @@ describe('modelConfigUtils', () => {
selectedAuthType,
});
expect(console.warn).toHaveBeenCalledWith('Warning 1');
expect(console.warn).toHaveBeenCalledWith('Warning 2');
expect(mockWriteStderrLine).toHaveBeenCalledWith('Warning 1');
expect(mockWriteStderrLine).toHaveBeenCalledWith('Warning 2');
});
it('should use custom env when provided', () => {

View file

@ -33,14 +33,12 @@ import { relaunchAppInChildProcess, relaunchOnExitCode } from './relaunch.js';
describe('relaunchOnExitCode', () => {
let processExitSpy: MockInstance;
let consoleErrorSpy: MockInstance;
let stdinResumeSpy: MockInstance;
beforeEach(() => {
processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => {
throw new Error('PROCESS_EXIT_CALLED');
});
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
stdinResumeSpy = vi
.spyOn(process.stdin, 'resume')
.mockImplementation(() => process.stdin);
@ -49,7 +47,6 @@ describe('relaunchOnExitCode', () => {
afterEach(() => {
processExitSpy.mockRestore();
consoleErrorSpy.mockRestore();
stdinResumeSpy.mockRestore();
});
@ -90,10 +87,6 @@ describe('relaunchOnExitCode', () => {
);
expect(runner).toHaveBeenCalledTimes(1);
expect(consoleErrorSpy).toHaveBeenCalledWith(
'Fatal error: Failed to relaunch the CLI process.',
error,
);
expect(stdinResumeSpy).toHaveBeenCalled();
expect(processExitSpy).toHaveBeenCalledWith(1);
});
@ -101,7 +94,6 @@ describe('relaunchOnExitCode', () => {
describe('relaunchAppInChildProcess', () => {
let processExitSpy: MockInstance;
let consoleErrorSpy: MockInstance;
let stdinPauseSpy: MockInstance;
let stdinResumeSpy: MockInstance;
@ -124,7 +116,6 @@ describe('relaunchAppInChildProcess', () => {
processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => {
throw new Error('PROCESS_EXIT_CALLED');
});
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
stdinPauseSpy = vi
.spyOn(process.stdin, 'pause')
.mockImplementation(() => process.stdin);
@ -140,7 +131,6 @@ describe('relaunchAppInChildProcess', () => {
process.execPath = originalExecPath;
processExitSpy.mockRestore();
consoleErrorSpy.mockRestore();
stdinPauseSpy.mockRestore();
stdinResumeSpy.mockRestore();
});

File diff suppressed because it is too large Load diff