From c87a3c3fa8ac3c8ebf278d0d994d63e89bbfe101 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Thu, 15 Oct 2026 13:37:56 +0200 Subject: [PATCH] Prevent theme telemetry failures from replacing command results --- .../theme/transfer-environments.test.ts | 14 ++--- .../utilities/theme-command-analytics.test.ts | 51 +++++++++++++++++++ .../theme/src/cli/utilities/theme-command.ts | 32 +++++++----- 3 files changed, 78 insertions(+), 19 deletions(-) create mode 100644 packages/theme/src/cli/utilities/theme-command-analytics.test.ts diff --git a/packages/theme/src/cli/commands/theme/transfer-environments.test.ts b/packages/theme/src/cli/commands/theme/transfer-environments.test.ts index 38bab40c6e8..085a88bacaa 100644 --- a/packages/theme/src/cli/commands/theme/transfer-environments.test.ts +++ b/packages/theme/src/cli/commands/theme/transfer-environments.test.ts @@ -4,6 +4,7 @@ import Share from './share.js' import {executeThemePush} from '../../services/push.js' import {executeThemePull} from '../../services/pull.js' import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' import {loadEnvironment} from '@shopify/cli-kit/node/environments' import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' @@ -20,17 +21,15 @@ vi.mock('@shopify/cli-kit/node/metadata') class TestPush extends Push { public parse = vi.fn() } - class TestPull extends Pull { public parse = vi.fn() } - class TestShare extends Share { public parse = vi.fn() } describe.each([TestPush, TestPull, TestShare])('%s', (Command) => { - test.each(['none', 'partial', 'total', 'cancelled'] as const)( + test.each(['none', 'partial', 'total', 'cancelled', 'analytics'] as const)( 'retains all environments in request order with %s failures', async (failures) => { await inTemporaryDirectory(async (path) => { @@ -45,6 +44,11 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => { const secondStarted = new Promise((resolve) => { releaseFirst = resolve }) + vi.mocked(addPublicMetadata).mockImplementation(async (collect) => { + const metadata = await collect() + if (failures === 'analytics' && metadata?.store_domain === 'second.myshopify.com') + throw new Error('analytics failed') + }) const executionOrder: string[] = [] const execute = async (flags: {environment?: string[]}, session?: {storeFqdn: string}) => { const environment = flags.environment![0]! @@ -73,7 +77,6 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => { } vi.mocked(executeThemePush).mockImplementation(execute) vi.mocked(executeThemePull).mockImplementation(execute) - const config = new Config({root: __dirname}) await config.load() const command = new Command( @@ -114,7 +117,7 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => { .map((line) => JSON.parse(line)) : [] expect(events.filter((event) => event.level === 'error')).toHaveLength( - {none: 0, partial: 1, total: 3, cancelled: 0}[failures], + {none: 0, partial: 1, total: 3, cancelled: 0, analytics: 0}[failures], ) }) } finally { @@ -151,7 +154,6 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => { ]) expect(executeThemePush).not.toHaveBeenCalled() expect(executeThemePull).not.toHaveBeenCalled() - expect(process.exitCode).toBe(1) }) } finally { diff --git a/packages/theme/src/cli/utilities/theme-command-analytics.test.ts b/packages/theme/src/cli/utilities/theme-command-analytics.test.ts new file mode 100644 index 00000000000..b36a3ed7914 --- /dev/null +++ b/packages/theme/src/cli/utilities/theme-command-analytics.test.ts @@ -0,0 +1,51 @@ +import ThemeCommand from './theme-command.js' +import {Config, Flags} from '@oclif/core' +import {compileData} from '@shopify/cli-kit/node/analytics' +import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' +import {expect, test, vi} from 'vitest' + +vi.mock('@shopify/cli-kit/node/analytics') +vi.mock('@shopify/cli-kit/node/session') +vi.mock('@shopify/cli-kit/node/metadata') + +test.each( + ['compile', 'public metadata', 'sensitive metadata'].flatMap((failure) => + [false, true].map((commandFails) => ({failure, commandFails})), + ), +)('preserves the command outcome when $failure fails, commandFails=$commandFails', async ({failure, commandFails}) => { + const commandError = new Error('Command failed') + const telemetryError = new Error('Telemetry failed') + class TestCommand extends ThemeCommand { + static flags = { + store: Flags.string({env: 'SHOPIFY_FLAG_STORE'}), + password: Flags.string({env: 'SHOPIFY_FLAG_PASSWORD'}), + } + + static multiEnvironmentsFlags = ['store'] + + command = vi.fn(async () => { + if (commandFails) throw commandError + }) + } + + vi.mocked(ensureAuthenticatedThemes).mockResolvedValue({storeFqdn: 'example.myshopify.com', token: 'token'}) + vi.mocked(compileData).mockImplementation(() => { + if (failure === 'compile') throw telemetryError + return {timings: {}, errors: {}, retries: {}, events: {}} as ReturnType + }) + vi.mocked(addPublicMetadata).mockImplementation(async (metadata) => { + if (failure === 'public metadata' && 'cmd_theme_timings' in (await metadata())) throw telemetryError + }) + if (failure === 'sensitive metadata') vi.mocked(addSensitiveMetadata).mockRejectedValue(telemetryError) + const config = new Config({root: __dirname}) + await config.load() + const command = new TestCommand(['--store', 'example.myshopify.com', '--password', 'token'], config) + + if (commandFails) { + await expect(command.run()).rejects.toBe(commandError) + } else { + await expect(command.run()).resolves.toBeUndefined() + } + expect(command.command).toHaveBeenCalledOnce() +}) diff --git a/packages/theme/src/cli/utilities/theme-command.ts b/packages/theme/src/cli/utilities/theme-command.ts index 9b51b2eaaf2..71f0b126df8 100644 --- a/packages/theme/src/cli/utilities/theme-command.ts +++ b/packages/theme/src/cli/utilities/theme-command.ts @@ -741,18 +741,24 @@ export default abstract class ThemeCommand extends Command { private async logAnalyticsData(session?: AdminSession): Promise { if (!session) return - const data = compileData() - await addPublicMetadata(() => ({ - store_fqdn_hash: hashString(session.storeFqdn), - store_domain: session.storeFqdn, - - cmd_theme_timings: JSON.stringify(data.timings), - cmd_theme_errors: JSON.stringify(data.errors), - cmd_theme_retries: JSON.stringify(data.retries), - cmd_theme_events: JSON.stringify(data.events), - })) - await addSensitiveMetadata(() => ({ - store_fqdn: session.storeFqdn, - })) + try { + const data = compileData() + await addPublicMetadata(() => ({ + store_fqdn_hash: hashString(session.storeFqdn), + store_domain: session.storeFqdn, + + cmd_theme_timings: JSON.stringify(data.timings), + cmd_theme_errors: JSON.stringify(data.errors), + cmd_theme_retries: JSON.stringify(data.retries), + cmd_theme_events: JSON.stringify(data.events), + })) + await addSensitiveMetadata(() => ({ + store_fqdn: session.storeFqdn, + })) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + // Telemetry must not discard completed work or replace the command's original error. + outputDebug(`Unable to record theme analytics: ${error instanceof Error ? error.message : String(error)}`) + } } }