Revert "[DI] Handle async errors in mocha tests (#4991)"

This reverts commit 1a95b0b.
DataDog · Dec 11, 2024 · 7f17ee0 · 7f17ee0
1 parent 3f20bfd
commit 7f17ee0
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 73 deletions.
diff --git a/integration-tests/debugger/basic.spec.js b/integration-tests/debugger/basic.spec.js
@@ -4,7 +4,7 @@ const os = require('os')
 
 const { assert } = require('chai')
 const { pollInterval, setup } = require('./utils')
-const { assertObjectContains, assertUUID, failOnException } = require('../helpers')
+const { assertObjectContains, assertUUID } = require('../helpers')
 const { ACKNOWLEDGED, ERROR } = require('../../packages/dd-trace/src/appsec/remote_config/apply_states')
 const { version } = require('../../package.json')
 
@@ -35,17 +35,17 @@ describe('Dynamic Instrumentation', function () {
         debugger: { diagnostics: { probeId, probeVersion: 0, status: 'EMITTING' } }
       }]
 
-      t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => {
+      t.agent.on('remote-config-ack-update', (id, version, state, error) => {
         assert.strictEqual(id, t.rcConfig.id)
         assert.strictEqual(version, 1)
         assert.strictEqual(state, ACKNOWLEDGED)
         assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail
 
         receivedAckUpdate = true
         endIfDone()
-      }))
+      })
 
