From 3b3827c228a032030f3889d9ee1ec00f35592402 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 6 Sep 2023 12:23:41 +0200 Subject: [PATCH 1/5] Fix flaky tests on TransactionExecutor suite The TransactionExecutor tests were mocking the global setTimeout and clearTimeout. This mocks were conflicting with other calls to this api in the browser tests. So, the calls to the set/clear were not being the expected in some situations. Replace the global mock for injecting the functions resolve this issue in a less intrusive way. --- .../core/src/internal/transaction-executor.ts | 20 +- .../lib/core/internal/transaction-executor.ts | 20 +- .../neo4j-driver/test/internal/timers-util.js | 52 ++- .../internal/transaction-executor.test.js | 340 +++++++++--------- 4 files changed, 223 insertions(+), 209 deletions(-) diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts index ae9eac969..12737bff5 100644 --- a/packages/core/src/internal/transaction-executor.ts +++ b/packages/core/src/internal/transaction-executor.ts @@ -33,19 +33,30 @@ type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +interface Dependencies { + setTimeout: typeof setTimeout + clearTimeout: typeof clearTimeout +} + export class TransactionExecutor { private readonly _maxRetryTimeMs: number private readonly _initialRetryDelayMs: number private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] + private readonly _setTimeout: typeof setTimeout + private readonly _clearTimeout: typeof clearTimeout public pipelineBegin: boolean constructor ( maxRetryTimeMs?: number | null, initialRetryDelayMs?: number, multiplier?: number, - jitterFactor?: number + jitterFactor?: number, + dependencies: Dependencies = { + setTimeout, + clearTimeout + } ) { this._maxRetryTimeMs = _valueOrDefault( maxRetryTimeMs, @@ -64,6 +75,9 @@ export class TransactionExecutor { DEFAULT_RETRY_DELAY_JITTER_FACTOR ) + this._setTimeout = dependencies.setTimeout + this._clearTimeout = dependencies.clearTimeout + this._inFlightTimeoutIds = [] this.pipelineBegin = false @@ -99,7 +113,7 @@ export class TransactionExecutor { close (): void { // cancel all existing timeouts to prevent further retries - this._inFlightTimeoutIds.forEach(timeoutId => clearTimeout(timeoutId)) + this._inFlightTimeoutIds.forEach(timeoutId => this._clearTimeout(timeoutId)) this._inFlightTimeoutIds = [] } @@ -119,7 +133,7 @@ export class TransactionExecutor { return new Promise((resolve, reject) => { const nextRetryTime = this._computeDelayWithJitter(retryDelayMs) - const timeoutId = setTimeout(() => { + const timeoutId = this._setTimeout(() => { // filter out this timeoutId when time has come and function is being executed this._inFlightTimeoutIds = this._inFlightTimeoutIds.filter( id => id !== timeoutId diff --git a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts index e95ce3a9a..e50a19505 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts @@ -33,19 +33,30 @@ type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +interface Dependencies { + setTimeout: typeof setTimeout + clearTimeout: typeof clearTimeout +} + export class TransactionExecutor { private readonly _maxRetryTimeMs: number private readonly _initialRetryDelayMs: number private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] + private readonly _setTimeout: typeof setTimeout + private readonly _clearTimeout: typeof clearTimeout public pipelineBegin: boolean constructor ( maxRetryTimeMs?: number | null, initialRetryDelayMs?: number, multiplier?: number, - jitterFactor?: number + jitterFactor?: number, + dependencies: Dependencies = { + setTimeout, + clearTimeout + } ) { this._maxRetryTimeMs = _valueOrDefault( maxRetryTimeMs, @@ -64,6 +75,9 @@ export class TransactionExecutor { DEFAULT_RETRY_DELAY_JITTER_FACTOR ) + this._setTimeout = dependencies.setTimeout + this._clearTimeout = dependencies.clearTimeout + this._inFlightTimeoutIds = [] this.pipelineBegin = false @@ -99,7 +113,7 @@ export class TransactionExecutor { close (): void { // cancel all existing timeouts to prevent further retries - this._inFlightTimeoutIds.forEach(timeoutId => clearTimeout(timeoutId)) + this._inFlightTimeoutIds.forEach(timeoutId => this._clearTimeout(timeoutId)) this._inFlightTimeoutIds = [] } @@ -119,7 +133,7 @@ export class TransactionExecutor { return new Promise((resolve, reject) => { const nextRetryTime = this._computeDelayWithJitter(retryDelayMs) - const timeoutId = setTimeout(() => { + const timeoutId = this._setTimeout(() => { // filter out this timeoutId when time has come and function is being executed this._inFlightTimeoutIds = this._inFlightTimeoutIds.filter( id => id !== timeoutId diff --git a/packages/neo4j-driver/test/internal/timers-util.js b/packages/neo4j-driver/test/internal/timers-util.js index 01ad01742..daf79bded 100644 --- a/packages/neo4j-driver/test/internal/timers-util.js +++ b/packages/neo4j-driver/test/internal/timers-util.js @@ -15,47 +15,37 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. +*/ + +/** + * This is a liter mock which only creates mocked functions to work + * as timeouts. */ -class SetTimeoutMock { +class TimeoutsMock { constructor () { - this._clearState() + this.clearState() + // bind it to be used as standalone functions + this.setTimeout = this.setTimeout.bind(this) + this.clearTimeout = this.clearTimeout.bind(this) } - install () { - this._originalSetTimeout = global.setTimeout - global.setTimeout = (code, delay) => { - if (!this._paused) { - code() - this.invocationDelays.push(delay) - } - return this._timeoutIdCounter++ - } - - this._originalClearTimeout = global.clearTimeout - global.clearTimeout = id => { - this.clearedTimeouts.push(id) + setTimeout (code, delay) { + if (!this._paused) { + code() + this.invocationDelays.push(delay) } + return this._timeoutIdCounter++ + } - return this + clearTimeout (id) { + this.clearedTimeouts.push(id) } pause () { this._paused = true } - uninstall () { - global.setTimeout = this._originalSetTimeout - global.clearTimeout = this._originalClearTimeout - this._clearState() - } - - setTimeoutOriginal (code, delay) { - return this._originalSetTimeout.call(null, code, delay) - } - - _clearState () { - this._originalSetTimeout = null - this._originalClearTimeout = null + clearState () { this._paused = false this._timeoutIdCounter = 0 @@ -64,4 +54,6 @@ class SetTimeoutMock { } } -export const setTimeoutMock = new SetTimeoutMock() +export { + TimeoutsMock +} diff --git a/packages/neo4j-driver/test/internal/transaction-executor.test.js b/packages/neo4j-driver/test/internal/transaction-executor.test.js index e0c10ba48..c58feb930 100644 --- a/packages/neo4j-driver/test/internal/transaction-executor.test.js +++ b/packages/neo4j-driver/test/internal/transaction-executor.test.js @@ -18,7 +18,7 @@ */ import { newError, error as err, internal } from 'neo4j-driver-core' -import { setTimeoutMock } from './timers-util' +import { TimeoutsMock } from './timers-util' import lolex from 'lolex' const { @@ -89,36 +89,31 @@ describe('#unit TransactionExecutor', () => { }, 60000) it('should cancel in-flight timeouts when closed', async () => { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - // do not execute setTimeout callbacks - fakeSetTimeout.pause() + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + // do not execute setTimeout callbacks + fakeSetTimeout.pause() - executor.execute(transactionCreator([SERVICE_UNAVAILABLE]), () => - Promise.resolve(42) - ) - executor.execute(transactionCreator([TRANSIENT_ERROR_1]), () => - Promise.resolve(4242) - ) - executor.execute(transactionCreator([SESSION_EXPIRED]), () => - Promise.resolve(424242) - ) + executor.execute(transactionCreator([SERVICE_UNAVAILABLE]), () => + Promise.resolve(42) + ) + executor.execute(transactionCreator([TRANSIENT_ERROR_1]), () => + Promise.resolve(4242) + ) + executor.execute(transactionCreator([SESSION_EXPIRED]), () => + Promise.resolve(424242) + ) - await new Promise((resolve, reject) => { - fakeSetTimeout.setTimeoutOriginal(() => { - try { - executor.close() - expect(fakeSetTimeout.clearedTimeouts.length).toEqual(3) - resolve() - } catch (error) { - reject(error) - } - }, 1000) - }) - } finally { - fakeSetTimeout.uninstall() - } + await new Promise((resolve, reject) => { + setTimeout(() => { + try { + executor.close() + expect(fakeSetTimeout.clearedTimeouts.length).toEqual(3) + resolve() + } catch (error) { + reject(error) + } + }, 1000) + }) }, 60000) }) @@ -320,178 +315,177 @@ describe('#unit TransactionExecutor', () => { }) async function testRetryWhenTransactionCreatorFails (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const transactionCreator = throwingTransactionCreator( - errorCodes, - new FakeTransaction() - ) - const usedTransactions = [] - - const result = await executor.execute(transactionCreator, tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return Promise.resolve(42) - }) - - expect(usedTransactions.length).toEqual(1) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const transactionCreator = throwingTransactionCreator( + errorCodes, + new FakeTransaction() + ) + const usedTransactions = [] + + const result = await executor.execute(transactionCreator, tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return Promise.resolve(42) + }) + + expect(usedTransactions.length).toEqual(1) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionBeginFails (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const transactionCreator = throwingTransactionCreatorOnBegin( - errorCodes, - new FakeTransaction() - ) - const usedTransactions = [] - const beginTransactions = [] - - const result = await executor.execute(transactionCreator, async tx => { - expect(tx).toBeDefined() - beginTransactions.push(tx) + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const transactionCreator = throwingTransactionCreatorOnBegin( + errorCodes, + new FakeTransaction() + ) + const usedTransactions = [] + const beginTransactions = [] + + const result = await executor.execute(transactionCreator, async tx => { + expect(tx).toBeDefined() + beginTransactions.push(tx) - if (pipelineBegin) { - // forcing await for tx since pipeline doesn't wait for begin return - await tx - } + if (pipelineBegin) { + // forcing await for tx since pipeline doesn't wait for begin return + await tx + } - usedTransactions.push(tx) - return Promise.resolve(42) - }) - - expect(beginTransactions.length).toEqual(pipelineBegin ? errorCodes.length + 1 : 1) - expect(usedTransactions.length).toEqual(1) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + usedTransactions.push(tx) + return Promise.resolve(42) + }) + + expect(beginTransactions.length).toEqual(pipelineBegin ? errorCodes.length + 1 : 1) + expect(usedTransactions.length).toEqual(1) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionWorkReturnsRejectedPromise ( errorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = transactionWork(errorCodes, 42) - - const result = await executor.execute(transactionCreator(), tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - }) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = transactionWork(errorCodes, 42) + + const result = await executor.execute(transactionCreator(), tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return realWork() + }) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionCommitReturnsRejectedPromise ( errorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = () => Promise.resolve(4242) - - const result = await executor.execute( - transactionCreator(errorCodes), - tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - } - ) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(4242) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } - } - - async function testRetryWhenTransactionWorkThrows (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = throwingTransactionWork(errorCodes, 42) - - const result = await executor.execute(transactionCreator(), async tx => { + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = () => Promise.resolve(4242) + + const result = await executor.execute( + transactionCreator(errorCodes), + tx => { expect(tx).toBeDefined() usedTransactions.push(tx) - if (pipelineBegin) { - await tx - } return realWork() - }) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + } + ) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(4242) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) + } + + async function testRetryWhenTransactionWorkThrows (errorCodes) { + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = throwingTransactionWork(errorCodes, 42) + + const result = await executor.execute(transactionCreator(), async tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + if (pipelineBegin) { + await tx + } + return realWork() + }) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionWorkThrowsAndRollbackFails ( txWorkErrorCodes, rollbackErrorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = throwingTransactionWork(txWorkErrorCodes, 424242) - - const result = await executor.execute( - transactionCreator([], rollbackErrorCodes), - tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - } - ) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(txWorkErrorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(424242) - verifyRetryDelays(fakeSetTimeout, txWorkErrorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = throwingTransactionWork(txWorkErrorCodes, 424242) + + const result = await executor.execute( + transactionCreator([], rollbackErrorCodes), + tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return realWork() + } + ) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(txWorkErrorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(424242) + verifyRetryDelays(fakeSetTimeout, txWorkErrorCodes.length) } }) }) +function createTransactionExecutorWithFakeTimeout (...args) { + const fakeSetTimeout = new TimeoutsMock() + const dependencies = { + setTimeout: fakeSetTimeout.setTimeout, + clearTimeout: fakeSetTimeout.clearTimeout + } + + // args should have at least 5 elements for place the dependencies + if (args.length < 5) { + args.length = 5 + } + + // dependencies not set, so we should set + if (args[4] === undefined) { + args[4] = dependencies + // dependencies are an object, so we should join both + } else if (typeof args[4] === 'object') { + args[4] = { ...dependencies, ...args[4] } + } else { + throw new TypeError( + `Expected object or undefined as args[4] but got ${typeof args[4]}`) + } + + return { + executor: new TransactionExecutor(...args), + fakeSetTimeout + } +} + async function testNoRetryOnUnknownError ( errorCodes, expectedWorkInvocationCount From 920d7d67913033246dfaae0a8026554aa31fa6c2 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 6 Sep 2023 13:06:26 +0200 Subject: [PATCH 2/5] Not changing the this on setTimeout and clearTimeout since in browser the two methods depends on window to be this --- .../core/src/internal/transaction-executor.ts | 22 ++++++++++++++----- .../lib/core/internal/transaction-executor.ts | 22 ++++++++++++++----- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts index 12737bff5..12ec9a13e 100644 --- a/packages/core/src/internal/transaction-executor.ts +++ b/packages/core/src/internal/transaction-executor.ts @@ -32,10 +32,20 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +type SetTimeout = (...args: Parameters) => Timeout +type ClearTimeout = (timeoutId: Timeout) => void interface Dependencies { - setTimeout: typeof setTimeout - clearTimeout: typeof clearTimeout + setTimeout: SetTimeout + clearTimeout: ClearTimeout +} + +function setTimeoutWrapper (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { + return setTimeout(callback, ms, ...args) +} + +function clearTimeoutWrapper (timeoutId: Timeout): void { + return clearTimeout(timeoutId) } export class TransactionExecutor { @@ -44,8 +54,8 @@ export class TransactionExecutor { private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] - private readonly _setTimeout: typeof setTimeout - private readonly _clearTimeout: typeof clearTimeout + private readonly _setTimeout: SetTimeout + private readonly _clearTimeout: ClearTimeout public pipelineBegin: boolean constructor ( @@ -54,8 +64,8 @@ export class TransactionExecutor { multiplier?: number, jitterFactor?: number, dependencies: Dependencies = { - setTimeout, - clearTimeout + setTimeout: setTimeoutWrapper, + clearTimeout: clearTimeoutWrapper } ) { this._maxRetryTimeMs = _valueOrDefault( diff --git a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts index e50a19505..b4e454874 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts @@ -32,10 +32,20 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +type SetTimeout = (...args: Parameters) => Timeout +type ClearTimeout = (timeoutId: Timeout) => void interface Dependencies { - setTimeout: typeof setTimeout - clearTimeout: typeof clearTimeout + setTimeout: SetTimeout + clearTimeout: ClearTimeout +} + +function setTimeoutWrapper (callback: (...args:unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { + return setTimeout(callback, ms, ...args) +} + +function clearTimeoutWrapper (timeoutId: Timeout): void { + return clearTimeout(timeoutId) } export class TransactionExecutor { @@ -44,8 +54,8 @@ export class TransactionExecutor { private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] - private readonly _setTimeout: typeof setTimeout - private readonly _clearTimeout: typeof clearTimeout + private readonly _setTimeout: SetTimeout + private readonly _clearTimeout: ClearTimeout public pipelineBegin: boolean constructor ( @@ -54,8 +64,8 @@ export class TransactionExecutor { multiplier?: number, jitterFactor?: number, dependencies: Dependencies = { - setTimeout, - clearTimeout + setTimeout: setTimeoutWrapper, + clearTimeout: clearTimeoutWrapper } ) { this._maxRetryTimeMs = _valueOrDefault( From fe0e52b74e782b5b498c5477c843ec8a272a055f Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 6 Sep 2023 13:11:59 +0200 Subject: [PATCH 3/5] Fix setTimeout type --- packages/core/src/internal/transaction-executor.ts | 2 +- .../lib/core/internal/transaction-executor.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts index 12ec9a13e..e1a7979f4 100644 --- a/packages/core/src/internal/transaction-executor.ts +++ b/packages/core/src/internal/transaction-executor.ts @@ -32,7 +32,7 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType -type SetTimeout = (...args: Parameters) => Timeout +type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout type ClearTimeout = (timeoutId: Timeout) => void interface Dependencies { diff --git a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts index b4e454874..27b055ba3 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts @@ -32,7 +32,7 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType -type SetTimeout = (...args: Parameters) => Timeout +type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout type ClearTimeout = (timeoutId: Timeout) => void interface Dependencies { @@ -40,7 +40,7 @@ interface Dependencies { clearTimeout: ClearTimeout } -function setTimeoutWrapper (callback: (...args:unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { +function setTimeoutWrapper (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { return setTimeout(callback, ms, ...args) } From 06ebfb59c26c2b08ffbed74973413f1540bdb49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Thu, 7 Sep 2023 11:36:45 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: grant lodge <6323995+thelonelyvulpes@users.noreply.github.com> Co-authored-by: Robsdedude --- packages/neo4j-driver/test/internal/timers-util.js | 7 ++++--- .../test/internal/transaction-executor.test.js | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/neo4j-driver/test/internal/timers-util.js b/packages/neo4j-driver/test/internal/timers-util.js index daf79bded..b47d7a19b 100644 --- a/packages/neo4j-driver/test/internal/timers-util.js +++ b/packages/neo4j-driver/test/internal/timers-util.js @@ -18,7 +18,7 @@ */ /** - * This is a liter mock which only creates mocked functions to work + * This is a lighter mock which only creates mocked functions to work * as timeouts. */ class TimeoutsMock { @@ -30,11 +30,12 @@ class TimeoutsMock { } setTimeout (code, delay) { + let timeoutId = this._timeoutIdCounter++ + this.invocationDelays.push(delay) if (!this._paused) { code() - this.invocationDelays.push(delay) } - return this._timeoutIdCounter++ + return timeoutId } clearTimeout (id) { diff --git a/packages/neo4j-driver/test/internal/transaction-executor.test.js b/packages/neo4j-driver/test/internal/transaction-executor.test.js index c58feb930..6a56a1350 100644 --- a/packages/neo4j-driver/test/internal/transaction-executor.test.js +++ b/packages/neo4j-driver/test/internal/transaction-executor.test.js @@ -477,7 +477,7 @@ function createTransactionExecutorWithFakeTimeout (...args) { args[4] = { ...dependencies, ...args[4] } } else { throw new TypeError( - `Expected object or undefined as args[4] but got ${typeof args[4]}`) + `Expected object or undefined as args[4] but got ${typeof args[4]}`) } return { From f3ca653342a3b02a7da03590b4280a4f4db168c6 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 7 Sep 2023 11:50:48 +0200 Subject: [PATCH 5/5] Addressing comments in the PR --- packages/neo4j-driver/test/internal/timers-util.js | 10 +++++----- .../test/internal/transaction-executor.test.js | 13 ++----------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/packages/neo4j-driver/test/internal/timers-util.js b/packages/neo4j-driver/test/internal/timers-util.js index b47d7a19b..2d1ee8708 100644 --- a/packages/neo4j-driver/test/internal/timers-util.js +++ b/packages/neo4j-driver/test/internal/timers-util.js @@ -30,9 +30,9 @@ class TimeoutsMock { } setTimeout (code, delay) { - let timeoutId = this._timeoutIdCounter++ + const timeoutId = this._timeoutIdCounter++ this.invocationDelays.push(delay) - if (!this._paused) { + if (!this._timeoutCallbacksDisabled) { code() } return timeoutId @@ -42,12 +42,12 @@ class TimeoutsMock { this.clearedTimeouts.push(id) } - pause () { - this._paused = true + disableTimeoutCallbacks () { + this._timeoutCallbacksDisabled = true } clearState () { - this._paused = false + this._timeoutCallbacksDisabled = false this._timeoutIdCounter = 0 this.invocationDelays = [] diff --git a/packages/neo4j-driver/test/internal/transaction-executor.test.js b/packages/neo4j-driver/test/internal/transaction-executor.test.js index 6a56a1350..60763e8ae 100644 --- a/packages/neo4j-driver/test/internal/transaction-executor.test.js +++ b/packages/neo4j-driver/test/internal/transaction-executor.test.js @@ -91,7 +91,7 @@ describe('#unit TransactionExecutor', () => { it('should cancel in-flight timeouts when closed', async () => { const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() // do not execute setTimeout callbacks - fakeSetTimeout.pause() + fakeSetTimeout.disableTimeoutCallbacks() executor.execute(transactionCreator([SERVICE_UNAVAILABLE]), () => Promise.resolve(42) @@ -464,16 +464,7 @@ function createTransactionExecutorWithFakeTimeout (...args) { clearTimeout: fakeSetTimeout.clearTimeout } - // args should have at least 5 elements for place the dependencies - if (args.length < 5) { - args.length = 5 - } - - // dependencies not set, so we should set - if (args[4] === undefined) { - args[4] = dependencies - // dependencies are an object, so we should join both - } else if (typeof args[4] === 'object') { + if (typeof args[4] === 'object' || args[4] === undefined) { args[4] = { ...dependencies, ...args[4] } } else { throw new TypeError(