Skip to content

Commit 98ce21e

Browse files
juan-fernandezjuan-fernandez
authored
[test optimization] Fix quarantine + EFD (#7491)
1 parent 5de82d7 commit 98ce21e

File tree

7 files changed

+192
-10
lines changed

7 files changed

+192
-10
lines changed

‎integration-tests/ci-visibility/test-management/test-efd-and-quarantine.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
'use strict'
2+
3+
const assert = require('assert')
4+
5+
let globalCounter = 0
6+
7+
describe('efd and quarantine', () => {
8+
it('is a new flaky test', () => {
9+
// Passes on even attempts (0, 2, 4...), fails on odd (1, 3, 5...)
10+
assert.strictEqual((globalCounter++) % 2, 0)
11+
})
12+
13+
it('is a quarantined failing test', () => {
14+
assert.strictEqual(1 + 2, 4)
15+
})
16+
})

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5460,6 +5460,136 @@ describe(`jest@${JEST_VERSION} commonJS`, () => {
54605460
eventsPromise,
54615461
])
54625462
})
5463+
5464+
it('quarantine prevents session failure when ATR is also enabled', async () => {
5465+
receiver.setSettings({
5466+
test_management: { enabled: true },
5467+
flaky_test_retries_enabled: true,
5468+
})
5469+
5470+
const eventsPromise = receiver
5471+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
5472+
const events = payloads.flatMap(({ payload }) => payload.events)
5473+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
5474+
const testSession = events.find(event => event.type === 'test_session_end').content
5475+
5476+
// Session should pass because the only failing test is quarantined
5477+
assert.strictEqual(testSession.meta[TEST_STATUS], 'pass')
5478+
assert.strictEqual(testSession.meta[TEST_MANAGEMENT_ENABLED], 'true')
5479+
5480+
// All executions of the quarantined test should be tagged as quarantined
5481+
const quarantinedTests = tests.filter(
5482+
test => test.meta[TEST_NAME] === 'quarantine tests can quarantine a test'
5483+
)
5484+
assert.ok(quarantinedTests.length > 1, 'quarantined test should have been retried by ATR')
5485+
for (const test of quarantinedTests) {
5486+
assert.strictEqual(test.meta[TEST_MANAGEMENT_IS_QUARANTINED], 'true')
5487+
}
5488+
5489+
// The last execution should have final_status = skip
5490+
const lastExecution = quarantinedTests[quarantinedTests.length - 1]
5491+
assert.strictEqual(lastExecution.meta[TEST_FINAL_STATUS], 'skip')
5492+
})
5493+
5494+
childProcess = exec(
5495+
runTestsCommand,
5496+
{
5497+
cwd,
5498+
env: {
5499+
...getCiVisAgentlessConfig(receiver.port),
5500+
TESTS_TO_RUN: 'test-management/test-quarantine-1',
5501+
},
5502+
}
5503+
)
5504+
5505+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), eventsPromise])
5506+
5507+
// Exit code should be 0 because the failing test is quarantined
5508+
assert.strictEqual(exitCode, 0)
5509+
})
5510+
5511+
it('session passes when EFD flaky retries and quarantine failures are combined', async () => {
5512+
const NUM_RETRIES_EFD = 3
5513+
5514+
// The new flaky test is NOT in known tests so EFD will retry it
5515+
receiver.setKnownTests({ jest: {} })
5516+
5517+
receiver.setSettings({
5518+
test_management: { enabled: true },
5519+
early_flake_detection: {
5520+
enabled: true,
5521+
slow_test_retries: { '5s': NUM_RETRIES_EFD },
5522+
faulty_session_threshold: 100,
5523+
},
5524+
known_tests_enabled: true,
5525+
})
5526+
5527+
// Only quarantine the always-failing test
5528+
receiver.setTestManagementTests({
5529+
jest: {
5530+
suites: {
5531+
'ci-visibility/test-management/test-efd-and-quarantine.js': {
5532+
tests: {
5533+
'efd and quarantine is a quarantined failing test': {
5534+
properties: {
5535+
quarantined: true,
5536+
},
5537+
},
5538+
},
5539+
},
5540+
},
5541+
},
5542+
})
5543+
5544+
const eventsPromise = receiver
5545+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
5546+
const events = payloads.flatMap(({ payload }) => payload.events)
5547+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
5548+
const testSession = events.find(event => event.type === 'test_session_end').content
5549+
5550+
// Session should pass:
5551+
// - The new flaky test has at least one passing EFD retry (so EFD can ignore its failures)
5552+
// - The quarantined test is quarantined (so quarantine can ignore its failure)
5553+
assert.strictEqual(testSession.meta[TEST_STATUS], 'pass')
5554+
5555+
// Verify the quarantined test is tagged
5556+
const quarantinedTests = tests.filter(
5557+
test => test.meta[TEST_NAME] === 'efd and quarantine is a quarantined failing test'
5558+
)
5559+
assert.ok(quarantinedTests.length >= 1)
5560+
for (const test of quarantinedTests) {
5561+
assert.strictEqual(test.meta[TEST_MANAGEMENT_IS_QUARANTINED], 'true')
5562+
}
5563+
5564+
// Verify the new flaky test has EFD retries (at least original + retries)
5565+
const flakyTests = tests.filter(
5566+
test => test.meta[TEST_NAME] === 'efd and quarantine is a new flaky test'
5567+
)
5568+
assert.ok(flakyTests.length > 1, 'flaky test should have been retried by EFD')
5569+
5570+
// At least one EFD retry should have passed
5571+
const passingFlakyTests = flakyTests.filter(t => t.meta[TEST_STATUS] === 'pass')
5572+
assert.ok(passingFlakyTests.length > 0, 'at least one EFD retry should pass')
5573+
})
5574+
5575+
childProcess = exec(
5576+
runTestsCommand,
5577+
{
5578+
cwd,
5579+
env: {
5580+
...getCiVisAgentlessConfig(receiver.port),
5581+
TESTS_TO_RUN: 'test-management/test-efd-and-quarantine',
5582+
},
5583+
}
5584+
)
5585+
5586+
const [[exitCode]] = await Promise.all([once(childProcess, 'exit'), eventsPromise])
5587+
5588+
// Exit code should be 0 because:
5589+
// - The flaky test has at least one passing retry (EFD considers it OK)
5590+
// - The always-failing test is quarantined
5591+
assert.strictEqual(exitCode, 0)
5592+
})
54635593
})
54645594

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

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3874,8 +3874,11 @@ describe(`mocha@${MOCHA_VERSION}`, function () {
38743874

38753875
if (isQuarantining) {
38763876
assert.strictEqual(testSession.meta[TEST_MANAGEMENT_ENABLED], 'true')
3877+
// Session should pass because the only failing test is quarantined
3878+
assert.strictEqual(testSession.meta[TEST_STATUS], 'pass')
38773879
} else {
38783880
assert.ok(!(TEST_MANAGEMENT_ENABLED in testSession.meta))
3881+
assert.strictEqual(testSession.meta[TEST_STATUS], 'fail')
38793882
}
38803883

38813884
const resourceNames = tests.map(span => span.resource)

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

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const {
1616
isModifiedTest,
1717
} = require('../../dd-trace/src/plugins/util/test')
1818
const {
19+
SEED_SUFFIX_RE,
1920
getFormattedJestTestParameters,
2021
getJestTestName,
2122
getJestSuitesToRun,
@@ -55,6 +56,7 @@ const itrSkippedSuitesCh = channel('ci:jest:itr:skipped-suites')
5556
// Message sent by jest's main process to workers to run a test suite (=test file)
5657
// https://github.com/jestjs/jest/blob/1d682f21c7a35da4d3ab3a1436a357b980ebd0fa/packages/jest-worker/src/types.ts#L37
5758
const CHILD_MESSAGE_CALL = 1
59+
5860
// Maximum time we'll wait for the tracer to flush
5961
const FLUSH_TIMEOUT = 10_000
6062

@@ -955,7 +957,7 @@ function getCliWrapper (isNewJestVersion) {
955957
isTestManagementTestsEnabled = false
956958
testManagementTests = {}
957959
} else {
958-
testManagementTests = receivedTestManagementTests
960+
testManagementTests = receivedTestManagementTests || {}
959961
}
960962
} catch (err) {
961963
log.error('Jest test management tests error', err)
@@ -1013,33 +1015,39 @@ function getCliWrapper (isNewJestVersion) {
10131015
* The rationale behind is the following: you may still be able to block your CI pipeline by gating
10141016
* on flakiness (the test will be considered flaky), but you may choose to unblock the pipeline too.
10151017
*/
1018+
let numEfdFailedTestsToIgnore = 0
10161019
if (isEarlyFlakeDetectionEnabled) {
1017-
let numFailedTestsToIgnore = 0
10181020
for (const testStatuses of newTestsTestStatuses.values()) {
10191021
const { pass, fail } = getTestStats(testStatuses)
10201022
if (pass > 0) { // as long as one passes, we'll consider the test passed
1021-
numFailedTestsToIgnore += fail
1023+
numEfdFailedTestsToIgnore += fail
10221024
}
10231025
}
10241026
// If every test that failed was an EFD retry, we'll consider the suite passed
1025-
if (numFailedTestsToIgnore !== 0 && result.results.numFailedTests === numFailedTestsToIgnore) {
1027+
if (numEfdFailedTestsToIgnore !== 0 && result.results.numFailedTests === numEfdFailedTestsToIgnore) {
10261028
result.results.success = true
10271029
}
10281030
}
10291031

1032+
let numFailedQuarantinedTests = 0
1033+
let numFailedQuarantinedOrDisabledAttemptedToFixTests = 0
10301034
if (isTestManagementTestsEnabled) {
10311035
const failedTests = result
10321036
.results
10331037
.testResults.flatMap(({ testResults, testFilePath: testSuiteAbsolutePath }) => (
10341038
testResults.map(({ fullName: testName, status }) => (
1035-
{ testName, testSuiteAbsolutePath, status }
1039+
{
1040+
// Strip @fast-check/jest seed suffix so the name matches what was reported via TEST_NAME
1041+
testName: testSuiteAbsolutePathsWithFastCheck.has(testSuiteAbsolutePath)
1042+
? testName.replace(SEED_SUFFIX_RE, '')
1043+
: testName,
1044+
testSuiteAbsolutePath,
1045+
status,
1046+
}
10361047
))
10371048
))
10381049
.filter(({ status }) => status === 'failed')
10391050

1040-
let numFailedQuarantinedTests = 0
1041-
let numFailedQuarantinedOrDisabledAttemptedToFixTests = 0
1042-
10431051
for (const { testName, testSuiteAbsolutePath } of failedTests) {
10441052
const testSuite = getTestSuitePath(testSuiteAbsolutePath, result.globalConfig.rootDir)
10451053
const testManagementTest = testManagementTests
@@ -1070,6 +1078,16 @@ function getCliWrapper (isNewJestVersion) {
10701078
}
10711079
}
10721080

1081+
// Combined check: if all failed tests are accounted for by EFD (flaky retries) and/or quarantine,
1082+
// we should consider the suite passed even when neither check alone covers all failures.
1083+
if (!result.results.success && (isEarlyFlakeDetectionEnabled || isTestManagementTestsEnabled)) {
1084+
const totalIgnoredFailures =
1085+
numEfdFailedTestsToIgnore + numFailedQuarantinedTests + numFailedQuarantinedOrDisabledAttemptedToFixTests
1086+
if (totalIgnoredFailures !== 0 && result.results.numFailedTests === totalIgnoredFailures) {
1087+
result.results.success = true
1088+
}
1089+
}
1090+
10731091
// Determine session status after EFD and quarantine checks have potentially modified success
10741092
let status, error
10751093
if (result.results.success) {

‎packages/datadog-instrumentations/src/mocha/main.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,15 @@ function getOnEndHandler (isParallel) {
165165
this.failures -= numFailedQuarantinedTests + numFailedRetriedQuarantinedOrDisabledTests
166166
}
167167

168+
// Recompute status after EFD and quarantine adjustments have reduced failure counts
169+
if (status === 'fail') {
170+
if (this.stats) {
171+
status = this.stats.failures === 0 ? 'pass' : 'fail'
172+
} else {
173+
status = this.failures === 0 ? 'pass' : 'fail'
174+
}
175+
}
176+
168177
if (status === 'fail') {
169178
error = new Error(`Failed tests: ${this.failures}.`)
170179
}

‎packages/datadog-plugin-jest/src/util.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,4 +166,10 @@ function getJestSuitesToRun (skippableSuites, originalTests, rootDir) {
166166
}
167167
}
168168

169-
module.exports = { getFormattedJestTestParameters, getJestTestName, getJestSuitesToRun, isMarkedAsUnskippable }
169+
module.exports = {
170+
SEED_SUFFIX_RE,
171+
getFormattedJestTestParameters,
172+
getJestTestName,
173+
getJestSuitesToRun,
174+
isMarkedAsUnskippable,
175+
}

‎packages/dd-trace/src/plugins/util/test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ function getTestSuitePath (testSuiteAbsolutePath, sourceRoot) {
573573
? testSuiteAbsolutePath
574574
: path.relative(sourceRoot, testSuiteAbsolutePath)
575575

576-
return testSuitePath.replace(path.sep, '/')
576+
return testSuitePath.replaceAll(path.sep, '/')
577577
}
578578

579579
const POSSIBLE_CODEOWNERS_LOCATIONS = [

0 commit comments

Comments
 (0)