-      t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
+      t.agent.on('debugger-diagnostics', ({ payload }) => {
         const expected = expectedPayloads.shift()
         assertObjectContains(payload, expected)
         assertUUID(payload.debugger.diagnostics.runtimeId)
@@ -60,7 +60,7 @@ describe('Dynamic Instrumentation', function () {
         } else {
           endIfDone()
         }
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
 
@@ -97,22 +97,22 @@ describe('Dynamic Instrumentation', function () {
         () => {}
       ]
 
-      t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => {
+      t.agent.on('remote-config-ack-update', (id, version, state, error) => {
         assert.strictEqual(id, t.rcConfig.id)
         assert.strictEqual(version, ++receivedAckUpdates)
         assert.strictEqual(state, ACKNOWLEDGED)
         assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail
 
         endIfDone()
-      }))
+      })
 
-      t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
+      t.agent.on('debugger-diagnostics', ({ payload }) => {
         const expected = expectedPayloads.shift()
         assertObjectContains(payload, expected)
         assertUUID(payload.debugger.diagnostics.runtimeId)
         if (payload.debugger.diagnostics.status === 'INSTALLED') triggers.shift()()
         endIfDone()
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
 
@@ -135,17 +135,17 @@ describe('Dynamic Instrumentation', function () {
         debugger: { diagnostics: { probeId, probeVersion: 0, status: 'INSTALLED' } }
       }]
 
-      t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => {
+      t.agent.on('remote-config-ack-update', (id, version, state, error) => {
         assert.strictEqual(id, t.rcConfig.id)
         assert.strictEqual(version, 1)
         assert.strictEqual(state, ACKNOWLEDGED)
         assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail
 
         receivedAckUpdate = true
         endIfDone()
-      }))
+      })
 
-      t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
+      t.agent.on('debugger-diagnostics', ({ payload }) => {
         const expected = expectedPayloads.shift()
         assertObjectContains(payload, expected)
         assertUUID(payload.debugger.diagnostics.runtimeId)
@@ -158,7 +158,7 @@ describe('Dynamic Instrumentation', function () {
             endIfDone()
           }, pollInterval * 2 * 1000) // wait twice as long as the RC poll interval
         }
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
 
@@ -183,15 +183,15 @@ describe('Dynamic Instrumentation', function () {
       it(title, function (done) {
         let receivedAckUpdate = false
 
-        t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => {
+        t.agent.on('remote-config-ack-update', (id, version, state, error) => {
           assert.strictEqual(id, `logProbe_${config.id}`)
           assert.strictEqual(version, 1)
           assert.strictEqual(state, ERROR)
           assert.strictEqual(error.slice(0, 6), 'Error:')
 
           receivedAckUpdate = true
           endIfDone()
-        }))
+        })
 
         const probeId = config.id
         const expectedPayloads = [{
@@ -201,10 +201,10 @@ describe('Dynamic Instrumentation', function () {
         }, {
           ddsource: 'dd_debugger',
           service: 'node',
-          debugger: { diagnostics: customErrorDiagnosticsObj ?? { probeId, probeVersion: 0, status: 'ERROR' } }
+          debugger: { diagnostics: customErrorDiagnosticsObj ?? { probeId, version: 0, status: 'ERROR' } }
         }]
 
-        t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
+        t.agent.on('debugger-diagnostics', ({ payload }) => {
           const expected = expectedPayloads.shift()
           assertObjectContains(payload, expected)
           const { diagnostics } = payload.debugger
@@ -218,7 +218,7 @@ describe('Dynamic Instrumentation', function () {
           }
 
           endIfDone()
-        }))
+        })
 
         t.agent.addRemoteConfig({
           product: 'LIVE_DEBUGGING',
@@ -237,7 +237,7 @@ describe('Dynamic Instrumentation', function () {
     it('should capture and send expected payload when a log line probe is triggered', function (done) {
       t.triggerBreakpoint()
 
-      t.agent.on('debugger-input', failOnException(done, ({ payload }) => {
+      t.agent.on('debugger-input', ({ payload }) => {
         const expected = {
           ddsource: 'dd_debugger',
           hostname: os.hostname(),
@@ -284,7 +284,7 @@ describe('Dynamic Instrumentation', function () {
         assert.strictEqual(topFrame.columnNumber, 3)
 
         done()
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
     })
@@ -307,31 +307,43 @@ describe('Dynamic Instrumentation', function () {
         if (payload.debugger.diagnostics.status === 'INSTALLED') triggers.shift()().catch(done)
       })
 
-      t.agent.on('debugger-input', failOnException(done, ({ payload }) => {
+      t.agent.on('debugger-input', ({ payload }) => {
         assert.strictEqual(payload.message, expectedMessages.shift())
         if (expectedMessages.length === 0) done()
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
     })
 
     it('should not trigger if probe is deleted', function (done) {
-      t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
-        if (payload.debugger.diagnostics.status === 'INSTALLED') {
-          t.agent.once('remote-confg-responded', failOnException(done, async () => {
-            await t.axios.get('/foo')
-            // We want to wait enough time to see if the client triggers on the breakpoint so that the test can fail
-            // if it does, but not so long that the test times out.
-            // TODO: Is there some signal we can use instead of a timer?
-            setTimeout(done, pollInterval * 2 * 1000) // wait twice as long as the RC poll interval
-          }))
+      t.agent.on('debugger-diagnostics', async ({ payload }) => {
+        try {
+          if (payload.debugger.diagnostics.status === 'INSTALLED') {
+            t.agent.once('remote-confg-responded', async () => {
+              try {
+                await t.axios.get('/foo')
+                // We want to wait enough time to see if the client triggers on the breakpoint so that the test can fail
+                // if it does, but not so long that the test times out.
+                // TODO: Is there some signal we can use instead of a timer?
+                setTimeout(done, pollInterval * 2 * 1000) // wait twice as long as the RC poll interval
+              } catch (err) {
+                // Nessecary hack: Any errors thrown inside of an async function is invisible to Mocha unless the outer
+                // `it` callback is also `async` (which we can't do in this case since we rely on the `done` callback).
+                done(err)
+              }
+            })
 
-          t.agent.removeRemoteConfig(t.rcConfig.id)
+            t.agent.removeRemoteConfig(t.rcConfig.id)
+          }
+        } catch (err) {
+          // Nessecary hack: Any errors thrown inside of an async function is invisible to Mocha unless the outer `it`
+          // callback is also `async` (which we can't do in this case since we rely on the `done` callback).
+          done(err)
         }
-      }))
+      })
 
       t.agent.on('debugger-input', () => {
-        done(new Error('should not capture anything when the probe is deleted'))
+        assert.fail('should not capture anything when the probe is deleted')
       })
 
       t.agent.addRemoteConfig(t.rcConfig)
@@ -342,8 +354,7 @@ describe('Dynamic Instrumentation', function () {
     it('should remove the last breakpoint completely before trying to add a new one', function (done) {
       const rcConfig2 = t.generateRemoteConfig()
 
-      t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => {
-        const { status, probeId } = payload.debugger.diagnostics
+      t.agent.on('debugger-diagnostics', ({ payload: { debugger: { diagnostics: { status, probeId } } } }) => {
         if (status !== 'INSTALLED') return
 
         if (probeId === t.rcConfig.config.id) {
@@ -376,7 +387,7 @@ describe('Dynamic Instrumentation', function () {
             if (!finished) done(err)
           })
         }
-      }))
+      })
 
       t.agent.addRemoteConfig(t.rcConfig)
     })

diff --git a/integration-tests/debugger/snapshot-pruning.spec.js b/integration-tests/debugger/snapshot-pruning.spec.js
@@ -2,7 +2,6 @@
 
 const { assert } = require('chai')
 const { setup, getBreakpointInfo } = require('./utils')
-const { failOnException } = require('../helpers')
 
 const { line } = getBreakpointInfo()
 
@@ -14,7 +13,7 @@ describe('Dynamic Instrumentation', function () {
       beforeEach(t.triggerBreakpoint)
 
       it('should prune snapshot if payload is too large', function (done) {
-        t.agent.on('debugger-input', failOnException(done, ({ payload }) => {
+        t.agent.on('debugger-input', ({ payload }) => {
           assert.isBelow(Buffer.byteLength(JSON.stringify(payload)), 1024 * 1024) // 1MB
           assert.deepEqual(payload['debugger.snapshot'].captures, {
             lines: {
@@ -27,7 +26,7 @@ describe('Dynamic Instrumentation', function () {
             }
           })
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({
           captureSnapshot: true,

diff --git a/integration-tests/debugger/snapshot.spec.js b/integration-tests/debugger/snapshot.spec.js
@@ -2,7 +2,6 @@
 
 const { assert } = require('chai')
 const { setup } = require('./utils')
-const { failOnException } = require('../helpers')
 
 describe('Dynamic Instrumentation', function () {
   const t = setup()
@@ -12,7 +11,7 @@ describe('Dynamic Instrumentation', function () {
       beforeEach(t.triggerBreakpoint)
 
       it('should capture a snapshot', function (done) {
-        t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => {
+        t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => {
           assert.deepEqual(Object.keys(captures), ['lines'])
           assert.deepEqual(Object.keys(captures.lines), [String(t.breakpoint.line)])
 
@@ -109,13 +108,13 @@ describe('Dynamic Instrumentation', function () {
           })
 
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true }))
       })
 
       it('should respect maxReferenceDepth', function (done) {
-        t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => {
+        t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => {
           const { locals } = captures.lines[t.breakpoint.line]
           delete locals.request
           delete locals.fastify
@@ -145,13 +144,13 @@ describe('Dynamic Instrumentation', function () {
           })
 
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true, capture: { maxReferenceDepth: 0 } }))
       })
 
       it('should respect maxLength', function (done) {
-        t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => {
+        t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => {
           const { locals } = captures.lines[t.breakpoint.line]
 
           assert.deepEqual(locals.lstr, {
@@ -162,13 +161,13 @@ describe('Dynamic Instrumentation', function () {
           })
 
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true, capture: { maxLength: 10 } }))
       })
 
       it('should respect maxCollectionSize', function (done) {
-        t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => {
+        t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => {
           const { locals } = captures.lines[t.breakpoint.line]
 
           assert.deepEqual(locals.arr, {
@@ -183,7 +182,7 @@ describe('Dynamic Instrumentation', function () {
           })
 
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true, capture: { maxCollectionSize: 3 } }))
       })
@@ -206,7 +205,7 @@ describe('Dynamic Instrumentation', function () {
           }
         }
 
-        t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => {
+        t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => {
           const { locals } = captures.lines[t.breakpoint.line]
 
           assert.deepEqual(Object.keys(locals), [
@@ -231,7 +230,7 @@ describe('Dynamic Instrumentation', function () {
           }
 
           done()
-        }))
+        })
 
         t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true, capture: { maxFieldCount } }))
       })

diff --git a/integration-tests/helpers/index.js b/integration-tests/helpers/index.js
@@ -356,26 +356,6 @@ function assertUUID (actual, msg = 'not a valid UUID') {
   assert.match(actual, /^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/, msg)
 }
 
-function failOnException (done, fn) {
-  if (fn[Symbol.toStringTag] === 'AsyncFunction') {
-    return async (...args) => {
-      try {
-        await fn(...args)
-      } catch (err) {
-        done(err)
-      }
-    }
-  } else {
-    return (...args) => {
-      try {
-        fn(...args)
-      } catch (err) {
-        done(err)
-      }
-    }
-  }
-}
-
 module.exports = {
   FakeAgent,
   hookFile,
@@ -392,6 +372,5 @@ module.exports = {
   spawnPluginIntegrationTestProc,
   useEnv,
   useSandbox,
-  sandboxCwd,
-  failOnException
+  sandboxCwd
 }