From f379aa833fbd859d1c3795114b3472c4b1a1173f Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Fri, 25 Jul 2025 10:32:23 -0700 Subject: [PATCH] Make errorReporting test windows compatible. (#4856) --- .../core/src/utils/errorReporting.test.ts | 144 +++++++----------- packages/core/src/utils/errorReporting.ts | 3 +- 2 files changed, 61 insertions(+), 86 deletions(-) diff --git a/packages/core/src/utils/errorReporting.test.ts b/packages/core/src/utils/errorReporting.test.ts index 8d1a5bab..6b92c249 100644 --- a/packages/core/src/utils/errorReporting.test.ts +++ b/packages/core/src/utils/errorReporting.test.ts @@ -4,36 +4,36 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach, Mock } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { reportError } from './errorReporting.js'; // Use a type alias for SpyInstance as it's not directly exported type SpyInstance = ReturnType; -import { reportError } from './errorReporting.js'; -import fs from 'node:fs/promises'; -import os from 'node:os'; - -// Mock dependencies -vi.mock('node:fs/promises'); -vi.mock('node:os'); describe('reportError', () => { let consoleErrorSpy: SpyInstance; - const MOCK_TMP_DIR = '/tmp'; + let testDir: string; const MOCK_TIMESTAMP = '2025-01-01T00-00-00-000Z'; - beforeEach(() => { + beforeEach(async () => { + // Create a temporary directory for logs + testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gemini-report-test-')); vi.resetAllMocks(); consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - (os.tmpdir as Mock).mockReturnValue(MOCK_TMP_DIR); vi.spyOn(Date.prototype, 'toISOString').mockReturnValue(MOCK_TIMESTAMP); }); - afterEach(() => { + afterEach(async () => { vi.restoreAllMocks(); + // Clean up the temporary directory + await fs.rm(testDir, { recursive: true, force: true }); }); const getExpectedReportPath = (type: string) => - `${MOCK_TMP_DIR}/gemini-client-error-${type}-${MOCK_TIMESTAMP}.json`; + path.join(testDir, `gemini-client-error-${type}-${MOCK_TIMESTAMP}.json`); it('should generate a report and log the path', async () => { const error = new Error('Test error'); @@ -43,22 +43,18 @@ describe('reportError', () => { const type = 'test-type'; const expectedReportPath = getExpectedReportPath(type); - (fs.writeFile as Mock).mockResolvedValue(undefined); + await reportError(error, baseMessage, context, type, testDir); - await reportError(error, baseMessage, context, type); + // Verify the file was written + const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); + const parsedReport = JSON.parse(reportContent); - expect(os.tmpdir).toHaveBeenCalledTimes(1); - expect(fs.writeFile).toHaveBeenCalledWith( - expectedReportPath, - JSON.stringify( - { - error: { message: 'Test error', stack: error.stack }, - context, - }, - null, - 2, - ), - ); + expect(parsedReport).toEqual({ + error: { message: 'Test error', stack: 'Test stack' }, + context, + }); + + // Verify the console log expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, ); @@ -70,19 +66,15 @@ describe('reportError', () => { const type = 'general'; const expectedReportPath = getExpectedReportPath(type); - (fs.writeFile as Mock).mockResolvedValue(undefined); - await reportError(error, baseMessage); + await reportError(error, baseMessage, undefined, type, testDir); + + const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); + const parsedReport = JSON.parse(reportContent); + + expect(parsedReport).toEqual({ + error: { message: 'Test plain object error' }, + }); - expect(fs.writeFile).toHaveBeenCalledWith( - expectedReportPath, - JSON.stringify( - { - error: { message: 'Test plain object error' }, - }, - null, - 2, - ), - ); expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, ); @@ -94,19 +86,15 @@ describe('reportError', () => { const type = 'general'; const expectedReportPath = getExpectedReportPath(type); - (fs.writeFile as Mock).mockResolvedValue(undefined); - await reportError(error, baseMessage); + await reportError(error, baseMessage, undefined, type, testDir); + + const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); + const parsedReport = JSON.parse(reportContent); + + expect(parsedReport).toEqual({ + error: { message: 'Just a string error' }, + }); - expect(fs.writeFile).toHaveBeenCalledWith( - expectedReportPath, - JSON.stringify( - { - error: { message: 'Just a string error' }, - }, - null, - 2, - ), - ); expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, ); @@ -115,22 +103,15 @@ describe('reportError', () => { it('should log fallback message if writing report fails', async () => { const error = new Error('Main error'); const baseMessage = 'Failed operation.'; - const writeError = new Error('Failed to write file'); const context = ['some context']; const type = 'general'; - const expectedReportPath = getExpectedReportPath(type); + const nonExistentDir = path.join(testDir, 'non-existent-dir'); - (fs.writeFile as Mock).mockRejectedValue(writeError); + await reportError(error, baseMessage, context, type, nonExistentDir); - await reportError(error, baseMessage, context, type); - - expect(fs.writeFile).toHaveBeenCalledWith( - expectedReportPath, - expect.any(String), - ); // It still tries to write expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Additionally, failed to write detailed error report:`, - writeError, + expect.any(Error), // The actual write error ); expect(consoleErrorSpy).toHaveBeenCalledWith( 'Original error that triggered report generation:', @@ -163,9 +144,7 @@ describe('reportError', () => { return originalJsonStringify(value, replacer, space); }); - (fs.writeFile as Mock).mockResolvedValue(undefined); // Mock for the minimal report write - - await reportError(error, baseMessage, context, type); + await reportError(error, baseMessage, context, type, testDir); expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Could not stringify report content (likely due to context):`, @@ -178,15 +157,14 @@ describe('reportError', () => { expect(consoleErrorSpy).toHaveBeenCalledWith( 'Original context could not be stringified or included in report.', ); - // Check that it attempts to write a minimal report - expect(fs.writeFile).toHaveBeenCalledWith( - expectedMinimalReportPath, - originalJsonStringify( - { error: { message: error.message, stack: error.stack } }, - null, - 2, - ), - ); + + // Check that it writes a minimal report + const reportContent = await fs.readFile(expectedMinimalReportPath, 'utf-8'); + const parsedReport = JSON.parse(reportContent); + expect(parsedReport).toEqual({ + error: { message: error.message, stack: error.stack }, + }); + expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Partial report (excluding context) available at: ${expectedMinimalReportPath}`, ); @@ -199,19 +177,15 @@ describe('reportError', () => { const type = 'general'; const expectedReportPath = getExpectedReportPath(type); - (fs.writeFile as Mock).mockResolvedValue(undefined); - await reportError(error, baseMessage, undefined, type); + await reportError(error, baseMessage, undefined, type, testDir); + + const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); + const parsedReport = JSON.parse(reportContent); + + expect(parsedReport).toEqual({ + error: { message: 'Error without context', stack: 'No context stack' }, + }); - expect(fs.writeFile).toHaveBeenCalledWith( - expectedReportPath, - JSON.stringify( - { - error: { message: 'Error without context', stack: error.stack }, - }, - null, - 2, - ), - ); expect(consoleErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, ); diff --git a/packages/core/src/utils/errorReporting.ts b/packages/core/src/utils/errorReporting.ts index 41ce3468..e7aa3469 100644 --- a/packages/core/src/utils/errorReporting.ts +++ b/packages/core/src/utils/errorReporting.ts @@ -27,10 +27,11 @@ export async function reportError( baseMessage: string, context?: Content[] | Record | unknown[], type = 'general', + reportingDir = os.tmpdir(), // for testing ): Promise { const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); const reportFileName = `gemini-client-error-${type}-${timestamp}.json`; - const reportPath = path.join(os.tmpdir(), reportFileName); + const reportPath = path.join(reportingDir, reportFileName); let errorToReport: { message: string; stack?: string }; if (error instanceof Error) {