From e340f7a36909cf017993351a5bec06c60a910cf6 Mon Sep 17 00:00:00 2001 From: Juan Fernandez Date: Thu, 9 Jan 2025 13:33:09 +0100 Subject: [PATCH] fix mocha --- integration-tests/jest/jest.spec.js | 6 ++- integration-tests/mocha/mocha.spec.js | 51 +++++++++---------- .../src/mocha/utils.js | 30 ++++++++--- packages/datadog-plugin-jest/src/index.js | 39 -------------- packages/datadog-plugin-mocha/src/index.js | 45 ++++++---------- packages/dd-trace/src/plugins/ci_plugin.js | 40 ++++++++++++++- 6 files changed, 107 insertions(+), 104 deletions(-) diff --git a/integration-tests/jest/jest.spec.js b/integration-tests/jest/jest.spec.js index d1de8dd6ec5..fa1e566be31 100644 --- a/integration-tests/jest/jest.spec.js +++ b/integration-tests/jest/jest.spec.js @@ -2530,9 +2530,11 @@ describe('jest CommonJS', () => { .endsWith('ci-visibility/dynamic-instrumentation/dependency.js') ) assert.equal(retriedTest.metrics[`${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_LINE_SUFFIX}`], 4) - assert.exists(retriedTest.meta[`${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}`]) - snapshotIdByTest = retriedTest.meta[`${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}`] + const snapshotIdKey = `${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}` + assert.exists(retriedTest.meta[snapshotIdKey]) + + snapshotIdByTest = retriedTest.meta[snapshotIdKey] spanIdByTest = retriedTest.span_id.toString() traceIdByTest = retriedTest.trace_id.toString() diff --git a/integration-tests/mocha/mocha.spec.js b/integration-tests/mocha/mocha.spec.js index d6d13673485..1bb369c0627 100644 --- a/integration-tests/mocha/mocha.spec.js +++ b/integration-tests/mocha/mocha.spec.js @@ -37,9 +37,10 @@ const { TEST_LEVEL_EVENT_TYPES, TEST_EARLY_FLAKE_ABORT_REASON, DI_ERROR_DEBUG_INFO_CAPTURED, - DI_DEBUG_ERROR_FILE, - DI_DEBUG_ERROR_SNAPSHOT_ID, - DI_DEBUG_ERROR_LINE + DI_DEBUG_ERROR_PREFIX, + DI_DEBUG_ERROR_FILE_SUFFIX, + DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX, + DI_DEBUG_ERROR_LINE_SUFFIX } = require('../../packages/dd-trace/src/plugins/util/test') const { DD_HOST_CPU_COUNT } = require('../../packages/dd-trace/src/plugins/util/env') const { ERROR_MESSAGE } = require('../../packages/dd-trace/src/constants') @@ -2166,10 +2167,10 @@ describe('mocha CommonJS', function () { assert.equal(retriedTests.length, 1) const [retriedTest] = retriedTests - assert.notProperty(retriedTest.meta, DI_ERROR_DEBUG_INFO_CAPTURED) - assert.notProperty(retriedTest.meta, DI_DEBUG_ERROR_FILE) - assert.notProperty(retriedTest.metrics, DI_DEBUG_ERROR_LINE) - assert.notProperty(retriedTest.meta, DI_DEBUG_ERROR_SNAPSHOT_ID) + const hasDebugTags = Object.keys(retriedTest.meta) + .some(property => property.startsWith(DI_DEBUG_ERROR_PREFIX) || property === DI_ERROR_DEBUG_INFO_CAPTURED) + + assert.isFalse(hasDebugTags) }) const logsPromise = receiver @@ -2217,10 +2218,10 @@ describe('mocha CommonJS', function () { assert.equal(retriedTests.length, 1) const [retriedTest] = retriedTests - assert.notProperty(retriedTest.meta, DI_ERROR_DEBUG_INFO_CAPTURED) - assert.notProperty(retriedTest.meta, DI_DEBUG_ERROR_FILE) - assert.notProperty(retriedTest.metrics, DI_DEBUG_ERROR_LINE) - assert.notProperty(retriedTest.meta, DI_DEBUG_ERROR_SNAPSHOT_ID) + const hasDebugTags = Object.keys(retriedTest.meta) + .some(property => property.startsWith(DI_DEBUG_ERROR_PREFIX) || property === DI_ERROR_DEBUG_INFO_CAPTURED) + + assert.isFalse(hasDebugTags) }) const logsPromise = receiver @@ -2273,15 +2274,17 @@ describe('mocha CommonJS', function () { const [retriedTest] = retriedTests assert.propertyVal(retriedTest.meta, DI_ERROR_DEBUG_INFO_CAPTURED, 'true') - assert.propertyVal( - retriedTest.meta, - DI_DEBUG_ERROR_FILE, - 'ci-visibility/dynamic-instrumentation/dependency.js' + assert.isTrue( + retriedTest.meta[`${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_FILE_SUFFIX}`] + .endsWith('ci-visibility/dynamic-instrumentation/dependency.js') ) - assert.equal(retriedTest.metrics[DI_DEBUG_ERROR_LINE], 4) - assert.exists(retriedTest.meta[DI_DEBUG_ERROR_SNAPSHOT_ID]) + assert.equal(retriedTest.metrics[`${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_LINE_SUFFIX}`], 4) + + const snapshotIdKey = `${DI_DEBUG_ERROR_PREFIX}.0.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}` + + assert.exists(retriedTest.meta[snapshotIdKey]) - snapshotIdByTest = retriedTest.meta[DI_DEBUG_ERROR_SNAPSHOT_ID] + snapshotIdByTest = retriedTest.meta[snapshotIdKey] spanIdByTest = retriedTest.span_id.toString() traceIdByTest = retriedTest.trace_id.toString() @@ -2358,14 +2361,10 @@ describe('mocha CommonJS', function () { assert.equal(retriedTests.length, 1) const [retriedTest] = retriedTests - assert.propertyVal(retriedTest.meta, DI_ERROR_DEBUG_INFO_CAPTURED, 'true') - assert.propertyVal( - retriedTest.meta, - DI_DEBUG_ERROR_FILE, - 'ci-visibility/dynamic-instrumentation/dependency.js' - ) - assert.equal(retriedTest.metrics[DI_DEBUG_ERROR_LINE], 4) - assert.exists(retriedTest.meta[DI_DEBUG_ERROR_SNAPSHOT_ID]) + const hasDebugTags = Object.keys(retriedTest.meta) + .some(property => property.startsWith(DI_DEBUG_ERROR_PREFIX) || property === DI_ERROR_DEBUG_INFO_CAPTURED) + + assert.isFalse(hasDebugTags) }) const logsPromise = receiver .gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/logs'), (payloads) => { diff --git a/packages/datadog-instrumentations/src/mocha/utils.js b/packages/datadog-instrumentations/src/mocha/utils.js index ce462f13256..97b5f2d1209 100644 --- a/packages/datadog-instrumentations/src/mocha/utils.js +++ b/packages/datadog-instrumentations/src/mocha/utils.js @@ -19,6 +19,7 @@ const skipCh = channel('ci:mocha:test:skip') // suite channels const testSuiteErrorCh = channel('ci:mocha:test-suite:error') +const BREAKPOINT_HIT_GRACE_PERIOD_MS = 200 const testToAr = new WeakMap() const originalFns = new WeakMap() const testToStartLine = new WeakMap() @@ -73,7 +74,7 @@ function isMochaRetry (test) { return test._currentRetry !== undefined && test._currentRetry !== 0 } -function isLastRetry (test) { +function getIsLastRetry (test) { return test._currentRetry === test._retries } @@ -203,14 +204,28 @@ function getOnTestHandler (isMain) { } function getOnTestEndHandler () { - return function (test) { + return async function (test) { const asyncResource = getTestAsyncResource(test) const status = getTestStatus(test) + // After finishing it might take a bit for the snapshot to be handled. + // This means that tests retried with DI are BREAKPOINT_HIT_GRACE_PERIOD_MS slower at least. + if (test._ddShouldWaitForHitProbe || test._retriedTest?._ddShouldWaitForHitProbe) { + await new Promise((resolve) => { + setTimeout(() => { + resolve() + }, BREAKPOINT_HIT_GRACE_PERIOD_MS) + }) + } + // if there are afterEach to be run, we don't finish the test yet if (asyncResource && !test.parent._afterEach.length) { asyncResource.runInAsyncScope(() => { - testFinishCh.publish({ status, hasBeenRetried: isMochaRetry(test) }) + testFinishCh.publish({ + status, + hasBeenRetried: isMochaRetry(test), + isLastRetry: getIsLastRetry(test) + }) }) } } @@ -220,16 +235,17 @@ function getOnHookEndHandler () { return function (hook) { const test = hook.ctx.currentTest if (test && hook.parent._afterEach.includes(hook)) { // only if it's an afterEach - const isLastAfterEach = hook.parent._afterEach.indexOf(hook) === hook.parent._afterEach.length - 1 - if (test._retries > 0 && !isLastRetry(test)) { + const isLastRetry = getIsLastRetry(test) + if (test._retries > 0 && !isLastRetry) { return } + const isLastAfterEach = hook.parent._afterEach.indexOf(hook) === hook.parent._afterEach.length - 1 if (isLastAfterEach) { const status = getTestStatus(test) const asyncResource = getTestAsyncResource(test) if (asyncResource) { asyncResource.runInAsyncScope(() => { - testFinishCh.publish({ status, hasBeenRetried: isMochaRetry(test) }) + testFinishCh.publish({ status, hasBeenRetried: isMochaRetry(test), isLastRetry }) }) } } @@ -286,7 +302,7 @@ function getOnTestRetryHandler () { const isFirstAttempt = test._currentRetry === 0 const willBeRetried = test._currentRetry < test._retries asyncResource.runInAsyncScope(() => { - testRetryCh.publish({ isFirstAttempt, err, willBeRetried }) + testRetryCh.publish({ isFirstAttempt, err, willBeRetried, test }) }) } const key = getTestToArKey(test) diff --git a/packages/datadog-plugin-jest/src/index.js b/packages/datadog-plugin-jest/src/index.js index 75736104db7..1825e644548 100644 --- a/packages/datadog-plugin-jest/src/index.js +++ b/packages/datadog-plugin-jest/src/index.js @@ -23,11 +23,6 @@ const { JEST_DISPLAY_NAME, TEST_IS_RUM_ACTIVE, TEST_BROWSER_DRIVER, - DI_ERROR_DEBUG_INFO_CAPTURED, - DI_DEBUG_ERROR_PREFIX, - DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX, - DI_DEBUG_ERROR_FILE_SUFFIX, - DI_DEBUG_ERROR_LINE_SUFFIX, getFormattedError } = require('../../dd-trace/src/plugins/util/test') const { COMPONENT } = require('../../dd-trace/src/constants') @@ -43,7 +38,6 @@ const { TELEMETRY_CODE_COVERAGE_NUM_FILES, TELEMETRY_TEST_SESSION } = require('../../dd-trace/src/ci-visibility/telemetry') -const log = require('../../dd-trace/src/log') const isJestWorker = !!process.env.JEST_WORKER_ID @@ -377,39 +371,6 @@ class JestPlugin extends CiPlugin { }) } - onDiBreakpointHit ({ snapshot }) { - if (!this.activeTestSpan || this.activeTestSpan.context()._isFinished) { - // This is unexpected and is caused by a race condition. - log.warn('Breakpoint snapshot could not be attached to the active test span') - return - } - - const stackIndex = this.testErrorStackIndex - - this.activeTestSpan.setTag(DI_ERROR_DEBUG_INFO_CAPTURED, 'true') - this.activeTestSpan.setTag( - `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}`, - snapshot.id - ) - this.activeTestSpan.setTag( - `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_FILE_SUFFIX}`, - snapshot.probe.location.file - ) - this.activeTestSpan.setTag( - `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_LINE_SUFFIX}`, - Number(snapshot.probe.location.lines[0]) - ) - - const activeTestSpanContext = this.activeTestSpan.context() - this.tracer._exporter.exportDiLogs(this.testEnvironmentMetadata, { - debugger: { snapshot }, - dd: { - trace_id: activeTestSpanContext.toTraceId(), - span_id: activeTestSpanContext.toSpanId() - } - }) - } - startTestSpan (test) { const { suite, diff --git a/packages/datadog-plugin-mocha/src/index.js b/packages/datadog-plugin-mocha/src/index.js index 1b40b9c5a1c..232369d34c0 100644 --- a/packages/datadog-plugin-mocha/src/index.js +++ b/packages/datadog-plugin-mocha/src/index.js @@ -52,8 +52,6 @@ const { const id = require('../../dd-trace/src/id') const log = require('../../dd-trace/src/log') -const debuggerParameterPerTest = new Map() - function getTestSuiteLevelVisibilityTags (testSuiteSpan) { const testSuiteSpanContext = testSuiteSpan.context() const suiteTags = { @@ -192,36 +190,15 @@ class MochaPlugin extends CiPlugin { const store = storage.getStore() const span = this.startTestSpan(testInfo) - const { testName } = testInfo - - const debuggerParameters = debuggerParameterPerTest.get(testName) - - if (debuggerParameters) { - const spanContext = span.context() - - // TODO: handle race conditions with this.retriedTestIds - this.retriedTestIds = { - spanId: spanContext.toSpanId(), - traceId: spanContext.toTraceId() - } - const { snapshotId, file, line } = debuggerParameters - - // TODO: should these be added on test:end if and only if the probe is hit? - // Sync issues: `hitProbePromise` might be resolved after the test ends - span.setTag(DI_ERROR_DEBUG_INFO_CAPTURED, 'true') - span.setTag(DI_DEBUG_ERROR_SNAPSHOT_ID, snapshotId) - span.setTag(DI_DEBUG_ERROR_FILE, file) - span.setTag(DI_DEBUG_ERROR_LINE, line) - } - this.enter(span, store) + this.activeTestSpan = span }) this.addSub('ci:mocha:worker:finish', () => { this.tracer._exporter.flush() }) - this.addSub('ci:mocha:test:finish', ({ status, hasBeenRetried }) => { + this.addSub('ci:mocha:test:finish', ({ status, hasBeenRetried, isLastRetry }) => { const store = storage.getStore() const span = store?.span @@ -245,6 +222,11 @@ class MochaPlugin extends CiPlugin { span.finish() finishAllTraceSpans(span) + this.activeTestSpan = null + if (this.di && this.libraryConfig?.isDiEnabled && this.runningTestProbeId && isLastRetry) { + this.removeDiProbe(this.runningTestProbeId) + this.runningTestProbeId = null + } } }) @@ -271,7 +253,7 @@ class MochaPlugin extends CiPlugin { } }) - this.addSub('ci:mocha:test:retry', ({ isFirstAttempt, willBeRetried, err }) => { + this.addSub('ci:mocha:test:retry', ({ isFirstAttempt, willBeRetried, err, test }) => { const store = storage.getStore() const span = store?.span if (span) { @@ -295,9 +277,14 @@ class MochaPlugin extends CiPlugin { } ) if (willBeRetried && this.di && this.libraryConfig?.isDiEnabled) { - const testName = span.context()._tags[TEST_NAME] - const debuggerParameters = this.addDiProbe(err) - debuggerParameterPerTest.set(testName, debuggerParameters) + const probeInformation = this.addDiProbe(err, this.onDiBreakpointHit.bind(this)) + if (probeInformation) { + const { probeId, stackIndex } = probeInformation + this.runningTestProbeId = probeId + this.testErrorStackIndex = stackIndex + test._ddShouldWaitForHitProbe = true + // TODO: we're not waiting for setProbePromise to be resolved, so there might be race conditions + } } span.finish() diff --git a/packages/dd-trace/src/plugins/ci_plugin.js b/packages/dd-trace/src/plugins/ci_plugin.js index 41d31a25c46..9f13d3f34f7 100644 --- a/packages/dd-trace/src/plugins/ci_plugin.js +++ b/packages/dd-trace/src/plugins/ci_plugin.js @@ -22,7 +22,12 @@ const { TEST_SOURCE_FILE, TEST_LEVEL_EVENT_TYPES, TEST_SUITE, - getFileAndLineNumberFromError + getFileAndLineNumberFromError, + DI_ERROR_DEBUG_INFO_CAPTURED, + DI_DEBUG_ERROR_PREFIX, + DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX, + DI_DEBUG_ERROR_FILE_SUFFIX, + DI_DEBUG_ERROR_LINE_SUFFIX } = require('./util/test') const Plugin = require('./plugin') const { COMPONENT } = require('../constants') @@ -291,6 +296,39 @@ module.exports = class CiPlugin extends Plugin { return testSpan } + onDiBreakpointHit ({ snapshot }) { + if (!this.activeTestSpan || this.activeTestSpan.context()._isFinished) { + // This is unexpected and is caused by a race condition. + log.warn('Breakpoint snapshot could not be attached to the active test span') + return + } + + const stackIndex = this.testErrorStackIndex + + this.activeTestSpan.setTag(DI_ERROR_DEBUG_INFO_CAPTURED, 'true') + this.activeTestSpan.setTag( + `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_SNAPSHOT_ID_SUFFIX}`, + snapshot.id + ) + this.activeTestSpan.setTag( + `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_FILE_SUFFIX}`, + snapshot.probe.location.file + ) + this.activeTestSpan.setTag( + `${DI_DEBUG_ERROR_PREFIX}.${stackIndex}.${DI_DEBUG_ERROR_LINE_SUFFIX}`, + Number(snapshot.probe.location.lines[0]) + ) + + const activeTestSpanContext = this.activeTestSpan.context() + this.tracer._exporter.exportDiLogs(this.testEnvironmentMetadata, { + debugger: { snapshot }, + dd: { + trace_id: activeTestSpanContext.toTraceId(), + span_id: activeTestSpanContext.toSpanId() + } + }) + } + removeDiProbe (probeId) { return this.di.removeProbe(probeId) }