From 12472ce9b22d8d73e7da852c670d0df0d3de48f4 Mon Sep 17 00:00:00 2001 From: Jainam M Date: Fri, 31 Oct 2025 13:03:33 +0530 Subject: [PATCH] =?UTF-8?q?refactor(core):=20Refactored=20and=20removed=20?= =?UTF-8?q?redundant=20test=20lines=20in=20teleme=E2=80=A6=20(#12284)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../clearcut-logger/clearcut-logger.test.ts | 129 +++++------- packages/core/src/telemetry/metrics.test.ts | 189 +++++++----------- .../src/telemetry/telemetry-utils.test.ts | 66 +++--- 3 files changed, 155 insertions(+), 229 deletions(-) diff --git a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.test.ts b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.test.ts index 10705cd24f..8ce24d2a59 100644 --- a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.test.ts +++ b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.test.ts @@ -211,45 +211,6 @@ describe('ClearcutLogger', () => { }); }); - it('logs the current surface from a github action', () => { - const { logger } = setup({}); - - vi.stubEnv('GITHUB_SHA', '8675309'); - - const event = logger?.createLogEvent(EventNames.CHAT_COMPRESSION, []); - - expect(event?.event_metadata[0]).toContainEqual({ - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SURFACE, - value: 'GitHub', - }); - }); - - it('logs the current surface from Cloud Shell via EDITOR_IN_CLOUD_SHELL', () => { - const { logger } = setup({}); - - vi.stubEnv('EDITOR_IN_CLOUD_SHELL', 'true'); - - const event = logger?.createLogEvent(EventNames.CHAT_COMPRESSION, []); - - expect(event?.event_metadata[0]).toContainEqual({ - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SURFACE, - value: 'cloudshell', - }); - }); - - it('logs the current surface from Cloud Shell via CLOUD_SHELL', () => { - const { logger } = setup({}); - - vi.stubEnv('CLOUD_SHELL', 'true'); - - const event = logger?.createLogEvent(EventNames.CHAT_COMPRESSION, []); - - expect(event?.event_metadata[0]).toContainEqual({ - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SURFACE, - value: 'cloudshell', - }); - }); - it('logs default metadata', () => { // Define expected values const session_id = 'my-session-id'; @@ -329,20 +290,6 @@ describe('ClearcutLogger', () => { }); }); - it('logs the current surface', () => { - const { logger } = setup({}); - - vi.stubEnv('TERM_PROGRAM', 'vscode'); - vi.stubEnv('SURFACE', 'ide-1234'); - - const event = logger?.createLogEvent(EventNames.API_ERROR, []); - - expect(event?.event_metadata[0]).toContainEqual({ - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SURFACE, - value: 'ide-1234', - }); - }); - it('logs all user settings', () => { const { logger } = setup({ config: { useSmartEdit: true, useModelRouter: true }, @@ -359,63 +306,85 @@ describe('ClearcutLogger', () => { }); }); - it.each([ + type SurfaceDetectionTestCase = { + name: string; + env: Record; + expected: string; + }; + + it.each([ { - env: { - CURSOR_TRACE_ID: 'abc123', - GITHUB_SHA: undefined, - TERM_PROGRAM: 'vscode', - }, - expectedValue: 'cursor', + name: 'github action', + env: { GITHUB_SHA: '8675309' }, + expected: 'GitHub', }, { + name: 'Cloud Shell via EDITOR_IN_CLOUD_SHELL', + env: { EDITOR_IN_CLOUD_SHELL: 'true' }, + expected: 'cloudshell', + }, + { + name: 'Cloud Shell via CLOUD_SHELL', + env: { CLOUD_SHELL: 'true' }, + expected: 'cloudshell', + }, + { + name: 'VSCode via TERM_PROGRAM', env: { TERM_PROGRAM: 'vscode', GITHUB_SHA: undefined, MONOSPACE_ENV: '', }, - expectedValue: 'vscode', + expected: 'vscode', }, { + name: 'SURFACE env var', + env: { SURFACE: 'ide-1234' }, + expected: 'ide-1234', + }, + { + name: 'SURFACE env var takes precedence', + env: { TERM_PROGRAM: 'vscode', SURFACE: 'ide-1234' }, + expected: 'ide-1234', + }, + { + name: 'Cursor', + env: { + CURSOR_TRACE_ID: 'abc123', + TERM_PROGRAM: 'vscode', + GITHUB_SHA: undefined, + }, + expected: 'cursor', + }, + { + name: 'Firebase Studio', env: { MONOSPACE_ENV: 'true', - GITHUB_SHA: undefined, TERM_PROGRAM: 'vscode', + GITHUB_SHA: undefined, }, - expectedValue: 'firebasestudio', + expected: 'firebasestudio', }, { + name: 'Devin', env: { __COG_BASHRC_SOURCED: 'true', - GITHUB_SHA: undefined, TERM_PROGRAM: 'vscode', - }, - expectedValue: 'devin', - }, - { - env: { - CLOUD_SHELL: 'true', GITHUB_SHA: undefined, - TERM_PROGRAM: 'vscode', }, - expectedValue: 'cloudshell', + expected: 'devin', }, ])( - 'logs the current surface as $expectedValue, preempting vscode detection', - ({ env, expectedValue }) => { + 'logs the current surface as $expected from $name', + ({ env, expected }) => { const { logger } = setup({}); for (const [key, value] of Object.entries(env)) { vi.stubEnv(key, value); } - // Clear Cursor-specific environment variables that might interfere with tests - // Only clear if not explicitly testing Cursor detection - if (!env.CURSOR_TRACE_ID) { - vi.stubEnv('CURSOR_TRACE_ID', ''); - } const event = logger?.createLogEvent(EventNames.API_ERROR, []); expect(event?.event_metadata[0]).toContainEqual({ gemini_cli_key: EventMetadataKey.GEMINI_CLI_SURFACE, - value: expectedValue, + value: expected, }); }, ); diff --git a/packages/core/src/telemetry/metrics.test.ts b/packages/core/src/telemetry/metrics.test.ts index a6c8d6e861..dafedfd23a 100644 --- a/packages/core/src/telemetry/metrics.test.ts +++ b/packages/core/src/telemetry/metrics.test.ts @@ -403,126 +403,81 @@ describe('Telemetry Metrics', () => { getTelemetryEnabled: () => true, } as unknown as Config; - it('should not record metrics if not initialized', () => { - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.CREATE, - lines: 10, - mimetype: 'text/plain', - extension: 'txt', - }); - expect(mockCounterAddFn).not.toHaveBeenCalled(); - }); + type FileOperationTestCase = { + name: string; + initialized: boolean; + attributes: { + operation: FileOperation; + lines?: number; + mimetype?: string; + extension?: string; + }; + shouldCall: boolean; + }; - it('should record file creation with all attributes', () => { - initializeMetricsModule(mockConfig); - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.CREATE, - lines: 10, - mimetype: 'text/plain', - extension: 'txt', - }); + it.each([ + { + name: 'should not record metrics if not initialized', + initialized: false, + attributes: { + operation: FileOperation.CREATE, + lines: 10, + mimetype: 'text/plain', + extension: 'txt', + }, + shouldCall: false, + }, + { + name: 'should record file creation with all attributes', + initialized: true, + attributes: { + operation: FileOperation.CREATE, + lines: 10, + mimetype: 'text/plain', + extension: 'txt', + }, + shouldCall: true, + }, + { + name: 'should record file read with minimal attributes', + initialized: true, + attributes: { operation: FileOperation.READ }, + shouldCall: true, + }, + { + name: 'should record file update with some attributes', + initialized: true, + attributes: { + operation: FileOperation.UPDATE, + mimetype: 'application/javascript', + }, + shouldCall: true, + }, + { + name: 'should record file update with no optional attributes', + initialized: true, + attributes: { operation: FileOperation.UPDATE }, + shouldCall: true, + }, + ])('$name', ({ initialized, attributes, shouldCall }) => { + if (initialized) { + initializeMetricsModule(mockConfig); + // The session start event also calls the counter. + mockCounterAddFn.mockClear(); + } - expect(mockCounterAddFn).toHaveBeenCalledTimes(2); - expect(mockCounterAddFn).toHaveBeenNthCalledWith(1, 1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - }); - expect(mockCounterAddFn).toHaveBeenNthCalledWith(2, 1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.CREATE, - lines: 10, - mimetype: 'text/plain', - extension: 'txt', - }); - }); + recordFileOperationMetricModule(mockConfig, attributes); - it('should record file read with minimal attributes', () => { - initializeMetricsModule(mockConfig); - mockCounterAddFn.mockClear(); - - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.READ, - }); - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.READ, - }); - }); - - it('should record file update with some attributes', () => { - initializeMetricsModule(mockConfig); - mockCounterAddFn.mockClear(); - - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.UPDATE, - mimetype: 'application/javascript', - }); - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.UPDATE, - mimetype: 'application/javascript', - }); - }); - - it('should record file operation without diffStat', () => { - initializeMetricsModule(mockConfig); - mockCounterAddFn.mockClear(); - - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.UPDATE, - }); - - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.UPDATE, - }); - }); - - it('should record minimal file operation when optional parameters are undefined', () => { - initializeMetricsModule(mockConfig); - mockCounterAddFn.mockClear(); - - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.UPDATE, - lines: 10, - mimetype: 'text/plain', - extension: 'txt', - }); - - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.UPDATE, - lines: 10, - mimetype: 'text/plain', - extension: 'txt', - }); - }); - - it('should not include diffStat attributes when diffStat is not provided', () => { - initializeMetricsModule(mockConfig); - mockCounterAddFn.mockClear(); - - recordFileOperationMetricModule(mockConfig, { - operation: FileOperation.UPDATE, - }); - - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - operation: FileOperation.UPDATE, - }); + if (shouldCall) { + expect(mockCounterAddFn).toHaveBeenCalledWith(1, { + 'session.id': 'test-session-id', + 'installation.id': 'test-installation-id', + 'user.email': 'test@example.com', + ...attributes, + }); + } else { + expect(mockCounterAddFn).not.toHaveBeenCalled(); + } }); }); diff --git a/packages/core/src/telemetry/telemetry-utils.test.ts b/packages/core/src/telemetry/telemetry-utils.test.ts index a575865447..4240ae6666 100644 --- a/packages/core/src/telemetry/telemetry-utils.test.ts +++ b/packages/core/src/telemetry/telemetry-utils.test.ts @@ -8,39 +8,41 @@ import { describe, it, expect } from 'vitest'; import { getProgrammingLanguage } from './telemetry-utils.js'; describe('getProgrammingLanguage', () => { - it('should return the programming language when file_path is present', () => { - const args = { file_path: 'src/test.ts' }; - const language = getProgrammingLanguage(args); - expect(language).toBe('TypeScript'); - }); + type ProgrammingLanguageTestCase = { + name: string; + args: Record; + expected: string | undefined; + }; - it('should return the programming language when absolute_path is present', () => { - const args = { absolute_path: 'src/test.py' }; + it.each([ + { + name: 'file_path is present', + args: { file_path: 'src/test.ts' }, + expected: 'TypeScript', + }, + { + name: 'absolute_path is present', + args: { absolute_path: 'src/test.py' }, + expected: 'Python', + }, + { name: 'path is present', args: { path: 'src/test.go' }, expected: 'Go' }, + { + name: 'no file path is present', + args: {}, + expected: undefined, + }, + { + name: 'unknown file extensions', + args: { file_path: 'src/test.unknown' }, + expected: undefined, + }, + { + name: 'files with no extension', + args: { file_path: 'src/test' }, + expected: undefined, + }, + ])('should return $expected when $name', ({ args, expected }) => { const language = getProgrammingLanguage(args); - expect(language).toBe('Python'); - }); - - it('should return the programming language when path is present', () => { - const args = { path: 'src/test.go' }; - const language = getProgrammingLanguage(args); - expect(language).toBe('Go'); - }); - - it('should return undefined when no file path is present', () => { - const args = {}; - const language = getProgrammingLanguage(args); - expect(language).toBeUndefined(); - }); - - it('should handle unknown file extensions gracefully', () => { - const args = { file_path: 'src/test.unknown' }; - const language = getProgrammingLanguage(args); - expect(language).toBeUndefined(); - }); - - it('should handle files with no extension', () => { - const args = { file_path: 'src/test' }; - const language = getProgrammingLanguage(args); - expect(language).toBeUndefined(); + expect(language).toBe(expected); }); });