From 873b149d8f063d5ebd36979816b3a8dc15b48b0e Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 17 Aug 2022 10:19:51 +1000 Subject: [PATCH] any retry and return a default when error is unexpected --- extension/src/cli/constants.ts | 2 +- extension/src/cli/reader.test.ts | 7 ++++-- extension/src/cli/reader.ts | 41 +++++++------------------------- extension/src/cli/retry.test.ts | 4 ++-- extension/src/cli/retry.ts | 19 +++++---------- 5 files changed, 22 insertions(+), 51 deletions(-) diff --git a/extension/src/cli/constants.ts b/extension/src/cli/constants.ts index 6f2c90dc8a..5ff51a6897 100644 --- a/extension/src/cli/constants.ts +++ b/extension/src/cli/constants.ts @@ -2,7 +2,7 @@ export const MIN_CLI_VERSION = '2.11.0' export const LATEST_TESTED_CLI_VERSION = '2.13.0' export const MAX_CLI_VERSION = '3' -export const EMPTY_REPO_ERROR = 'unexpected error - Empty git repo' +export const UNEXPECTED_ERROR_CODE = 255 export enum Command { ADD = 'add', diff --git a/extension/src/cli/reader.test.ts b/extension/src/cli/reader.test.ts index 4a19d59f33..3956e6636c 100644 --- a/extension/src/cli/reader.test.ts +++ b/extension/src/cli/reader.test.ts @@ -2,7 +2,8 @@ import { join } from 'path' import { EventEmitter } from 'vscode' import { Disposable, Disposer } from '@hediet/std/disposable' import { CliResult, CliStarted } from '.' -import { EMPTY_REPO_ERROR } from './constants' +import { UNEXPECTED_ERROR_CODE } from './constants' +import { MaybeConsoleError } from './error' import { CliReader, ExperimentsOutput } from './reader' import { createProcess } from '../processExecution' import { getFailingMockedProcess, getMockedProcess } from '../test/util/jest' @@ -80,8 +81,10 @@ describe('CliReader', () => { it('should return the default output if the cli returns an empty repo error', async () => { const cwd = __dirname + const error = new Error('unexpected error - something something') + ;(error as MaybeConsoleError).exitCode = UNEXPECTED_ERROR_CODE mockedCreateProcess.mockImplementationOnce(() => { - throw new Error(EMPTY_REPO_ERROR) + throw error }) const cliOutput = await cliReader.expShow(cwd) diff --git a/extension/src/cli/reader.ts b/extension/src/cli/reader.ts index 8ecf50b6a5..889714a648 100644 --- a/extension/src/cli/reader.ts +++ b/extension/src/cli/reader.ts @@ -3,7 +3,6 @@ import { Cli, typeCheckCommands } from '.' import { Args, Command, - EMPTY_REPO_ERROR, ExperimentFlag, Flag, ListFlag, @@ -137,13 +136,14 @@ export class CliReader extends Cli { cwd: string, ...flags: ExperimentFlag[] ): Promise { - return this.readProcessJsonWithKnownErrors( + return this.readProcess( cwd, + JSON.parse, + JSON.stringify(defaultExperimentsOutput), Command.EXPERIMENT, - defaultExperimentsOutput, - [EMPTY_REPO_ERROR], SubCommand.SHOW, - ...flags + ...flags, + Flag.SHOW_JSON ) } @@ -162,11 +162,9 @@ export class CliReader extends Cli { } public plotsDiff(cwd: string, ...revisions: string[]): Promise { - return this.readProcessJsonWithKnownErrors( + return this.readProcessJson( cwd, Command.PLOTS, - {}, - [EMPTY_REPO_ERROR], Command.DIFF, ...revisions, Flag.OUTPUT_PATH, @@ -193,15 +191,11 @@ export class CliReader extends Cli { cwd: string, formatter: typeof trimAndSplit | typeof JSON.parse | typeof trim, defaultValue: string, - nonRetryErrors: string[], ...args: Args ): Promise { const output = - (await retry( - () => this.executeProcess(cwd, ...args), - args.join(' '), - nonRetryErrors - )) || defaultValue + (await retry(() => this.executeProcess(cwd, ...args), args.join(' '))) || + defaultValue if (!formatter) { return output as unknown as T } @@ -213,25 +207,6 @@ export class CliReader extends Cli { cwd, JSON.parse, '{}', - [], - command, - ...args, - Flag.SHOW_JSON - ) - } - - private readProcessJsonWithKnownErrors( - cwd: string, - command: Command, - defaultValue: T, - nonRetryErrors: string[], - ...args: Args - ) { - return this.readProcess( - cwd, - JSON.parse, - JSON.stringify(defaultValue), - nonRetryErrors, command, ...args, Flag.SHOW_JSON diff --git a/extension/src/cli/retry.test.ts b/extension/src/cli/retry.test.ts index c052a6557d..d034fef76d 100644 --- a/extension/src/cli/retry.test.ts +++ b/extension/src/cli/retry.test.ts @@ -17,7 +17,7 @@ describe('retry', () => { const promiseRefresher = jest.fn().mockImplementation(() => promise()) - const output = await retry(promiseRefresher, 'Definitely did not', []) + const output = await retry(promiseRefresher, 'Definitely did not') expect(output).toStrictEqual(returnValue) @@ -45,7 +45,7 @@ describe('retry', () => { .fn() .mockImplementation(() => unreliablePromise()) - await retry(promiseRefresher, 'Data update', []) + await retry(promiseRefresher, 'Data update') expect(promiseRefresher).toBeCalledTimes(4) expect(mockedDelay).toBeCalledTimes(3) diff --git a/extension/src/cli/retry.ts b/extension/src/cli/retry.ts index 371c8271f2..1995e3b5fd 100644 --- a/extension/src/cli/retry.ts +++ b/extension/src/cli/retry.ts @@ -1,22 +1,15 @@ +import { UNEXPECTED_ERROR_CODE } from './constants' +import { MaybeConsoleError } from './error' import { delay } from '../util/time' import { Logger } from '../common/logger' -const isNonRetryError = ( - errorMessage: string, - nonRetryErrors: string[] -): boolean => { - for (const partialErrorMessage of nonRetryErrors) { - if (errorMessage.includes(partialErrorMessage)) { - return true - } - } - return false +const isUnexpectedError = (error: unknown): boolean => { + return (error as MaybeConsoleError).exitCode === UNEXPECTED_ERROR_CODE } export const retry = async ( getNewPromise: () => Promise, args: string, - nonRetryErrors: string[], waitBeforeRetry = 500 ): Promise => { try { @@ -25,11 +18,11 @@ export const retry = async ( const errorMessage = (error as Error).message Logger.error(`${args} failed with ${errorMessage} retrying...`) - if (isNonRetryError(errorMessage, nonRetryErrors)) { + if (isUnexpectedError(error)) { return '' } await delay(waitBeforeRetry) - return retry(getNewPromise, args, nonRetryErrors, waitBeforeRetry * 2) + return retry(getNewPromise, args, waitBeforeRetry * 2) } }