Skip to content

Commit c71f24e

Browse files
juan-fernandezjuan-fernandez
committed
[test optimization] Better nomenclature usage in impacted tests logic (#6614)
1 parent 3cb2897 commit c71f24e

File tree

16 files changed

+117
-133
lines changed

16 files changed

+117
-133
lines changed

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

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2707,8 +2707,7 @@ versions.forEach(version => {
27072707
}
27082708
})
27092709

2710-
const runImpactedTest = (
2711-
done,
2710+
const runImpactedTest = async (
27122711
{ isModified, isEfd, isParallel, isNew },
27132712
extraEnvVars = {}
27142713
) => {
@@ -2731,45 +2730,46 @@ versions.forEach(version => {
27312730
}
27322731
)
27332732

2734-
childProcess.on('exit', (code) => {
2735-
testAssertionsPromise.then(done).catch(done)
2736-
})
2733+
await Promise.all([
2734+
once(childProcess, 'exit'),
2735+
testAssertionsPromise
2736+
])
27372737
}
27382738

27392739
context('test is not new', () => {
2740-
it('should be detected as impacted', (done) => {
2740+
it('should be detected as impacted', async () => {
27412741
receiver.setSettings({ impacted_tests_enabled: true })
27422742

2743-
runImpactedTest(done, { isModified: true })
2743+
await runImpactedTest({ isModified: true })
27442744
})
27452745

2746-
it('should not be detected as impacted if disabled', (done) => {
2746+
it('should not be detected as impacted if disabled', async () => {
27472747
receiver.setSettings({ impacted_tests_enabled: false })
27482748

2749-
runImpactedTest(done, { isModified: false })
2749+
await runImpactedTest({ isModified: false })
27502750
})
27512751

27522752
it('should not be detected as impacted if DD_CIVISIBILITY_IMPACTED_TESTS_DETECTION_ENABLED is false',
2753-
(done) => {
2753+
async () => {
27542754
receiver.setSettings({ impacted_tests_enabled: true })
27552755

2756-
runImpactedTest(done,
2756+
await runImpactedTest(
27572757
{ isModified: false },
27582758
{ DD_CIVISIBILITY_IMPACTED_TESTS_DETECTION_ENABLED: '0' }
27592759
)
27602760
})
27612761

27622762
if (version !== '7.0.0') {
2763-
it('can detect impacted tests in parallel mode', (done) => {
2763+
it('can detect impacted tests in parallel mode', async () => {
27642764
receiver.setSettings({ impacted_tests_enabled: true })
27652765

2766-
runImpactedTest(done, { isModified: true, isParallel: true })
2766+
await runImpactedTest({ isModified: true, isParallel: true })
27672767
})
27682768
}
27692769
})
27702770

27712771
context('test is new', () => {
2772-
it('should be retried and marked both as new and modified', (done) => {
2772+
it('should be retried and marked both as new and modified', async () => {
27732773
receiver.setKnownTests({
27742774
cucumber: {}
27752775
})
@@ -2784,7 +2784,7 @@ versions.forEach(version => {
27842784
},
27852785
known_tests_enabled: true
27862786
})
2787-
runImpactedTest(done, { isModified: true, isEfd: true, isNew: true })
2787+
await runImpactedTest({ isModified: true, isEfd: true, isNew: true })
27882788
})
27892789
})
27902790
})

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ const skippableSuitesCh = channel('ci:cucumber:test-suite:skippable')
2525
const sessionStartCh = channel('ci:cucumber:session:start')
2626
const sessionFinishCh = channel('ci:cucumber:session:finish')
2727
const testManagementTestsCh = channel('ci:cucumber:test-management-tests')
28-
const impactedTestsCh = channel('ci:cucumber:modified-tests')
28+
const modifiedFilesCh = channel('ci:cucumber:modified-files')
2929
const isModifiedCh = channel('ci:cucumber:is-modified-test')
3030

3131
const workerReportTraceCh = channel('ci:cucumber:worker-report:trace')
@@ -80,7 +80,7 @@ let isTestManagementTestsEnabled = false
8080
let isImpactedTestsEnabled = false
8181
let testManagementAttemptToFixRetries = 0
8282
let testManagementTests = {}
83-
let modifiedTests = {}
83+
let modifiedFiles = {}
8484
let numTestRetries = 0
8585
let knownTests = {}
8686
let skippedSuites = []
@@ -537,9 +537,9 @@ function getWrappedStart (start, frameworkVersion, isParallel = false, isCoordin
537537
}
538538

539539
if (isImpactedTestsEnabled) {
540-
const impactedTestsResponse = await getChannelPromise(impactedTestsCh)
540+
const impactedTestsResponse = await getChannelPromise(modifiedFilesCh)
541541
if (!impactedTestsResponse.err) {
542-
modifiedTests = impactedTestsResponse.modifiedTests
542+
modifiedFiles = impactedTestsResponse.modifiedFiles
543543
}
544544
}
545545

@@ -655,7 +655,7 @@ function getWrappedRunTestCase (runTestCaseFunction, isNewerCucumberVersion = fa
655655
isModifiedCh.publish({
656656
scenarios,
657657
testFileAbsolutePath: gherkinDocument.uri,
658-
modifiedTests,
658+
modifiedFiles,
659659
stepIds,
660660
stepDefinitions: this.supportCodeLibrary.stepDefinitions,
661661
setIsModified
@@ -1000,7 +1000,7 @@ addHook({
10001000

10011001
if (isImpactedTestsEnabled) {
10021002
this.options.worldParameters._ddImpactedTestsEnabled = isImpactedTestsEnabled
1003-
this.options.worldParameters._ddModifiedTests = modifiedTests
1003+
this.options.worldParameters._ddModifiedFiles = modifiedFiles
10041004
}
10051005

10061006
return startWorker.apply(this, arguments)
@@ -1036,7 +1036,7 @@ addHook({
10361036
}
10371037
isImpactedTestsEnabled = !!this.options.worldParameters._ddImpactedTestsEnabled
10381038
if (isImpactedTestsEnabled) {
1039-
modifiedTests = this.options.worldParameters._ddModifiedTests
1039+
modifiedFiles = this.options.worldParameters._ddModifiedFiles
10401040
}
10411041
}
10421042
)

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

Lines changed: 13 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ const skippableSuitesCh = channel('ci:jest:test-suite:skippable')
4646
const libraryConfigurationCh = channel('ci:jest:library-configuration')
4747
const knownTestsCh = channel('ci:jest:known-tests')
4848
const testManagementTestsCh = channel('ci:jest:test-management-tests')
49-
const impactedTestsCh = channel('ci:jest:modified-tests')
49+
const modifiedFilesCh = channel('ci:jest:modified-files')
5050

5151
const itrSkippedSuitesCh = channel('ci:jest:itr:skipped-suites')
5252

@@ -78,7 +78,7 @@ let isTestManagementTestsEnabled = false
7878
let testManagementTests = {}
7979
let testManagementAttemptToFixRetries = 0
8080
let isImpactedTestsEnabled = false
81-
let modifiedTests = {}
81+
let modifiedFiles = {}
8282

8383
const testContexts = new WeakMap()
8484
const originalTestFns = new WeakMap()
@@ -197,10 +197,8 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) {
197197

198198
if (this.isImpactedTestsEnabled) {
199199
try {
200-
const hasImpactedTests = Object.keys(modifiedTests).length > 0
201-
this.modifiedTestsForThisSuite = hasImpactedTests
202-
? this.getModifiedTestForThisSuite(modifiedTests)
203-
: this.getModifiedTestForThisSuite(this.testEnvironmentOptions._ddModifiedTests)
200+
const hasImpactedTests = Object.keys(modifiedFiles).length > 0
201+
this.modifiedFiles = hasImpactedTests ? modifiedFiles : this.testEnvironmentOptions._ddModifiedFiles
204202
} catch (e) {
205203
log.error('Error parsing impacted tests', e)
206204
this.isImpactedTestsEnabled = false
@@ -290,19 +288,6 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) {
290288
return result
291289
}
292290

293-
getModifiedTestForThisSuite (modifiedTests) {
294-
if (this.modifiedTestsForThisSuite) {
295-
return this.modifiedTestsForThisSuite
296-
}
297-
let modifiedTestsForThisSuite = modifiedTests
298-
// If jest is using workers, modified tests are serialized to json.
299-
// If jest runs in band, they are not.
300-
if (typeof modifiedTestsForThisSuite === 'string') {
301-
modifiedTestsForThisSuite = JSON.parse(modifiedTestsForThisSuite)
302-
}
303-
return modifiedTestsForThisSuite
304-
}
305-
306291
// Generic function to handle test retries
307292
retryTest ({
308293
jestEvent,
@@ -378,7 +363,7 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) {
378363
this.testSourceFile,
379364
testStartLine,
380365
testEndLine,
381-
this.modifiedTestsForThisSuite,
366+
this.modifiedFiles,
382367
'jest'
383368
)
384369
}
@@ -465,7 +450,7 @@ function getWrappedEnvironment (BaseEnvironment, jestVersion) {
465450
this.testSourceFile,
466451
testStartLine,
467452
testEndLine,
468-
this.modifiedTestsForThisSuite,
453+
this.modifiedFiles,
469454
'jest'
470455
)
471456
if (isModified && !retriedTestsToNumAttempts.has(testFullName) && this.isEarlyFlakeDetectionEnabled) {
@@ -831,12 +816,12 @@ function getCliWrapper (isNewJestVersion) {
831816
onDone = resolve
832817
})
833818

834-
impactedTestsCh.publish({ onDone })
819+
modifiedFilesCh.publish({ onDone })
835820

836821
try {
837-
const { err, modifiedTests: receivedModifiedTests } = await impactedTestsPromise
822+
const { err, modifiedFiles: receivedModifiedFiles } = await impactedTestsPromise
838823
if (!err) {
839-
modifiedTests = receivedModifiedTests
824+
modifiedFiles = receivedModifiedFiles
840825
}
841826
} catch (err) {
842827
log.error('Jest impacted tests error', err)
@@ -1237,7 +1222,7 @@ addHook({
12371222
_ddIsTestManagementTestsEnabled,
12381223
_ddTestManagementTests,
12391224
_ddTestManagementAttemptToFixRetries,
1240-
_ddModifiedTests,
1225+
_ddModifiedFiles,
12411226
...restOfTestEnvironmentOptions
12421227
} = testEnvironmentOptions
12431228

@@ -1365,17 +1350,15 @@ function sendWrapper (send) {
13651350

13661351
const suiteTestManagementTests = testManagementTests?.jest?.suites?.[testSuite]?.tests || {}
13671352

1368-
const suiteModifiedTests = Object.keys(modifiedTests).length > 0
1369-
? modifiedTests
1370-
: {}
1371-
13721353
args[0].config = {
13731354
...config,
13741355
testEnvironmentOptions: {
13751356
...config.testEnvironmentOptions,
13761357
_ddKnownTests: suiteKnownTests,
13771358
_ddTestManagementTests: suiteTestManagementTests,
1378-
_ddModifiedTests: suiteModifiedTests
1359+
// TODO: figure out if we can reduce the size of the modified files object
1360+
// Can we use `testSuite` (it'd have to be relative to repository root though)
1361+
_ddModifiedFiles: modifiedFiles
13791362
}
13801363
}
13811364
}

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

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ const skippableSuitesCh = channel('ci:mocha:test-suite:skippable')
6868
const mochaGlobalRunCh = channel('ci:mocha:global:run')
6969

7070
const testManagementTestsCh = channel('ci:mocha:test-management-tests')
71-
const impactedTestsCh = channel('ci:mocha:modified-tests')
71+
const modifiedFilesCh = channel('ci:mocha:modified-files')
7272
const workerReportTraceCh = channel('ci:mocha:worker-report:trace')
7373
const testSessionStartCh = channel('ci:mocha:session:start')
7474
const testSessionFinishCh = channel('ci:mocha:session:finish')
@@ -233,12 +233,12 @@ function getExecutionConfiguration (runner, isParallel, frameworkVersion, onFini
233233
})
234234
}
235235

236-
const onReceivedImpactedTests = ({ err, modifiedTests: receivedModifiedTests }) => {
236+
const onReceivedImpactedTests = ({ err, modifiedFiles: receivedModifiedFiles }) => {
237237
if (err) {
238-
config.modifiedTests = []
238+
config.modifiedFiles = []
239239
config.isImpactedTestsEnabled = false
240240
} else {
241-
config.modifiedTests = receivedModifiedTests
241+
config.modifiedFiles = receivedModifiedFiles
242242
}
243243
if (config.isSuitesSkippingEnabled) {
244244
ctx.onDone = onReceivedSkippableSuites
@@ -260,7 +260,7 @@ function getExecutionConfiguration (runner, isParallel, frameworkVersion, onFini
260260
}
261261
if (config.isImpactedTestsEnabled) {
262262
ctx.onDone = onReceivedImpactedTests
263-
impactedTestsCh.runStores(ctx, () => {})
263+
modifiedFilesCh.runStores(ctx, () => {})
264264
} else if (config.isSuitesSkippingEnabled) {
265265
ctx.onDone = onReceivedSkippableSuites
266266
skippableSuitesCh.runStores(ctx, () => {})
@@ -284,7 +284,7 @@ function getExecutionConfiguration (runner, isParallel, frameworkVersion, onFini
284284
testManagementTestsCh.runStores(ctx, () => {})
285285
} if (config.isImpactedTestsEnabled) {
286286
ctx.onDone = onReceivedImpactedTests
287-
impactedTestsCh.runStores(ctx, () => {})
287+
modifiedFilesCh.runStores(ctx, () => {})
288288
} else if (config.isSuitesSkippingEnabled) {
289289
ctx.onDone = onReceivedSkippableSuites
290290
skippableSuitesCh.runStores(ctx, () => {})
@@ -321,7 +321,7 @@ function getExecutionConfiguration (runner, isParallel, frameworkVersion, onFini
321321
testManagementTestsCh.runStores(ctx, () => {})
322322
} else if (config.isImpactedTestsEnabled) {
323323
ctx.onDone = onReceivedImpactedTests
324-
impactedTestsCh.runStores(ctx, () => {})
324+
modifiedFilesCh.runStores(ctx, () => {})
325325
} else if (config.isSuitesSkippingEnabled) {
326326
ctx.onDone = onReceivedSkippableSuites
327327
skippableSuitesCh.runStores(ctx, () => {})
@@ -696,9 +696,8 @@ addHook({
696696
}
697697

698698
if (config.isImpactedTestsEnabled) {
699-
const testSuiteImpactedTests = config.modifiedTests || {}
700699
newWorkerArgs._ddIsImpactedTestsEnabled = true
701-
newWorkerArgs._ddModifiedTests = testSuiteImpactedTests
700+
newWorkerArgs._ddModifiedFiles = config.modifiedFiles || {}
702701
}
703702

704703
// We pass the known tests for the test file to the worker

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ function getRunTestsWrapper (runTests, config) {
446446
if (config.isImpactedTestsEnabled) {
447447
suite.tests.forEach((test) => {
448448
isModifiedCh.publish({
449-
modifiedTests: config.modifiedTests,
449+
modifiedFiles: config.modifiedFiles,
450450
file: suite.file,
451451
onDone: (isModified) => {
452452
if (isModified) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ addHook({
3636
}
3737
if (this.options._ddIsImpactedTestsEnabled) {
3838
config.isImpactedTestsEnabled = true
39-
config.modifiedTests = this.options._ddModifiedTests
39+
config.modifiedFiles = this.options._ddModifiedFiles
4040
delete this.options._ddIsImpactedTestsEnabled
41-
delete this.options._ddModifiedTests
41+
delete this.options._ddModifiedFiles
4242
}
4343
if (this.options._ddIsTestManagementTestsEnabled) {
4444
config.isTestManagementTestsEnabled = true

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ const testSessionFinishCh = channel('ci:playwright:session:finish')
2121
const libraryConfigurationCh = channel('ci:playwright:library-configuration')
2222
const knownTestsCh = channel('ci:playwright:known-tests')
2323
const testManagementTestsCh = channel('ci:playwright:test-management-tests')
24-
const impactedTestsCh = channel('ci:playwright:modified-tests')
24+
const modifiedFilesCh = channel('ci:playwright:modified-files')
2525
const isModifiedCh = channel('ci:playwright:test:is-modified')
2626

2727
const testSuiteStartCh = channel('ci:playwright:test-suite:start')
@@ -59,7 +59,7 @@ let isTestManagementTestsEnabled = false
5959
let testManagementAttemptToFixRetries = 0
6060
let testManagementTests = {}
6161
let isImpactedTestsEnabled = false
62-
let modifiedTests = {}
62+
let modifiedFiles = {}
6363
const quarantinedOrDisabledTestsAttemptToFix = []
6464
let quarantinedButNotAttemptToFixFqns = new Set()
6565
let rootDir = ''
@@ -593,11 +593,11 @@ function runAllTestsWrapper (runAllTests, playwrightVersion) {
593593

594594
if (isImpactedTestsEnabled && satisfies(playwrightVersion, MINIMUM_SUPPORTED_VERSION_RANGE_EFD)) {
595595
try {
596-
const { err, modifiedTests: receivedModifiedTests } = await getChannelPromise(impactedTestsCh)
596+
const { err, modifiedFiles: receivedModifiedFiles } = await getChannelPromise(modifiedFilesCh)
597597
if (err) {
598598
isImpactedTestsEnabled = false
599599
} else {
600-
modifiedTests = receivedModifiedTests
600+
modifiedFiles = receivedModifiedFiles
601601
}
602602
} catch (err) {
603603
isImpactedTestsEnabled = false
@@ -819,7 +819,7 @@ addHook({
819819
await Promise.all(allTests.map(async (test) => {
820820
const { isModified } = await getChannelPromise(isModifiedCh, {
821821
filePath: test._requireFile,
822-
modifiedTests
822+
modifiedFiles
823823
})
824824
if (isModified) {
825825
test._ddIsModified = true

0 commit comments

Comments
 (0)