Skip to content

Commit 3d8fdf5

Browse files
juan-fernandezjuan-fernandez
authored
[test optimization] Fix error suppression bug when jest fails with test suite errors (#7524)
1 parent 015654a commit 3d8fdf5

File tree

5 files changed

+247
-3
lines changed

5 files changed

+247
-3
lines changed

‎eslint.config.mjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export default [
5454
'**/acmeair-nodejs', // We don't own this.
5555
'**/vendor', // Generally, we didn't author this code.
5656
'**/.analysis', // Ignore apm-instrumentation-toolkit analysis results
57+
'integration-tests/ci-visibility/test-management/test-suite-failed-to-run-parse.js', // Intentional syntax error
5758
'integration-tests/code-origin/typescript.js', // Generated
5859
'integration-tests/debugger/target-app/source-map-support/bundle.js', // Generated
5960
'integration-tests/debugger/target-app/source-map-support/hello/world.js', // Generated

‎integration-tests/ci-visibility/test-management/test-suite-failed-to-run-parse.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
'use strict'
2+
3+
// Intentionally cause "Test suite failed to run" - parse error so no tests run.
4+
// Used to verify we do NOT flip exit code to 0 when quarantine/EFD would otherwise
5+
// ignore failures, because suite-level failures cannot be ignored.
6+
)

‎integration-tests/ci-visibility/test-management/test-suite-failed-to-run-resolution.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
'use strict'
2+
3+
// Intentionally cause "Test suite failed to run" - module resolution error so no tests run.
4+
// Used to verify we do NOT flip exit code to 0 when quarantine/EFD would otherwise
5+
// ignore failures, because suite-level failures cannot be ignored.
6+
require('./this-module-does-not-exist-xyz')
7+
8+
describe('suite failed to run', () => {
9+
it('will not run', () => {})
10+
})

‎integration-tests/jest/jest.spec.js‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3556,6 +3556,106 @@ describe(`jest@${JEST_VERSION} commonJS`, () => {
35563556
])
35573557
})
35583558
})
3559+
3560+
it('does not flip exit code to 0 when a test suite fails to parse', async () => {
3561+
receiver.setKnownTests({ jest: {} })
3562+
receiver.setSettings({
3563+
early_flake_detection: {
3564+
enabled: true,
3565+
slow_test_retries: { '5s': 3 },
3566+
faulty_session_threshold: 100,
3567+
},
3568+
known_tests_enabled: true,
3569+
})
3570+
3571+
// Scenario: (1) test-suite-failed-to-run-parse.js fails to parse,
3572+
// (2) occasionally-failing-test is new, flaky (pass/fail alternates), EFD would ignore its failures.
3573+
const testAssertionsPromise = receiver
3574+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
3575+
const events = payloads.flatMap(({ payload }) => payload.events)
3576+
const testSession = events.find(event => event.type === 'test_session_end')?.content
3577+
assert.strictEqual(testSession.meta[TEST_STATUS], 'fail')
3578+
assert.strictEqual(testSession.meta[TEST_EARLY_FLAKE_ENABLED], 'true', 'EFD should be running')
3579+
3580+
// TODO: parsing errors do not report test suite
3581+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
3582+
const occasionallyFailingTests = tests.filter(t => t.resource?.includes('occasionally-failing-test'))
3583+
const numRetries = 3 // slow_test_retries: { '5s': 3 }
3584+
assert.strictEqual(occasionallyFailingTests.length, 1 + numRetries, '1 original + 3 EFD retries')
3585+
const efdRetried = occasionallyFailingTests.filter(t =>
3586+
t.meta?.[TEST_IS_RETRY] === 'true' && t.meta?.[TEST_RETRY_REASON] === TEST_RETRY_REASON_TYPES.efd
3587+
)
3588+
assert.strictEqual(efdRetried.length, numRetries, 'all but 1 should have EFD retry tag and reason')
3589+
})
3590+
3591+
childProcess = exec(
3592+
runTestsCommand,
3593+
{
3594+
cwd,
3595+
env: {
3596+
...getCiVisAgentlessConfig(receiver.port),
3597+
TESTS_TO_RUN: '(test-management/test-suite-failed-to-run-parse|' +
3598+
'test-early-flake-detection/occasionally-failing-test)',
3599+
SHOULD_CHECK_RESULTS: '1',
3600+
},
3601+
}
3602+
)
3603+
3604+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), testAssertionsPromise])
3605+
assert.strictEqual(exitCode, 1, 'exit code 1 when test suite fails to parse')
3606+
})
3607+
3608+
it('does not flip exit code to 0 when a test suite fails due to module resolution error', async () => {
3609+
receiver.setKnownTests({ jest: {} })
3610+
receiver.setSettings({
3611+
early_flake_detection: {
3612+
enabled: true,
3613+
slow_test_retries: { '5s': 3 },
3614+
faulty_session_threshold: 100,
3615+
},
3616+
known_tests_enabled: true,
3617+
})
3618+
3619+
// Scenario: (1) test-suite-failed-to-run-resolution.js fails to load,
3620+
// (2) occasionally-failing-test is new, flaky, EFD would ignore its failures.
3621+
const testAssertionsPromise = receiver
3622+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
3623+
const events = payloads.flatMap(({ payload }) => payload.events)
3624+
const testSession = events.find(event => event.type === 'test_session_end')?.content
3625+
assert.strictEqual(testSession.meta[TEST_STATUS], 'fail')
3626+
assert.strictEqual(testSession.meta[TEST_EARLY_FLAKE_ENABLED], 'true', 'EFD should be running')
3627+
3628+
const suites = events.filter(event => event.type === 'test_suite_end').map(event => event.content)
3629+
const failedSuite = suites.find(s => s.meta?.[TEST_SUITE]?.includes('test-suite-failed-to-run-resolution'))
3630+
assert.ok(failedSuite, 'failing test suite should be reported')
3631+
assert.strictEqual(failedSuite.meta[TEST_STATUS], 'fail')
3632+
3633+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
3634+
const occasionallyFailingTests = tests.filter(t => t.resource?.includes('occasionally-failing-test'))
3635+
const numRetries = 3 // slow_test_retries: { '5s': 3 }
3636+
assert.strictEqual(occasionallyFailingTests.length, 1 + numRetries, '1 original + 3 EFD retries')
3637+
const efdRetried = occasionallyFailingTests.filter(t =>
3638+
t.meta?.[TEST_IS_RETRY] === 'true' && t.meta?.[TEST_RETRY_REASON] === TEST_RETRY_REASON_TYPES.efd
3639+
)
3640+
assert.strictEqual(efdRetried.length, numRetries, 'all but 1 should have EFD retry tag and reason')
3641+
})
3642+
3643+
childProcess = exec(
3644+
runTestsCommand,
3645+
{
3646+
cwd,
3647+
env: {
3648+
...getCiVisAgentlessConfig(receiver.port),
3649+
TESTS_TO_RUN: '(test-management/test-suite-failed-to-run-resolution|' +
3650+
'test-early-flake-detection/occasionally-failing-test)',
3651+
SHOULD_CHECK_RESULTS: '1',
3652+
},
3653+
}
3654+
)
3655+
3656+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), testAssertionsPromise])
3657+
assert.strictEqual(exitCode, 1, 'exit code 1 when suite fails (resolution error, EFD)')
3658+
})
35593659
})
35603660

