From 0d49ecf28c9c375523bb78d20ab32fa1028f9426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Antonio=20Fern=C3=A1ndez=20de=20Alba?= Date: Mon, 20 Jan 2025 15:00:53 +0100 Subject: [PATCH] [test optimization] Fix ATR + DI issues with jest (#5136) --- .../dynamic-instrumentation/is-jest.js | 7 --- .../test-hit-breakpoint.js | 15 +++--- .../test-not-hit-breakpoint.js | 9 ---- integration-tests/jest/jest.spec.js | 53 +++++++++++++++++-- integration-tests/mocha/mocha.spec.js | 12 +++-- packages/datadog-instrumentations/src/jest.js | 4 +- packages/dd-trace/src/plugins/util/test.js | 10 ++-- 7 files changed, 68 insertions(+), 42 deletions(-) delete mode 100644 integration-tests/ci-visibility/dynamic-instrumentation/is-jest.js diff --git a/integration-tests/ci-visibility/dynamic-instrumentation/is-jest.js b/integration-tests/ci-visibility/dynamic-instrumentation/is-jest.js deleted file mode 100644 index 483b2a543d3..00000000000 --- a/integration-tests/ci-visibility/dynamic-instrumentation/is-jest.js +++ /dev/null @@ -1,7 +0,0 @@ -module.exports = function () { - try { - return typeof jest !== 'undefined' - } catch (e) { - return false - } -} diff --git a/integration-tests/ci-visibility/dynamic-instrumentation/test-hit-breakpoint.js b/integration-tests/ci-visibility/dynamic-instrumentation/test-hit-breakpoint.js index ed2e3d14e51..57f1762edf9 100644 --- a/integration-tests/ci-visibility/dynamic-instrumentation/test-hit-breakpoint.js +++ b/integration-tests/ci-visibility/dynamic-instrumentation/test-hit-breakpoint.js @@ -1,19 +1,16 @@ /* eslint-disable */ const sum = require('./dependency') -const isJest = require('./is-jest') const { expect } = require('chai') -// TODO: instead of retrying through jest, this should be retried with auto test retries -if (isJest()) { - jest.retryTimes(1) -} - +let count = 0 describe('dynamic-instrumentation', () => { it('retries with DI', function () { - if (this.retries) { - this.retries(1) + if (process.env.TEST_SHOULD_PASS_AFTER_RETRY && count++ === 1) { + // Passes after a retry if TEST_SHOULD_PASS_AFTER_RETRY is passed + expect(sum(1, 3)).to.equal(4) + } else { + expect(sum(11, 3)).to.equal(14) } - expect(sum(11, 3)).to.equal(14) }) it('is not retried', () => { diff --git a/integration-tests/ci-visibility/dynamic-instrumentation/test-not-hit-breakpoint.js b/integration-tests/ci-visibility/dynamic-instrumentation/test-not-hit-breakpoint.js index 7960852a52c..bf051a37754 100644 --- a/integration-tests/ci-visibility/dynamic-instrumentation/test-not-hit-breakpoint.js +++ b/integration-tests/ci-visibility/dynamic-instrumentation/test-not-hit-breakpoint.js @@ -1,19 +1,10 @@ /* eslint-disable */ const sum = require('./dependency') -const isJest = require('./is-jest') const { expect } = require('chai') -// TODO: instead of retrying through jest, this should be retried with auto test retries -if (isJest()) { - jest.retryTimes(1) -} - let count = 0 describe('dynamic-instrumentation', () => { it('retries with DI', function () { - if (this.retries) { - this.retries(1) - } const willFail = count++ === 0 if (willFail) { expect(sum(11, 3)).to.equal(14) // only throws the first time diff --git a/integration-tests/jest/jest.spec.js b/integration-tests/jest/jest.spec.js index 47a5af89b85..ac604d96b5e 100644 --- a/integration-tests/jest/jest.spec.js +++ b/integration-tests/jest/jest.spec.js @@ -518,7 +518,7 @@ describe('jest CommonJS', () => { const retriedTests = tests.filter(test => test.meta[TEST_IS_RETRY] === 'true') assert.equal(retriedTests.length, 2) - const [retriedTest] = retriedTests + const retriedTest = retriedTests.find(test => test.meta[TEST_SUITE].includes('test-hit-breakpoint.js')) assert.propertyVal(retriedTest.meta, DI_ERROR_DEBUG_INFO_CAPTURED, 'true') @@ -560,6 +560,7 @@ describe('jest CommonJS', () => { ...getCiVisAgentlessConfig(receiver.port), TESTS_TO_RUN: 'dynamic-instrumentation/test-', DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1', RUN_IN_PARALLEL: true }, stdio: 'inherit' @@ -2518,7 +2519,8 @@ describe('jest CommonJS', () => { cwd, env: { ...getCiVisAgentlessConfig(receiver.port), - TESTS_TO_RUN: 'dynamic-instrumentation/test-hit-breakpoint' + TESTS_TO_RUN: 'dynamic-instrumentation/test-hit-breakpoint', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2565,7 +2567,8 @@ describe('jest CommonJS', () => { env: { ...getCiVisAgentlessConfig(receiver.port), TESTS_TO_RUN: 'dynamic-instrumentation/test-hit-breakpoint', - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2649,7 +2652,8 @@ describe('jest CommonJS', () => { env: { ...getCiVisAgentlessConfig(receiver.port), TESTS_TO_RUN: 'dynamic-instrumentation/test-hit-breakpoint', - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2698,7 +2702,8 @@ describe('jest CommonJS', () => { env: { ...getCiVisAgentlessConfig(receiver.port), TESTS_TO_RUN: 'dynamic-instrumentation/test-not-hit-breakpoint', - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2711,6 +2716,44 @@ describe('jest CommonJS', () => { }).catch(done) }) }) + + it('does not wait for breakpoint for a passed test', (done) => { + receiver.setSettings({ + flaky_test_retries_enabled: true, + di_enabled: true + }) + + const eventsPromise = receiver + .gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => { + const events = payloads.flatMap(({ payload }) => payload.events) + + const tests = events.filter(event => event.type === 'test').map(event => event.content) + const retriedTests = tests.filter(test => test.meta[TEST_IS_RETRY] === 'true') + + assert.equal(retriedTests.length, 1) + const [retriedTest] = retriedTests + // Duration is in nanoseconds, so 200 * 1e6 is 200ms + assert.equal(retriedTest.duration < 200 * 1e6, true) + }) + + childProcess = exec(runTestsWithCoverageCommand, + { + cwd, + env: { + ...getCiVisAgentlessConfig(receiver.port), + TESTS_TO_RUN: 'dynamic-instrumentation/test-hit-breakpoint', + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1', + TEST_SHOULD_PASS_AFTER_RETRY: '1' + }, + stdio: 'inherit' + } + ) + + childProcess.on('exit', () => { + eventsPromise.then(() => done()).catch(done) + }) + }) }) // This happens when using office-addin-mock diff --git a/integration-tests/mocha/mocha.spec.js b/integration-tests/mocha/mocha.spec.js index 1bb369c0627..a7c23b067df 100644 --- a/integration-tests/mocha/mocha.spec.js +++ b/integration-tests/mocha/mocha.spec.js @@ -2188,7 +2188,8 @@ describe('mocha CommonJS', function () { ...getCiVisAgentlessConfig(receiver.port), TESTS_TO_RUN: JSON.stringify([ './dynamic-instrumentation/test-hit-breakpoint' - ]) + ]), + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2240,7 +2241,8 @@ describe('mocha CommonJS', function () { TESTS_TO_RUN: JSON.stringify([ './dynamic-instrumentation/test-hit-breakpoint' ]), - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2329,7 +2331,8 @@ describe('mocha CommonJS', function () { TESTS_TO_RUN: JSON.stringify([ './dynamic-instrumentation/test-hit-breakpoint' ]), - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } @@ -2382,7 +2385,8 @@ describe('mocha CommonJS', function () { TESTS_TO_RUN: JSON.stringify([ './dynamic-instrumentation/test-not-hit-breakpoint' ]), - DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true' + DD_TEST_DYNAMIC_INSTRUMENTATION_ENABLED: 'true', + DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1' }, stdio: 'inherit' } diff --git a/packages/datadog-instrumentations/src/jest.js b/packages/datadog-instrumentations/src/jest.js index 898927aeaff..7a1001d11f3 100644 --- a/packages/datadog-instrumentations/src/jest.js +++ b/packages/datadog-instrumentations/src/jest.js @@ -303,7 +303,7 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) { const numRetries = this.global[RETRY_TIMES] const numTestExecutions = event.test?.invocations const willBeRetried = numRetries > 0 && numTestExecutions - 1 < numRetries - const mightHitBreakpoint = this.isDiEnabled && numTestExecutions >= 1 + const mightHitBreakpoint = this.isDiEnabled && numTestExecutions >= 2 const asyncResource = asyncResources.get(event.test) @@ -319,7 +319,7 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) { // 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 (mightHitBreakpoint) { + if (status === 'fail' && mightHitBreakpoint) { await new Promise(resolve => { setTimeout(() => { resolve() diff --git a/packages/dd-trace/src/plugins/util/test.js b/packages/dd-trace/src/plugins/util/test.js index 11181e1d9eb..d8aab1a44da 100644 --- a/packages/dd-trace/src/plugins/util/test.js +++ b/packages/dd-trace/src/plugins/util/test.js @@ -691,14 +691,12 @@ function getFileAndLineNumberFromError (error, repositoryRoot) { return [] } -// The error.stack property in TestingLibraryElementError includes the message, which results in redundant information function getFormattedError (error, repositoryRoot) { - if (error.name !== 'TestingLibraryElementError') { - return error - } - const { stack } = error const newError = new Error(error.message) - newError.stack = stack.split('\n').filter(line => line.includes(repositoryRoot)).join('\n') + if (error.stack) { + newError.stack = error.stack.split('\n').filter(line => line.includes(repositoryRoot)).join('\n') + } + newError.name = error.name return newError }