Fix flaky tests on TransactionExecutor suite (#1137)

bigmontz · thelonelyvulpes · robsdedude · web-flow · commit cf1fde820026 · 2023-09-07T14:27:28.000+02:00
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.

Co-authored-by: grant lodge &lt;6323995+thelonelyvulpes@users.noreply.github.com&gt;
Co-authored-by: Robsdedude &lt;dev@rouvenbauer.de&gt;
diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts
@@ -32,20 +32,41 @@ type TransactionWork<T, Tx = Transaction> = (tx: Tx) => T | Promise<T>
 type Resolve<T> = (value: T | PromiseLike<T>) => void
 type Reject = (value: any) => void
 type Timeout = ReturnType<typeof setTimeout>
+type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout
+type ClearTimeout = (timeoutId: Timeout) => void
+
+interface Dependencies {
+  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 {
   private readonly _maxRetryTimeMs: number
   private readonly _initialRetryDelayMs: number
   private readonly _multiplier: number
   private readonly _jitterFactor: number
   private _inFlightTimeoutIds: Timeout[]
+  private readonly _setTimeout: SetTimeout
+  private readonly _clearTimeout: ClearTimeout
   public pipelineBegin: boolean
 
   constructor (
     maxRetryTimeMs?: number | null,
     initialRetryDelayMs?: number,
     multiplier?: number,
-    jitterFactor?: number
+    jitterFactor?: number,
+    dependencies: Dependencies = {
+      setTimeout: setTimeoutWrapper,
+      clearTimeout: clearTimeoutWrapper
+    }
   ) {
     this._maxRetryTimeMs = _valueOrDefault(
       maxRetryTimeMs,
@@ -64,6 +85,9 @@ export class TransactionExecutor {
       DEFAULT_RETRY_DELAY_JITTER_FACTOR
     )
 
+    this._setTimeout = dependencies.setTimeout
+    this._clearTimeout = dependencies.clearTimeout
+
     this._inFlightTimeoutIds = []
     this.pipelineBegin = false
 
@@ -99,7 +123,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 +143,7 @@ export class TransactionExecutor {
 
     return new Promise<T>((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
@@ -32,20 +32,41 @@ type TransactionWork<T, Tx = Transaction> = (tx: Tx) => T | Promise<T>
 type Resolve<T> = (value: T | PromiseLike<T>) => void
 type Reject = (value: any) => void
 type Timeout = ReturnType<typeof setTimeout>
+type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout
+type ClearTimeout = (timeoutId: Timeout) => void
+
+interface Dependencies {
+  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 {
   private readonly _maxRetryTimeMs: number
   private readonly _initialRetryDelayMs: number
   private readonly _multiplier: number
   private readonly _jitterFactor: number
   private _inFlightTimeoutIds: Timeout[]
+  private readonly _setTimeout: SetTimeout
+  private readonly _clearTimeout: ClearTimeout
   public pipelineBegin: boolean
 
   constructor (
     maxRetryTimeMs?: number | null,
     initialRetryDelayMs?: number,
     multiplier?: number,
-    jitterFactor?: number
+    jitterFactor?: number,
+    dependencies: Dependencies = {
+      setTimeout: setTimeoutWrapper,
+      clearTimeout: clearTimeoutWrapper
+    }
   ) {
     this._maxRetryTimeMs = _valueOrDefault(
       maxRetryTimeMs,
@@ -64,6 +85,9 @@ export class TransactionExecutor {
       DEFAULT_RETRY_DELAY_JITTER_FACTOR
     )
 
+    this._setTimeout = dependencies.setTimeout
+    this._clearTimeout = dependencies.clearTimeout
+
     this._inFlightTimeoutIds = []
     this.pipelineBegin = false
 
@@ -99,7 +123,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 +143,7 @@ export class TransactionExecutor {
 
     return new Promise<T>((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
@@ -15,53 +15,46 @@
  * 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 lighter 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) {
+    const timeoutId = this._timeoutIdCounter++
+    this.invocationDelays.push(delay)
+    if (!this._timeoutCallbacksDisabled) {
+      code()
     }
-
-    return this
-  }
-
-  pause () {
-    this._paused = true
+    return timeoutId
   }
 
-  uninstall () {
-    global.setTimeout = this._originalSetTimeout
-    global.clearTimeout = this._originalClearTimeout
-    this._clearState()
+  clearTimeout (id) {
+    this.clearedTimeouts.push(id)
   }
 
-  setTimeoutOriginal (code, delay) {
-    return this._originalSetTimeout.call(null, code, delay)
+  disableTimeoutCallbacks () {
+    this._timeoutCallbacksDisabled = true
   }
 
-  _clearState () {
-    this._originalSetTimeout = null
-    this._originalClearTimeout = null
-    this._paused = false
+  clearState () {
+    this._timeoutCallbacksDisabled = false
     this._timeoutIdCounter = 0
 
     this.invocationDelays = []
     this.clearedTimeouts = []
   }
 }
 
-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
