Add timeout to wait-until-called (#9996)

The `waitUntilCalled` utility now has a timeout. It will now throw an
error if the stub is not called enough times, rather than blocking
forever.

The return type had to be changed to a function, so that we could throw
when the timeout is triggered. I tried returning an error that rejected
first, but if you don't handle the error synchronously Node.js will
consider it to be an unhandled Promise rejected (even if it _is_
handled later on).

I worked around this by resolving in the timeout case as well, so that
there is never a "deferred" Promise exception in the timeout case. The
returned function re-throws the error if it's given. That way there is
never any unhandled Promise rejection.

test/e2e/metrics.spec.js

@@ -28,14 +28,16 @@ describe('Segment metrics', function () {
         mockSegment: true,
       },
       async ({ driver, segmentStub }) => {
-        const threeSegmentEventsReceived = waitUntilCalled(segmentStub, null, 3)
+        const threeSegmentEventsReceived = waitUntilCalled(segmentStub, null, {
+          callCount: 3,
+        })
         await driver.navigate()
 
         const passwordField = await driver.findElement(By.css('#password'))
         await passwordField.sendKeys('correct horse battery staple')
         await passwordField.sendKeys(Key.ENTER)
 
-        await threeSegmentEventsReceived
+        await threeSegmentEventsReceived()
 
         assert.ok(segmentStub.called, 'Segment should receive metrics')
 

test/lib/wait-until-called.js

@@ -1,22 +1,41 @@
+const DEFAULT_TIMEOUT = 10000
+
 /**
- * A function that wraps a sinon stubbed function and returns a Promise
- * when this stub was called.
+ * A function that wraps a sinon stub and returns an asynchronous function
+ * that resolves if the stubbed function was called enough times, or throws
+ * if the timeout is exceeded.
  *
  * The stub that has been passed in will be setup to call the wrapped function
- * directly, then trigger the returned Promise to resolve.
+ * directly.
  *
  * WARNING: Any existing `callsFake` behavior will be overwritten.
  *
  * @param {import('sinon').stub} stub - A sinon stub of a function
- * @param {unknown} [wrappedThis] - The object the stubbed function was called on, if any (i.e. the `this` value)
- * @param {number} [callCount] - The number of calls to wait for. Defaults to 1.
- * @returns {Promise} A Promise that resolves when the stub has been called
+ * @param {unknown} [wrappedThis] - The object the stubbed function was called
+ *   on, if any (i.e. the `this` value)
+ * @param {Object} [options] - Optional configuration
+ * @param {number} [options.callCount] - The number of calls to wait for.
+ * @param {number|null} [options.timeout] - The timeout, in milliseconds. Pass
+ *   in `null` to disable the timeout.
+ * @returns {Function} An asynchronous function that resolves when the stub is
+ *   called enough times, or throws if the timeout is reached.
  */
-function waitUntilCalled(stub, wrappedThis = null, callCount = 1) {
+function waitUntilCalled(
+  stub,
+  wrappedThis = null,
+  { callCount = 1, timeout = DEFAULT_TIMEOUT } = {},
+) {
   let numCalls = 0
   let resolve
+  let timeoutHandle
   const stubHasBeenCalled = new Promise((_resolve) => {
     resolve = _resolve
+    if (timeout !== null) {
+      timeoutHandle = setTimeout(
+        () => resolve(new Error('Timeout exceeded')),
+        timeout,
+      )
+    }
   })
   stub.callsFake((...args) => {
     try {
@@ -27,12 +46,21 @@ function waitUntilCalled(stub, wrappedThis = null, callCount = 1) {
       if (numCalls < callCount) {
         numCalls += 1
         if (numCalls === callCount) {
+          if (timeoutHandle) {
+            clearTimeout(timeoutHandle)
+          }
           resolve()
         }
       }
     }
   })
-  return stubHasBeenCalled
+
+  return async () => {
+    const error = await stubHasBeenCalled
+    if (error) {
+      throw error
+    }
+  }
 }
 
 module.exports = waitUntilCalled

test/unit/app/controllers/incoming-transactions-test.js

@@ -249,7 +249,7 @@ describe('IncomingTransactionsController', function () {
       )
 
       incomingTransactionsController.start()
-      await updateStateCalled
+      await updateStateCalled()
 
       const actualState = incomingTransactionsController.store.getState()
       const generatedTxId = actualState?.incomingTransactions?.['0xfake']?.id
@@ -345,8 +345,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),
@@ -412,8 +412,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),
@@ -475,8 +475,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),
@@ -540,8 +540,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),
@@ -592,7 +592,7 @@ describe('IncomingTransactionsController', function () {
       // TODO: stop skipping the first event
       await subscription({ selectedAddress: MOCK_SELECTED_ADDRESS })
       await subscription({ selectedAddress: NEW_MOCK_SELECTED_ADDRESS })
-      await updateStateCalled
+      await updateStateCalled()
 
       const actualState = incomingTransactionsController.store.getState()
       const generatedTxId = actualState?.incomingTransactions?.['0xfake']?.id
@@ -696,8 +696,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),
@@ -746,7 +746,7 @@ describe('IncomingTransactionsController', function () {
         ROPSTEN_CHAIN_ID,
       )
       await subscription(ROPSTEN)
-      await updateStateCalled
+      await updateStateCalled()
 
       const actualState = incomingTransactionsController.store.getState()
       const generatedTxId = actualState?.incomingTransactions?.['0xfake']?.id
@@ -848,8 +848,8 @@ describe('IncomingTransactionsController', function () {
 
       try {
         await Promise.race([
-          updateStateCalled,
-          putStateCalled,
+          updateStateCalled(),
+          putStateCalled(),
           new Promise((_, reject) => {
             setTimeout(() => reject(new Error('TIMEOUT')), SET_STATE_TIMEOUT)
           }),

test/unit/app/controllers/metametrics-test.js

@@ -400,7 +400,7 @@ describe('MetaMetricsController', function () {
         },
         { flushImmediately: true },
       )
-      assert.doesNotReject(flushCalled)
+      assert.doesNotReject(flushCalled())
     })
 
     it('should throw if event or category not provided', function () {