[test optimization] Fix retry logic in impacted tests for playwright (#6755)

Author: juan-fernandez

integration-tests/ci-visibility/playwright-tests-impacted-tests/impacted-test.js

@@ -13,3 +13,11 @@ test.describe('impacted test', () => {
     ])
   })
 })
+
+test.describe('impacted test 2', () => {
+  test('should be impacted 2', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})

integration-tests/playwright/playwright.spec.js

@@ -1740,7 +1740,8 @@ versions.forEach((version) => {
       beforeEach(() => {
         receiver.setKnownTests({
           playwright: {
-            'ci-visibility/playwright-tests-impacted-tests/impacted-test.js': ['impacted test should be impacted']
+            'ci-visibility/playwright-tests-impacted-tests/impacted-test.js':
+              ['impacted test should be impacted', 'impacted test 2 should be impacted 2']
           }
         })
       })
@@ -1762,6 +1763,13 @@ versions.forEach((version) => {
                  'Hello Worldd'
                ])
              })
+           })
+           test.describe('impacted test 2', () => {
+             test('should be impacted 2', async ({ page }) => {
+               await expect(page.locator('.hello-world')).toHaveText([
+                 'Hello World'
+               ])
+             })
            })`
         )
         execSync('git add ci-visibility/playwright-tests-impacted-tests/impacted-test.js', { cwd, stdio: 'ignore' })
@@ -1790,18 +1798,18 @@ versions.forEach((version) => {
 
             assert.includeMembers(resourceNames,
               [
-                'impacted-test.js.impacted test should be impacted'
+                'impacted-test.js.impacted test should be impacted',
+                'impacted-test.js.impacted test 2 should be impacted 2'
               ]
             )
 
             const impactedTests = tests.filter(test =>
-              test.meta[TEST_SOURCE_FILE] === 'ci-visibility/playwright-tests-impacted-tests/impacted-test.js' &&
-              test.meta[TEST_NAME] === 'impacted test should be impacted')
+              test.meta[TEST_SOURCE_FILE] === 'ci-visibility/playwright-tests-impacted-tests/impacted-test.js')
 
             if (isEfd) {
-              assert.equal(impactedTests.length, NUM_RETRIES_EFD + 1) // Retries + original test
+              assert.equal(impactedTests.length, (NUM_RETRIES_EFD + 1) * 2) // Retries + original test
             } else {
-              assert.equal(impactedTests.length, 1)
+              assert.equal(impactedTests.length, 2)
             }
 
             for (const impactedTest of impactedTests) {
@@ -1819,10 +1827,9 @@ versions.forEach((version) => {
 
             if (isEfd) {
               const retriedTests = tests.filter(
-                test => test.meta[TEST_IS_RETRY] === 'true' &&
-                test.meta[TEST_NAME] === 'impacted test should be impacted'
+                test => test.meta[TEST_IS_RETRY] === 'true'
               )
-              assert.equal(retriedTests.length, NUM_RETRIES_EFD)
+              assert.equal(retriedTests.length, NUM_RETRIES_EFD * 2)
               let retriedTestNew = 0
               let retriedTestsWithReason = 0
               retriedTests.forEach(test => {
@@ -1833,13 +1840,12 @@ versions.forEach((version) => {
                   retriedTestsWithReason++
                 }
               })
-              assert.equal(retriedTestNew, isNew ? NUM_RETRIES_EFD : 0)
-              assert.equal(retriedTestsWithReason, NUM_RETRIES_EFD)
+              assert.equal(retriedTestNew, isNew ? NUM_RETRIES_EFD * 2 : 0)
+              assert.equal(retriedTestsWithReason, NUM_RETRIES_EFD * 2)
             }
           }, 25000)
 
-      const runImpactedTest = (
-        done,
+      const runImpactedTest = async (
         { isModified, isEfd = false, isNew = false },
         extraEnvVars = {}
       ) => {
@@ -1860,37 +1866,38 @@ versions.forEach((version) => {
           }
         )
 
-        childProcess.on('exit', () => {
-          testAssertionsPromise.then(done).catch(done)
-        })
+        await Promise.all([
+          once(childProcess, 'exit'),
+          testAssertionsPromise
+        ])
       }
 
       context('test is not new', () => {
-        it('should be detected as impacted', (done) => {
+        it('should be detected as impacted', async () => {
           receiver.setSettings({ impacted_tests_enabled: true })
 
-          runImpactedTest(done, { isModified: true })
+          await runImpactedTest({ isModified: true })
         })
 
-        it('should not be detected as impacted if disabled', (done) => {
+        it('should not be detected as impacted if disabled', async () => {
           receiver.setSettings({ impacted_tests_enabled: false })
 
-          runImpactedTest(done, { isModified: false })
+          await runImpactedTest({ isModified: false })
         })
 
         it('should not be detected as impacted if DD_CIVISIBILITY_IMPACTED_TESTS_DETECTION_ENABLED is false',
-          (done) => {
+          async () => {
             receiver.setSettings({ impacted_tests_enabled: true })
 
-            runImpactedTest(done,
+            await runImpactedTest(
               { isModified: false },
               { DD_CIVISIBILITY_IMPACTED_TESTS_DETECTION_ENABLED: '0' }
             )
           })
       })
 
       context('test is new', () => {
-        it('should be retried and marked both as new and modified', (done) => {
+        it('should be retried and marked both as new and modified', async () => {
           receiver.setKnownTests({
             playwright: {}
           })
@@ -1904,8 +1911,7 @@ versions.forEach((version) => {
             },
             known_tests_enabled: true
           })
-          runImpactedTest(
-            done,
+          await runImpactedTest(
             { isModified: true, isEfd: true, isNew: true }
           )
         })

packages/datadog-instrumentations/src/playwright.js

@@ -113,8 +113,10 @@ function deepCloneSuite (suite, filterTest, tags = []) {
       if (filterTest(entry)) {
         const copiedTest = entry._clone()
         tags.forEach(tag => {
-          if (tag) {
-            copiedTest[tag] = true
+          const resolvedTag = typeof tag === 'function' ? tag(entry) : tag
+
+          if (resolvedTag) {
+            copiedTest[resolvedTag] = true
           }
         })
         copy._addTest(copiedTest)
@@ -764,6 +766,30 @@ addHook({
   return suiteUtilsPackage
 })
 
+/**
+ * We could repeat the logic of `applyRepeatEachIndex` here, but it'd be more risky
+ * as playwright could change it at any time.
+ *
+ * `applyRepeatEachIndex` goes through all the tests in a suite and applies the "repeat" logic
+ * for a single repeat index.
+ *
+ * This means that the clone logic is cumbersome:
+ * - we grab the unique file suites that have new tests
+ * - we store its project suite
+ * - we clone each of these file suites for each repeat index
+ * - we execute `applyRepeatEachIndex` for each of these cloned file suites
+ * - we add the cloned file suites to the project suite
+ */
+function applyRetriesToTests (fileSuitesWithTestsToRetry, filterTest, tagsToApply) {
+  for (const [fileSuite, projectSuite] of fileSuitesWithTestsToRetry.entries()) {
+    for (let repeatEachIndex = 1; repeatEachIndex <= earlyFlakeDetectionNumRetries; repeatEachIndex++) {
+      const copyFileSuite = deepCloneSuite(fileSuite, filterTest, tagsToApply)
+      applyRepeatEachIndex(projectSuite._fullProject, copyFileSuite, repeatEachIndex + 1)
+      projectSuite._addSuite(copyFileSuite)
+    }
+  }
+}
+
 addHook({
   name: 'playwright',
   file: 'lib/runner/loadUtils.js',
@@ -819,31 +845,36 @@ addHook({
     }
 
     if (isImpactedTestsEnabled) {
-      await Promise.all(allTests.map(async (test) => {
-        const { isModified } = await getChannelPromise(isModifiedCh, {
+      const impactedTests = allTests.filter(test => {
+        let isImpacted = false
+        isModifiedCh.publish({
           filePath: test._requireFile,
-          modifiedFiles
+          modifiedFiles,
+          onDone: (isModified) => { isImpacted = isModified }
         })
-        if (isModified) {
-          test._ddIsModified = true
-        }
-        if (isEarlyFlakeDetectionEnabled && test.expectedStatus !== 'skipped') {
-          const isNew = isKnownTestsEnabled && isNewTest(test)
-          const fileSuite = getSuiteType(test, 'file')
-          const projectSuite = getSuiteType(test, 'project')
-          // If something change in the file, all tests in the file are impacted
-          const isModifiedTest = () => isModified
-          for (let repeatEachIndex = 1; repeatEachIndex <= earlyFlakeDetectionNumRetries; repeatEachIndex++) {
-            const copyFileSuite = deepCloneSuite(fileSuite, isModifiedTest, [
-              isNew && '_ddIsNew',
-              '_ddIsModified',
-              '_ddIsEfdRetry'
-            ])
-            applyRepeatEachIndex(projectSuite._fullProject, copyFileSuite, repeatEachIndex + 1)
-            projectSuite._addSuite(copyFileSuite)
+        return isImpacted
+      })
+
+      const fileSuitesWithImpactedTestsToProjects = new Map()
+      impactedTests.forEach(impactedTest => {
+        impactedTest._ddIsModified = true
+        if (isEarlyFlakeDetectionEnabled && impactedTest.expectedStatus !== 'skipped') {
+          const fileSuite = getSuiteType(impactedTest, 'file')
+          if (!fileSuitesWithImpactedTestsToProjects.has(fileSuite)) {
+            fileSuitesWithImpactedTestsToProjects.set(fileSuite, getSuiteType(impactedTest, 'project'))
           }
         }
-      }))
+      })
+      // If something change in the file, all tests in the file are impacted, hence the () => true filter
+      applyRetriesToTests(
+        fileSuitesWithImpactedTestsToProjects,
+        () => true,
+        [
+          '_ddIsModified',
+          '_ddIsEfdRetry',
+          (test) => (isKnownTestsEnabled && isNewTest(test) ? '_ddIsNew' : null)
+        ]
+      )
     }
 
     if (isKnownTestsEnabled) {
@@ -860,21 +891,6 @@ addHook({
         isKnownTestsEnabled = false
         isEarlyFlakeDetectionFaulty = true
       } else {
-        /**
-         * We could repeat the logic of `applyRepeatEachIndex` here, but it'd be more risky
-         * as playwright could change it at any time.
-         *
-         * `applyRepeatEachIndex` goes through all the tests in a suite and applies the "repeat" logic
-         * for a single repeat index.
-         *
-         * This means that the clone logic is cumbersome:
-         * - we grab the unique file suites that have new tests
-         * - we store its project suite
-         * - we clone each of these file suites for each repeat index
-         * - we execute `applyRepeatEachIndex` for each of these cloned file suites
-         * - we add the cloned file suites to the project suite
-         */
-
         const fileSuitesWithNewTestsToProjects = new Map()
         newTests.forEach(newTest => {
           newTest._ddIsNew = true
@@ -886,16 +902,7 @@ addHook({
           }
         })
 
-        for (const [fileSuite, projectSuite] of fileSuitesWithNewTestsToProjects.entries()) {
-          for (let repeatEachIndex = 1; repeatEachIndex <= earlyFlakeDetectionNumRetries; repeatEachIndex++) {
-            const copyFileSuite = deepCloneSuite(fileSuite, isNewTest, [
-              '_ddIsNew',
-              '_ddIsEfdRetry'
-            ])
-            applyRepeatEachIndex(projectSuite._fullProject, copyFileSuite, repeatEachIndex + 1)
-            projectSuite._addSuite(copyFileSuite)
-          }
-        }
+        applyRetriesToTests(fileSuitesWithNewTestsToProjects, isNewTest, ['_ddIsNew', '_ddIsEfdRetry'])
       }
     }