From 94a36020b2b30c9b2871141bfec0c1208b2b2627 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Thu, 16 Jan 2025 13:47:37 -0500 Subject: [PATCH 1/4] refactor(util): merge withRetries and waitUntil Problem: We had 2 different implementation of similar code, between withRetries and waitUntil. We couldn't easily merge them though since behavior was slightly different when it came to expected return types, and since one threw and the other did not. - waitUntil returns undefined if it never properly resolves - withRetries throws if it never properly resolves Solution: As a solution we keep both methods but they share the same underlying code. - waitUntil now has a backoff field to increase the delta between intervals - withRetries also has this - withRetries will retry even on a thrown exeception - on timeout it will throw the last exception it encountered Signed-off-by: nkomonen-amazon --- packages/core/src/notifications/controller.ts | 11 ++- packages/core/src/shared/crashMonitoring.ts | 18 +++- .../resourcefetcher/httpResourceFetcher.ts | 11 ++- .../src/shared/utilities/functionUtils.ts | 33 +------ .../core/src/shared/utilities/timeoutUtils.ts | 86 ++++++++++++++--- .../src/test/notifications/controller.test.ts | 48 +++++----- .../shared/utilities/functionUtils.test.ts | 83 +---------------- .../shared/utilities/timeoutUtils.test.ts | 92 ++++++++++++++++++- 8 files changed, 219 insertions(+), 163 deletions(-) diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts index 83886a14c1a..41ec81f8500 100644 --- a/packages/core/src/notifications/controller.ts +++ b/packages/core/src/notifications/controller.ts @@ -24,11 +24,11 @@ import { NotificationsNode } from './panelNode' import { Commands } from '../shared/vscode/commands2' import { RuleEngine } from './rules' import { TreeNode } from '../shared/treeview/resourceTreeDataProvider' -import { withRetries } from '../shared/utilities/functionUtils' import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetcher' import { isAmazonQ } from '../shared/extensionUtilities' import { telemetry } from '../shared/telemetry/telemetry' import { randomUUID } from '../shared/crypto' +import { waitUntil } from '../shared/utilities/timeoutUtils' const logger = getLogger('notifications') @@ -266,8 +266,8 @@ export interface NotificationFetcher { } export class RemoteFetcher implements NotificationFetcher { - public static readonly retryNumber = 5 public static readonly retryIntervalMs = 30000 + public static readonly retryTimeout = RemoteFetcher.retryIntervalMs * 5 private readonly startUpEndpoint: string = 'https://idetoolkits-hostedfiles.amazonaws.com/Notifications/VSCode/startup/1.x.json' @@ -286,7 +286,7 @@ export class RemoteFetcher implements NotificationFetcher { }) logger.verbose('Attempting to fetch notifications for category: %s at endpoint: %s', category, endpoint) - return withRetries( + return waitUntil( async () => { try { return await fetcher.getNewETagContent(versionTag) @@ -296,8 +296,9 @@ export class RemoteFetcher implements NotificationFetcher { } }, { - maxRetries: RemoteFetcher.retryNumber, - delay: RemoteFetcher.retryIntervalMs, + interval: RemoteFetcher.retryIntervalMs, + timeout: RemoteFetcher.retryTimeout, + retryOnFail: true, // No exponential backoff - necessary? } ) diff --git a/packages/core/src/shared/crashMonitoring.ts b/packages/core/src/shared/crashMonitoring.ts index 5638ad875f0..662a7907875 100644 --- a/packages/core/src/shared/crashMonitoring.ts +++ b/packages/core/src/shared/crashMonitoring.ts @@ -17,8 +17,8 @@ import fs from './fs/fs' import { getLogger } from './logger/logger' import { crashMonitoringDirName } from './constants' import { throwOnUnstableFileSystem } from './filesystemUtilities' -import { withRetries } from './utilities/functionUtils' import { truncateUuid } from './crypto' +import { waitUntil } from './utilities/timeoutUtils' const className = 'CrashMonitoring' @@ -489,7 +489,12 @@ export class FileSystemState { this.deps.devLogger?.debug(`HEARTBEAT sent for ${truncateUuid(this.ext.sessionId)}`) } const funcWithCtx = () => withFailCtx('sendHeartbeatState', func) - const funcWithRetries = withRetries(funcWithCtx, { maxRetries: 6, delay: 100, backoff: 2 }) + const funcWithRetries = waitUntil(funcWithCtx, { + timeout: 15_000, + interval: 100, + backoff: 2, + retryOnFail: true, + }) return funcWithRetries } catch (e) { @@ -542,7 +547,12 @@ export class FileSystemState { await nodeFs.rm(filePath, { force: true }) } const funcWithCtx = () => withFailCtx(ctx, func) - const funcWithRetries = withRetries(funcWithCtx, { maxRetries: 6, delay: 100, backoff: 2 }) + const funcWithRetries = waitUntil(funcWithCtx, { + timeout: 15_000, + interval: 100, + backoff: 2, + retryOnFail: true, + }) await funcWithRetries } @@ -609,7 +619,7 @@ export class FileSystemState { } const funcWithIgnoreBadFile = () => ignoreBadFileError(loadExtFromDisk) const funcWithRetries = () => - withRetries(funcWithIgnoreBadFile, { maxRetries: 6, delay: 100, backoff: 2 }) + waitUntil(funcWithIgnoreBadFile, { timeout: 15_000, interval: 100, backoff: 2, retryOnFail: true }) const funcWithCtx = () => withFailCtx('parseRunningExtFile', funcWithRetries) const ext: ExtInstanceHeartbeat | undefined = await funcWithCtx() diff --git a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts index e942e0ec2d0..e85e1ded70b 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -6,9 +6,8 @@ import { VSCODE_EXTENSION_ID } from '../extensions' import { getLogger, Logger } from '../logger' import { ResourceFetcher } from './resourcefetcher' -import { Timeout, CancelEvent } from '../utilities/timeoutUtils' +import { Timeout, CancelEvent, waitUntil } from '../utilities/timeoutUtils' import request, { RequestError } from '../request' -import { withRetries } from '../utilities/functionUtils' type RequestHeaders = { eTag?: string; gZip?: boolean } @@ -30,7 +29,6 @@ export class HttpResourceFetcher implements ResourceFetcher { showUrl: boolean friendlyName?: string timeout?: Timeout - retries?: number } ) {} @@ -97,7 +95,7 @@ export class HttpResourceFetcher implements ResourceFetcher { } private async getResponseFromGetRequest(timeout?: Timeout, headers?: RequestHeaders): Promise { - return withRetries( + return waitUntil( () => { const req = request.fetch('GET', this.url, { headers: this.buildRequestHeaders(headers), @@ -111,7 +109,10 @@ export class HttpResourceFetcher implements ResourceFetcher { return req.response.finally(() => cancelListener?.dispose()) }, { - maxRetries: this.params.retries ?? 1, + timeout: 3000, + interval: 100, + backoff: 2, + retryOnFail: true, } ) } diff --git a/packages/core/src/shared/utilities/functionUtils.ts b/packages/core/src/shared/utilities/functionUtils.ts index f61a3abde34..d21727bac1d 100644 --- a/packages/core/src/shared/utilities/functionUtils.ts +++ b/packages/core/src/shared/utilities/functionUtils.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { sleep, Timeout } from './timeoutUtils' +import { Timeout } from './timeoutUtils' /** * Creates a function that always returns a 'shared' Promise. @@ -145,34 +145,3 @@ export function cancellableDebounce( cancel: cancel, } } - -/** - * Executes the given function, retrying if it throws. - * - * @param opts - if no opts given, defaults are used - */ -export async function withRetries( - fn: () => Promise, - opts?: { maxRetries?: number; delay?: number; backoff?: number } -): Promise { - const maxRetries = opts?.maxRetries ?? 3 - const delay = opts?.delay ?? 0 - const backoff = opts?.backoff ?? 1 - - let retryCount = 0 - let latestDelay = delay - while (true) { - try { - return await fn() - } catch (err) { - retryCount++ - if (retryCount >= maxRetries) { - throw err - } - if (latestDelay > 0) { - await sleep(latestDelay) - latestDelay = latestDelay * backoff - } - } - } -} diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 8dc38cfef5c..8333d1bb804 100644 --- a/packages/core/src/shared/utilities/timeoutUtils.ts +++ b/packages/core/src/shared/utilities/timeoutUtils.ts @@ -219,40 +219,102 @@ interface WaitUntilOptions { readonly interval?: number /** Wait for "truthy" result, else wait for any defined result including `false` (default: true) */ readonly truthy?: boolean + /** A backoff multiplier for how long the next interval will be (default: None, i.e 1) */ + readonly backoff?: number + /** + * Only retries when an error is thrown, otherwise returning the result regardless of truthiness. + * - Ignores 'truthy' arg + * - If the timeout is reached it will throw the last error + * - default: false + */ + readonly retryOnFail?: boolean } +export const waitUntilDefaultTimeout = 2000 +export const waitUntilDefaultInterval = 500 + /** * Invokes `fn()` until it returns a truthy value (or non-undefined if `truthy:false`). * + * Also look at {@link withRetries} to be able to retry on failures. + * * @param fn Function whose result is checked * @param options See {@link WaitUntilOptions} * * @returns Result of `fn()`, or `undefined` if timeout was reached. */ +export async function waitUntil(fn: () => Promise, options: WaitUntilOptions & { retryOnFail: true }): Promise +export async function waitUntil( + fn: () => Promise, + options: WaitUntilOptions & { retryOnFail: false } +): Promise +export async function waitUntil( + fn: () => Promise, + options: Omit +): Promise export async function waitUntil(fn: () => Promise, options: WaitUntilOptions): Promise { - const opt = { timeout: 5000, interval: 500, truthy: true, ...options } + // set default opts + const opt = { + timeout: waitUntilDefaultTimeout, + interval: waitUntilDefaultInterval, + truthy: true, + backoff: 1, + retryOnFail: false, + ...options, + } + + let interval = opt.interval + let lastError: Error | undefined + let elapsed: number = 0 + let remaining = opt.timeout + for (let i = 0; true; i++) { const start: number = globals.clock.Date.now() let result: T - // Needed in case a caller uses a 0 timeout (function is only called once) - if (opt.timeout > 0) { - result = await Promise.race([fn(), new Promise((r) => globals.clock.setTimeout(r, opt.timeout))]) - } else { - result = await fn() + try { + // Needed in case a caller uses a 0 timeout (function is only called once) + if (remaining > 0) { + result = await Promise.race([fn(), new Promise((r) => globals.clock.setTimeout(r, remaining))]) + } else { + result = await fn() + } + + if (opt.retryOnFail || (opt.truthy && result) || (!opt.truthy && result !== undefined)) { + return result + } + } catch (e) { + if (!opt.retryOnFail) { + throw e + } + + // Unlikely to hit this, but exists for typing + if (!(e instanceof Error)) { + throw e + } + + lastError = e } // Ensures that we never overrun the timeout - opt.timeout -= globals.clock.Date.now() - start + remaining -= globals.clock.Date.now() - start + + // If the sleep will exceed the timeout, abort early + if (elapsed + interval >= remaining) { + if (!opt.retryOnFail) { + return undefined + } - if ((opt.truthy && result) || (!opt.truthy && result !== undefined)) { - return result + throw lastError } - if (i * opt.interval >= opt.timeout) { - return undefined + + // when testing, this saves the need to progress the stubbed clock + if (interval > 0) { + await sleep(interval) } - await sleep(opt.interval) + elapsed += interval + interval = interval * opt.backoff } } diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts index e28f0271d19..e7b62a518e5 100644 --- a/packages/core/src/test/notifications/controller.test.ts +++ b/packages/core/src/test/notifications/controller.test.ts @@ -6,7 +6,7 @@ import * as vscode from 'vscode' import * as FakeTimers from '@sinonjs/fake-timers' import assert from 'assert' -import sinon from 'sinon' +import sinon, { createSandbox } from 'sinon' import globals from '../../shared/extensionGlobals' import { randomUUID } from '../../shared/crypto' import { getContext } from '../../shared/vscode/setContext' @@ -27,6 +27,7 @@ import { import { HttpResourceFetcher } from '../../shared/resourcefetcher/httpResourceFetcher' import { NotificationsNode } from '../../notifications/panelNode' import { RuleEngine } from '../../notifications/rules' +import Sinon from 'sinon' // one test node to use across different tests export const panelNode: NotificationsNode = NotificationsNode.instance @@ -512,13 +513,16 @@ describe('Notifications Controller', function () { describe('RemoteFetcher', function () { let clock: FakeTimers.InstalledClock + let sandbox: Sinon.SinonSandbox before(function () { clock = installFakeClock() + sandbox = createSandbox() }) afterEach(function () { clock.reset() + sandbox.restore() }) after(function () { @@ -526,29 +530,29 @@ describe('RemoteFetcher', function () { }) it('retries and throws error', async function () { - const httpStub = sinon.stub(HttpResourceFetcher.prototype, 'getNewETagContent') + // Setup + const httpStub = sandbox.stub(HttpResourceFetcher.prototype, 'getNewETagContent') httpStub.throws(new Error('network error')) - const runClock = (async () => { - await clock.tickAsync(1) - for (let n = 1; n <= RemoteFetcher.retryNumber; n++) { - assert.equal(httpStub.callCount, n) - await clock.tickAsync(RemoteFetcher.retryIntervalMs) - } - - // Stop trying - await clock.tickAsync(RemoteFetcher.retryNumber) - assert.equal(httpStub.callCount, RemoteFetcher.retryNumber) - })() - - const fetcher = new RemoteFetcher() - await fetcher - .fetch('startUp', 'any') - .then(() => assert.ok(false, 'Did not throw exception.')) - .catch(() => assert.ok(true)) - await runClock - - httpStub.restore() + // Start function under test + const fetcher = new RemoteFetcher().fetch('startUp', 'any').then(() => assert.fail('Did not throw exception.')) + + // Progresses the clock, allowing the fetcher logic to break out of sleep for each iteration of withRetries() + assert.strictEqual(httpStub.callCount, 1) // 0 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 2) // 30_000 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 3) // 60_000 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 4) // 120_000 + httpStub.throws(new Error('last error')) + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 5) // 150_000 + + // We hit timeout so the last error will be thrown + await assert.rejects(fetcher, (e) => { + return e instanceof Error && e.message === 'last error' + }) }) }) diff --git a/packages/core/src/test/shared/utilities/functionUtils.test.ts b/packages/core/src/test/shared/utilities/functionUtils.test.ts index fb87cf3501e..43da4ebb619 100644 --- a/packages/core/src/test/shared/utilities/functionUtils.test.ts +++ b/packages/core/src/test/shared/utilities/functionUtils.test.ts @@ -4,10 +4,8 @@ */ import assert from 'assert' -import { once, onceChanged, debounce, withRetries } from '../../../shared/utilities/functionUtils' +import { once, onceChanged, debounce } from '../../../shared/utilities/functionUtils' import { installFakeClock } from '../../testUtil' -import { stub, SinonStub } from 'sinon' -import { InstalledClock } from '@sinonjs/fake-timers' describe('functionUtils', function () { it('once()', function () { @@ -109,82 +107,3 @@ describe('debounce', function () { }) }) }) - -// function to test the withRetries method. It passes in a stub function as the argument and has different tests that throw on different iterations -describe('withRetries', function () { - let clock: InstalledClock - let fn: SinonStub - - beforeEach(function () { - fn = stub() - clock = installFakeClock() - }) - - afterEach(function () { - clock.uninstall() - }) - - it('retries the function until it succeeds, using defaults', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).resolves('success') - assert.strictEqual(await withRetries(fn), 'success') - }) - - it('retries the function until it succeeds at the final try', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).resolves('success') - assert.strictEqual(await withRetries(fn, { maxRetries: 4 }), 'success') - }) - - it('throws the last error if the function always fails, using defaults', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).resolves('unreachable') - await assert.rejects(async () => { - await withRetries(fn) - }) - }) - - it('throws the last error if the function always fails', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).throws() - fn.onCall(4).resolves('unreachable') - await assert.rejects(async () => { - await withRetries(fn, { maxRetries: 4 }) - }) - }) - - it('honors retry delay + backoff multiplier', async function () { - fn.onCall(0).throws() // 100ms - fn.onCall(1).throws() // 200ms - fn.onCall(2).throws() // 400ms - fn.onCall(3).resolves('success') - - const res = withRetries(fn, { maxRetries: 4, delay: 100, backoff: 2 }) - - // Check the call count after each iteration, ensuring the function is called - // after the correct delay between retries. - await clock.tickAsync(99) - assert.strictEqual(fn.callCount, 1) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 2) - - await clock.tickAsync(199) - assert.strictEqual(fn.callCount, 2) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 3) - - await clock.tickAsync(399) - assert.strictEqual(fn.callCount, 3) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 4) - - assert.strictEqual(await res, 'success') - }) -}) diff --git a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts index c518b1cfae1..5409e617d75 100644 --- a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts +++ b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts @@ -7,7 +7,7 @@ import assert from 'assert' import * as FakeTimers from '@sinonjs/fake-timers' import * as timeoutUtils from '../../../shared/utilities/timeoutUtils' import { installFakeClock, tickPromise } from '../../../test/testUtil' -import { sleep } from '../../../shared/utilities/timeoutUtils' +import { sleep, waitUntil } from '../../../shared/utilities/timeoutUtils' import { SinonStub, SinonSandbox, createSandbox } from 'sinon' // We export this describe() so it can be used in the web tests as well @@ -303,6 +303,17 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { assert.strictEqual(returnValue, testSettings.callGoal) }) + it('returns value after multiple function calls WITH backoff', async function () { + testSettings.callGoal = 4 + const returnValue: number | undefined = await timeoutUtils.waitUntil(testFunction, { + timeout: 10000, + interval: 10, + truthy: false, + backoff: 2, + }) + assert.strictEqual(returnValue, testSettings.callGoal) + }) + it('timeout before function returns defined value', async function () { testSettings.callGoal = 7 const returnValue: number | undefined = await timeoutUtils.waitUntil(testFunction, { @@ -376,6 +387,85 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { }) }) + describe('waitUntil w/ retries', function () { + let fn: SinonStub + + beforeEach(function () { + fn = sandbox.stub() + }) + + it('retries the function until it succeeds', async function () { + fn.onCall(0).throws() + fn.onCall(1).throws() + fn.onCall(2).resolves('success') + + const res = waitUntil(fn, { retryOnFail: true }) + + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 3) + assert.strictEqual(await res, 'success') + }) + + it('retryOnFail ignores truthiness', async function () { + fn.resolves(false) + const res = waitUntil(fn, { retryOnFail: true, truthy: true }) + assert.strictEqual(await res, false) + }) + + // This test causes the following error, cannot figure out why: + // `rejected promise not handled within 1 second: Error: last` + it('throws the last error if the function always fails, using defaults', async function () { + fn.onCall(0).throws() // 0 + fn.onCall(1).throws() // 500 + fn.onCall(2).throws(new Error('second last')) // 1000 + fn.onCall(3).throws(new Error('last')) // 1500 + fn.onCall(4).resolves('this is not hit') + + const res = waitUntil(fn, { retryOnFail: true }) + + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 500 + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 1000 + assert.strictEqual(fn.callCount, 3) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 1500 + assert.strictEqual(fn.callCount, 4) + + await assert.rejects(res, (e) => e instanceof Error && e.message === 'last') + }) + + it('honors retry delay + backoff multiplier', async function () { + fn.onCall(0).throws(Error('0')) // 0ms + fn.onCall(1).throws(Error('1')) // 100ms + fn.onCall(2).throws(Error('2')) // 200ms + fn.onCall(3).resolves('success') // 400ms + + // Note 701 instead of 700 for timeout. The 1 millisecond allows the final call to execute + // since the timeout condition is >= instead of > + const res = waitUntil(fn, { timeout: 701, interval: 100, backoff: 2, retryOnFail: true }) + + // Check the call count after each iteration, ensuring the function is called + // after the correct delay between retries. + await clock.tickAsync(99) + assert.strictEqual(fn.callCount, 1) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 2) + + await clock.tickAsync(199) + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 3) + + await clock.tickAsync(399) + assert.strictEqual(fn.callCount, 3) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 4) + + assert.strictEqual(await res, 'success') + }) + }) + describe('waitTimeout', async function () { async function testFunction(delay: number = 500, error?: Error) { await sleep(delay) From 7bee61eade20b64442346b7bc1b53c63fdc26572 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Tue, 21 Jan 2025 11:46:03 -0500 Subject: [PATCH 2/4] fix docstrings Signed-off-by: nkomonen-amazon --- .../core/src/shared/utilities/timeoutUtils.ts | 15 +++++++-------- .../test/shared/utilities/timeoutUtils.test.ts | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 8333d1bb804..7c1a6e521ca 100644 --- a/packages/core/src/shared/utilities/timeoutUtils.ts +++ b/packages/core/src/shared/utilities/timeoutUtils.ts @@ -222,9 +222,9 @@ interface WaitUntilOptions { /** A backoff multiplier for how long the next interval will be (default: None, i.e 1) */ readonly backoff?: number /** - * Only retries when an error is thrown, otherwise returning the result regardless of truthiness. - * - Ignores 'truthy' arg - * - If the timeout is reached it will throw the last error + * Only retries when an error is thrown, otherwise returning the immediate result. + * - 'truthy' arg is ignored + * - If the timeout is reached it throws the last error * - default: false */ readonly retryOnFail?: boolean @@ -234,14 +234,13 @@ export const waitUntilDefaultTimeout = 2000 export const waitUntilDefaultInterval = 500 /** - * Invokes `fn()` until it returns a truthy value (or non-undefined if `truthy:false`). - * - * Also look at {@link withRetries} to be able to retry on failures. + * Invokes `fn()` on an interval based on the given arguments. This can be used for retries, or until + * an expected result is given. Read {@link WaitUntilOptions} carefully. * * @param fn Function whose result is checked * @param options See {@link WaitUntilOptions} * - * @returns Result of `fn()`, or `undefined` if timeout was reached. + * @returns Result of `fn()`, or possibly `undefined` depending on the arguments. */ export async function waitUntil(fn: () => Promise, options: WaitUntilOptions & { retryOnFail: true }): Promise export async function waitUntil( @@ -308,7 +307,7 @@ export async function waitUntil(fn: () => Promise, options: WaitUntilOptio throw lastError } - // when testing, this saves the need to progress the stubbed clock + // when testing, this avoids the need to progress the stubbed clock if (interval > 0) { await sleep(interval) } diff --git a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts index 5409e617d75..96a4a5bdcbb 100644 --- a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts +++ b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts @@ -388,7 +388,7 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { }) describe('waitUntil w/ retries', function () { - let fn: SinonStub + let fn: SinonStub<[], Promise> beforeEach(function () { fn = sandbox.stub() From ecb2768a210d72a508dc45e566202d52cae39bb2 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Wed, 22 Jan 2025 11:42:38 -0500 Subject: [PATCH 3/4] fix rejected promise Signed-off-by: nkomonen-amazon --- .../core/src/test/shared/utilities/timeoutUtils.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts index 96a4a5bdcbb..3ee5968132b 100644 --- a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts +++ b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts @@ -423,7 +423,12 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { fn.onCall(3).throws(new Error('last')) // 1500 fn.onCall(4).resolves('this is not hit') - const res = waitUntil(fn, { retryOnFail: true }) + // We must wrap w/ assert.rejects() here instead of at the end, otherwise Mocha raise a + // `rejected promise not handled within 1 second: Error: last` + const res = assert.rejects( + waitUntil(fn, { retryOnFail: true }), + (e) => e instanceof Error && e.message === 'last' + ) await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 500 assert.strictEqual(fn.callCount, 2) @@ -432,7 +437,7 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 1500 assert.strictEqual(fn.callCount, 4) - await assert.rejects(res, (e) => e instanceof Error && e.message === 'last') + await res }) it('honors retry delay + backoff multiplier', async function () { From 2266430d59f5f44428f232debe2cfb6cefd6e733 Mon Sep 17 00:00:00 2001 From: nkomonen-amazon Date: Wed, 22 Jan 2025 14:15:32 -0500 Subject: [PATCH 4/4] fix another unhandled promise Signed-off-by: nkomonen-amazon --- packages/core/src/test/notifications/controller.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts index e7b62a518e5..761c6ce4bf2 100644 --- a/packages/core/src/test/notifications/controller.test.ts +++ b/packages/core/src/test/notifications/controller.test.ts @@ -535,7 +535,9 @@ describe('RemoteFetcher', function () { httpStub.throws(new Error('network error')) // Start function under test - const fetcher = new RemoteFetcher().fetch('startUp', 'any').then(() => assert.fail('Did not throw exception.')) + const fetcher = assert.rejects(new RemoteFetcher().fetch('startUp', 'any'), (e) => { + return e instanceof Error && e.message === 'last error' + }) // Progresses the clock, allowing the fetcher logic to break out of sleep for each iteration of withRetries() assert.strictEqual(httpStub.callCount, 1) // 0 @@ -550,9 +552,7 @@ describe('RemoteFetcher', function () { assert.strictEqual(httpStub.callCount, 5) // 150_000 // We hit timeout so the last error will be thrown - await assert.rejects(fetcher, (e) => { - return e instanceof Error && e.message === 'last error' - }) + await fetcher }) })