From 7f35fe33a6a2e110d358d433fb33b85fb97fdb62 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 11 Dec 2024 10:02:46 +0100 Subject: [PATCH 1/3] [DI] Handle async errors in mocha tests If an async error is thrown in mocha tests, mocha doesn't see it. Best case, the test will just time out, worst case, it will pass. --- integration-tests/debugger/basic.spec.js | 82 ++++++++----------- .../debugger/snapshot-pruning.spec.js | 5 +- integration-tests/debugger/snapshot.spec.js | 21 ++--- integration-tests/helpers/index.js | 23 +++++- 4 files changed, 71 insertions(+), 60 deletions(-) diff --git a/integration-tests/debugger/basic.spec.js b/integration-tests/debugger/basic.spec.js index 8782bc90449..e12aaaedc87 100644 --- 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 } = require('../helpers') +const { assertObjectContains, assertUUID, failOnException } = require('../helpers') const { ACKNOWLEDGED, ERROR } = require('../../packages/dd-trace/src/appsec/remote_config/apply_states') const { version } = require('../../package.json') @@ -35,7 +35,7 @@ describe('Dynamic Instrumentation', function () { debugger: { diagnostics: { probeId, probeVersion: 0, status: 'EMITTING' } } }] - t.agent.on('remote-config-ack-update', (id, version, state, error) => { + t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => { assert.strictEqual(id, t.rcConfig.id) assert.strictEqual(version, 1) assert.strictEqual(state, ACKNOWLEDGED) @@ -43,9 +43,9 @@ describe('Dynamic Instrumentation', function () { receivedAckUpdate = true endIfDone() - }) + })) - t.agent.on('debugger-diagnostics', ({ payload }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ 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', (id, version, state, error) => { + t.agent.on('remote-config-ack-update', failOnException(done, (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', ({ payload }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ 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,7 +135,7 @@ describe('Dynamic Instrumentation', function () { debugger: { diagnostics: { probeId, probeVersion: 0, status: 'INSTALLED' } } }] - t.agent.on('remote-config-ack-update', (id, version, state, error) => { + t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => { assert.strictEqual(id, t.rcConfig.id) assert.strictEqual(version, 1) assert.strictEqual(state, ACKNOWLEDGED) @@ -143,9 +143,9 @@ describe('Dynamic Instrumentation', function () { receivedAckUpdate = true endIfDone() - }) + })) - t.agent.on('debugger-diagnostics', ({ payload }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ 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,7 +183,7 @@ describe('Dynamic Instrumentation', function () { it(title, function (done) { let receivedAckUpdate = false - t.agent.on('remote-config-ack-update', (id, version, state, error) => { + t.agent.on('remote-config-ack-update', failOnException(done, (id, version, state, error) => { assert.strictEqual(id, `logProbe_${config.id}`) assert.strictEqual(version, 1) assert.strictEqual(state, ERROR) @@ -191,7 +191,7 @@ describe('Dynamic Instrumentation', function () { receivedAckUpdate = true endIfDone() - }) + })) const probeId = config.id const expectedPayloads = [{ @@ -204,7 +204,7 @@ describe('Dynamic Instrumentation', function () { debugger: { diagnostics: customErrorDiagnosticsObj ?? { probeId, version: 0, status: 'ERROR' } } }] - t.agent.on('debugger-diagnostics', ({ payload }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ 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', ({ payload }) => { + t.agent.on('debugger-input', failOnException(done, ({ 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,43 +307,31 @@ describe('Dynamic Instrumentation', function () { if (payload.debugger.diagnostics.status === 'INSTALLED') triggers.shift()().catch(done) }) - t.agent.on('debugger-input', ({ payload }) => { + t.agent.on('debugger-input', failOnException(done, ({ 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', 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.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.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.removeRemoteConfig(t.rcConfig.id) } - }) + })) t.agent.on('debugger-input', () => { - assert.fail('should not capture anything when the probe is deleted') + done(new Error('should not capture anything when the probe is deleted')) }) t.agent.addRemoteConfig(t.rcConfig) @@ -354,7 +342,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', ({ payload: { debugger: { diagnostics: { status, probeId } } } }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ payload: { debugger: { diagnostics: { status, probeId } } } }) => { if (status !== 'INSTALLED') return if (probeId === t.rcConfig.config.id) { @@ -387,7 +375,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 index 91190a1c25d..6458491c11c 100644 --- a/integration-tests/debugger/snapshot-pruning.spec.js +++ b/integration-tests/debugger/snapshot-pruning.spec.js @@ -2,6 +2,7 @@ const { assert } = require('chai') const { setup, getBreakpointInfo } = require('./utils') +const { failOnException } = require('../helpers') const { line } = getBreakpointInfo() @@ -13,7 +14,7 @@ describe('Dynamic Instrumentation', function () { beforeEach(t.triggerBreakpoint) it('should prune snapshot if payload is too large', function (done) { - t.agent.on('debugger-input', ({ payload }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload }) => { assert.isBelow(Buffer.byteLength(JSON.stringify(payload)), 1024 * 1024) // 1MB assert.deepEqual(payload['debugger.snapshot'].captures, { lines: { @@ -26,7 +27,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 index e3d17b225c4..d296c9c1b53 100644 --- a/integration-tests/debugger/snapshot.spec.js +++ b/integration-tests/debugger/snapshot.spec.js @@ -2,6 +2,7 @@ const { assert } = require('chai') const { setup } = require('./utils') +const { failOnException } = require('../helpers') describe('Dynamic Instrumentation', function () { const t = setup() @@ -11,7 +12,7 @@ describe('Dynamic Instrumentation', function () { beforeEach(t.triggerBreakpoint) it('should capture a snapshot', function (done) { - t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => { assert.deepEqual(Object.keys(captures), ['lines']) assert.deepEqual(Object.keys(captures.lines), [String(t.breakpoint.line)]) @@ -108,13 +109,13 @@ describe('Dynamic Instrumentation', function () { }) done() - }) + })) t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true })) }) it('should respect maxReferenceDepth', function (done) { - t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => { const { locals } = captures.lines[t.breakpoint.line] delete locals.request delete locals.fastify @@ -144,13 +145,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', ({ payload: { 'debugger.snapshot': { captures } } }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => { const { locals } = captures.lines[t.breakpoint.line] assert.deepEqual(locals.lstr, { @@ -161,13 +162,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', ({ payload: { 'debugger.snapshot': { captures } } }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => { const { locals } = captures.lines[t.breakpoint.line] assert.deepEqual(locals.arr, { @@ -182,7 +183,7 @@ describe('Dynamic Instrumentation', function () { }) done() - }) + })) t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true, capture: { maxCollectionSize: 3 } })) }) @@ -205,7 +206,7 @@ describe('Dynamic Instrumentation', function () { } } - t.agent.on('debugger-input', ({ payload: { 'debugger.snapshot': { captures } } }) => { + t.agent.on('debugger-input', failOnException(done, ({ payload: { 'debugger.snapshot': { captures } } }) => { const { locals } = captures.lines[t.breakpoint.line] assert.deepEqual(Object.keys(locals), [ @@ -230,7 +231,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 index 22074c3af20..a39a9ac0ed9 100644 --- a/integration-tests/helpers/index.js +++ b/integration-tests/helpers/index.js @@ -356,6 +356,26 @@ 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, @@ -372,5 +392,6 @@ module.exports = { spawnPluginIntegrationTestProc, useEnv, useSandbox, - sandboxCwd + sandboxCwd, + failOnException } From b35fc2b76939dc0dcfe67e89a62a068207cc1791 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 11 Dec 2024 10:04:18 +0100 Subject: [PATCH 2/3] [DI] Fix async error in itegration test --- integration-tests/debugger/basic.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/debugger/basic.spec.js b/integration-tests/debugger/basic.spec.js index e12aaaedc87..54319e3861d 100644 --- a/integration-tests/debugger/basic.spec.js +++ b/integration-tests/debugger/basic.spec.js @@ -201,7 +201,7 @@ describe('Dynamic Instrumentation', function () { }, { ddsource: 'dd_debugger', service: 'node', - debugger: { diagnostics: customErrorDiagnosticsObj ?? { probeId, version: 0, status: 'ERROR' } } + debugger: { diagnostics: customErrorDiagnosticsObj ?? { probeId, probeVersion: 0, status: 'ERROR' } } }] t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => { From 56ee3fe04d1e774c20bc603826cda87070270c45 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 11 Dec 2024 10:23:39 +0100 Subject: [PATCH 3/3] Fix linting --- integration-tests/debugger/basic.spec.js | 3 ++- integration-tests/helpers/index.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/integration-tests/debugger/basic.spec.js b/integration-tests/debugger/basic.spec.js index 54319e3861d..189032049f2 100644 --- a/integration-tests/debugger/basic.spec.js +++ b/integration-tests/debugger/basic.spec.js @@ -342,7 +342,8 @@ 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: { debugger: { diagnostics: { status, probeId } } } }) => { + t.agent.on('debugger-diagnostics', failOnException(done, ({ payload }) => { + const { status, probeId } = payload.debugger.diagnostics if (status !== 'INSTALLED') return if (probeId === t.rcConfig.config.id) { diff --git a/integration-tests/helpers/index.js b/integration-tests/helpers/index.js index a39a9ac0ed9..b67f21a6d92 100644 --- a/integration-tests/helpers/index.js +++ b/integration-tests/helpers/index.js @@ -370,7 +370,7 @@ function failOnException (done, fn) { try { fn(...args) } catch (err) { - done (err) + done(err) } } }