35613661
context('flaky test retries', () => {
@@ -5590,6 +5690,114 @@ describe(`jest@${JEST_VERSION} commonJS`, () => {
55905690
// - The always-failing test is quarantined
55915691
assert.strictEqual(exitCode, 0)
55925692
})
5693+
5694+
it('does not flip exit code to 0 when a test suite fails to parse', async () => {
5695+
receiver.setSettings({ test_management: { enabled: true } })
5696+
5697+
// Scenario: (1) test-suite-failed-to-run-parse.js fails to parse so no tests run,
5698+
// (2) test-quarantine-1.js parses and runs, its only failing test is quarantined.
5699+
receiver.setTestManagementTests({
5700+
jest: {
5701+
suites: {
5702+
'ci-visibility/test-management/test-quarantine-1.js': {
5703+
tests: {
5704+
'quarantine tests can quarantine a test': {
5705+
properties: {
5706+
quarantined: true,
5707+
},
5708+
},
5709+
},
5710+
},
5711+
},
5712+
},
5713+
})
5714+
5715+
const testAssertionsPromise = receiver
5716+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
5717+
const events = payloads.flatMap(({ payload }) => payload.events)
5718+
const testSession = events.find(event => event.type === 'test_session_end')?.content
5719+
assert.strictEqual(testSession.meta[TEST_STATUS], 'fail')
5720+
assert.strictEqual(testSession.meta[TEST_MANAGEMENT_ENABLED], 'true', 'test management should be running')
5721+
5722+
// TODO: parsing errors do not report test suite
5723+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
5724+
const quarantine1Tests = tests.filter(t => t.resource?.includes('test-quarantine-1'))
5725+
const withQuarantineTag = quarantine1Tests.filter(t => t.meta?.[TEST_MANAGEMENT_IS_QUARANTINED] === 'true')
5726+
assert.strictEqual(withQuarantineTag.length, 1, 'only one test from test-quarantine-1 has quarantine tag')
5727+
assert.strictEqual(withQuarantineTag[0].meta[TEST_MANAGEMENT_IS_QUARANTINED], 'true')
5728+
})
5729+
5730+
childProcess = exec(
5731+
runTestsCommand,
5732+
{
5733+
cwd,
5734+
env: {
5735+
...getCiVisAgentlessConfig(receiver.port),
5736+
TESTS_TO_RUN: 'test-management/(test-suite-failed-to-run-parse|test-quarantine-1)',
5737+
SHOULD_CHECK_RESULTS: '1',
5738+
},
5739+
}
5740+
)
5741+
5742+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), testAssertionsPromise])
5743+
assert.strictEqual(exitCode, 1, 'exit code should be 1 when a test suite fails to parse')
5744+
})
5745+
5746+
it('does not flip exit code to 0 when a test suite fails due to module resolution error', async () => {
5747+
receiver.setSettings({ test_management: { enabled: true } })
5748+
5749+
// Scenario: (1) test-suite-failed-to-run-resolution.js fails to load (invalid require),
5750+
// (2) test-quarantine-1.js parses and runs, its only failing test is quarantined.
5751+
receiver.setTestManagementTests({
5752+
jest: {
5753+
suites: {
5754+
'ci-visibility/test-management/test-quarantine-1.js': {
5755+
tests: {
5756+
'quarantine tests can quarantine a test': {
5757+
properties: {
5758+
quarantined: true,
5759+
},
5760+
},
5761+
},
5762+
},
5763+
},
5764+
},
5765+
})
5766+
5767+
const testAssertionsPromise = receiver
5768+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
5769+
const events = payloads.flatMap(({ payload }) => payload.events)
5770+
const testSession = events.find(event => event.type === 'test_session_end')?.content
5771+
assert.strictEqual(testSession.meta[TEST_STATUS], 'fail')
5772+
assert.strictEqual(testSession.meta[TEST_MANAGEMENT_ENABLED], 'true', 'test management should be running')
5773+
5774+
const suites = events.filter(event => event.type === 'test_suite_end').map(event => event.content)
5775+
const failedSuite = suites.find(s => s.meta?.[TEST_SUITE]?.includes('test-suite-failed-to-run-resolution'))
5776+
assert.ok(failedSuite, 'failing test suite should be reported')
5777+
assert.strictEqual(failedSuite.meta[TEST_STATUS], 'fail')
5778+
5779+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
5780+
const quarantine1Tests = tests.filter(t => t.resource?.includes('test-quarantine-1'))
5781+
const withQuarantineTag = quarantine1Tests.filter(t => t.meta?.[TEST_MANAGEMENT_IS_QUARANTINED] === 'true')
5782+
assert.strictEqual(withQuarantineTag.length, 1, 'only one test from test-quarantine-1 has quarantine tag')
5783+
assert.strictEqual(withQuarantineTag[0].meta[TEST_MANAGEMENT_IS_QUARANTINED], 'true')
5784+
})
5785+
5786+
childProcess = exec(
5787+
runTestsCommand,
5788+
{
5789+
cwd,
5790+
env: {
5791+
...getCiVisAgentlessConfig(receiver.port),
5792+
TESTS_TO_RUN: 'test-management/(test-suite-failed-to-run-resolution|test-quarantine-1)',
5793+
SHOULD_CHECK_RESULTS: '1',
5794+
},
5795+
}
5796+
)
5797+
5798+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), testAssertionsPromise])
5799+
assert.strictEqual(exitCode, 1, 'exit code 1 when suite fails (resolution error)')
5800+
})
55935801
})
55945802

55955803
it('does not crash if the request to get test management tests fails', async () => {

‎packages/datadog-instrumentations/src/jest.js‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -994,11 +994,18 @@ function getCliWrapper (isNewJestVersion) {
994994
coverageMap,
995995
numFailedTestSuites,
996996
numFailedTests,
997+
numRuntimeErrorTestSuites = 0,
997998
numTotalTests,
998999
numTotalTestSuites,
1000+
runExecError,
1001+
wasInterrupted,
9991002
},
10001003
} = result
10011004

1005+
const hasSuiteLevelFailures = numRuntimeErrorTestSuites > 0
1006+
const hasRunLevelFailure = runExecError != null || wasInterrupted === true
1007+
const mustNotFlipSuccess = hasSuiteLevelFailures || hasRunLevelFailure
1008+
10021009
let testCodeCoverageLinesTotal
10031010

10041011
if (isUserCodeCoverageEnabled) {
@@ -1026,7 +1033,11 @@ function getCliWrapper (isNewJestVersion) {
10261033
}
10271034
}
10281035
// If every test that failed was an EFD retry, we'll consider the suite passed
1029-
if (numEfdFailedTestsToIgnore !== 0 && result.results.numFailedTests === numEfdFailedTestsToIgnore) {
1036+
if (
1037+
!mustNotFlipSuccess &&
1038+
numEfdFailedTestsToIgnore !== 0 &&
1039+
result.results.numFailedTests === numEfdFailedTestsToIgnore
1040+
) {
10301041
result.results.success = true
10311042
}
10321043
}
@@ -1072,6 +1083,7 @@ function getCliWrapper (isNewJestVersion) {
10721083
// it's considered quarantined both if it's disabled and if it's quarantined
10731084
// (it'll run but its status is ignored)
10741085
if (
1086+
!mustNotFlipSuccess &&
10751087
(numFailedQuarantinedOrDisabledAttemptedToFixTests !== 0 || numFailedQuarantinedTests !== 0) &&
10761088
result.results.numFailedTests ===
10771089
numFailedQuarantinedTests + numFailedQuarantinedOrDisabledAttemptedToFixTests
@@ -1082,10 +1094,17 @@ function getCliWrapper (isNewJestVersion) {
10821094

10831095
// Combined check: if all failed tests are accounted for by EFD (flaky retries) and/or quarantine,
10841096
// we should consider the suite passed even when neither check alone covers all failures.
1085-
if (!result.results.success && (isEarlyFlakeDetectionEnabled || isTestManagementTestsEnabled)) {
1097+
if (
1098+
!result.results.success &&
1099+
!mustNotFlipSuccess &&
1100+
(isEarlyFlakeDetectionEnabled || isTestManagementTestsEnabled)
1101+
) {
10861102
const totalIgnoredFailures =
10871103
numEfdFailedTestsToIgnore + numFailedQuarantinedTests + numFailedQuarantinedOrDisabledAttemptedToFixTests
1088-
if (totalIgnoredFailures !== 0 && result.results.numFailedTests === totalIgnoredFailures) {
1104+
if (
1105+
totalIgnoredFailures !== 0 &&
1106+
result.results.numFailedTests === totalIgnoredFailures
1107+
) {
10891108
result.results.success = true
10901109
}
10911110
}

0 commit comments

Comments
 (0)