[test optimization] Fix reported test suite duration in playwright

integration-tests/ci-visibility/playwright-test-duration/long-suite-test.js

@@ -0,0 +1,10 @@
+'use strict'
+
+const { test, expect } = require('@playwright/test')
+
+test.describe('long suite', () => {
+  test('should be able to run', async () => {
+    await new Promise(resolve => setTimeout(resolve, 5000))
+    expect(true).toBe(true)
+  })
+})

integration-tests/ci-visibility/playwright-test-duration/short-suite-test.js

@@ -0,0 +1,13 @@
+'use strict'
+
+const { test, expect } = require('@playwright/test')
+
+test.describe('short suite', () => {
+  test('should be able to run', async () => {
+    expect(true).toBe(true)
+  })
+
+  test.skip('should skip and not mess up the duration of the test suite', async () => {
+    // TODO
+  })
+})

integration-tests/playwright.config.js

@@ -7,6 +7,8 @@ const config = {
   baseURL: process.env.PW_BASE_URL,
   testDir: process.env.TEST_DIR || './ci-visibility/playwright-tests',
   timeout: Number(process.env.TEST_TIMEOUT) || 30000,
+  fullyParallel: process.env.FULLY_PARALLEL === 'true',
+  workers: process.env.PLAYWRIGHT_WORKERS ? Number(process.env.PLAYWRIGHT_WORKERS) : undefined,
   reporter: 'line',
   /* Configure projects for major browsers */
   projects: [

integration-tests/playwright/playwright.spec.js

@@ -2004,5 +2004,69 @@ versions.forEach((version) => {
         })
       })
     })
+
+    const fullyParallelConfigValue = [true, false]
+
+    fullyParallelConfigValue.forEach((parallelism) => {
+      context(`with fullyParallel=${parallelism}`, () => {
+        /**
+         * Due to a bug in the playwright plugin, durations of test suites that included skipped tests
+         * were not reported correctly, as they dragged out until the end of the test session.
+         * This test checks that a long suite, which makes the test session longer,
+         * does not affect the duration of a short suite, which is expected to finish earlier.
+         * This only happened with tests that included skipped tests.
+         */
+        it('should report the correct test suite duration when there are skipped tests', async () => {
+          const receiverPromise = receiver
+            .gatherPayloadsMaxTimeout(({ url }) => url === '/api/v2/citestcycle', (payloads) => {
+              const events = payloads.flatMap(({ payload }) => payload.events)
+
+              const testSuites = events.filter(event => event.type === 'test_suite_end').map(event => event.content)
+              assert.equal(testSuites.length, 2)
+              const tests = events.filter(event => event.type === 'test').map(event => event.content)
+              assert.equal(tests.length, 3)
+
+              const skippedTest = tests.find(test => test.meta[TEST_STATUS] === 'skip')
+              assert.propertyVal(
+                skippedTest.meta,
+                TEST_NAME,
+                'short suite should skip and not mess up the duration of the test suite'
+              )
+              const shortSuite = testSuites.find(suite => suite.meta[TEST_SUITE].endsWith('short-suite-test.js'))
+              const longSuite = testSuites.find(suite => suite.meta[TEST_SUITE].endsWith('long-suite-test.js'))
+              // The values are not deterministic, so we can only assert that they're distant enough
+              // This checks that the long suite takes at least twice longer than the short suite
+              assert.isAbove(
+                Number(longSuite.duration),
+                Number(shortSuite.duration) * 2,
+                'The long test suite should take at least twice as long as the short suite, ' +
+                'but their durations are: \n' +
+                `- Long suite: ${Number(longSuite.duration) / 1e6}ms \n` +
+                `- Short suite: ${Number(shortSuite.duration) / 1e6}ms`
+              )
+            })
+
+          childProcess = exec(
+            './node_modules/.bin/playwright test -c playwright.config.js',
+            {
+              cwd,
+              env: {
+                ...getCiVisAgentlessConfig(receiver.port),
+                PW_BASE_URL: `http://localhost:${webAppPort}`,
+                TEST_DIR: './ci-visibility/playwright-test-duration',
+                FULLY_PARALLEL: parallelism,
+                PLAYWRIGHT_WORKERS: 2
+              },
+              stdio: 'pipe'
+            }
+          )
+
+          await Promise.all([
+            receiverPromise,
+            once(childProcess, 'exit')
+          ])
+        })
+      })
+    })
   })
 })

packages/datadog-instrumentations/src/playwright.js

@@ -15,6 +15,7 @@ const { DD_MAJOR } = require('../../../version')
 
 const testStartCh = channel('ci:playwright:test:start')
 const testFinishCh = channel('ci:playwright:test:finish')
+const testSkipCh = channel('ci:playwright:test:skip')
 
 const testSessionStartCh = channel('ci:playwright:session:start')
 const testSessionFinishCh = channel('ci:playwright:session:finish')
@@ -285,7 +286,12 @@ function getTestFullname (test) {
   return names.join(' ')
 }
 
-function testBeginHandler (test, browserName, isMainProcess) {
+function shouldFinishTestSuite (testSuiteAbsolutePath) {
+  const remainingTests = remainingTestsByFile[testSuiteAbsolutePath]
+  return !remainingTests.length || remainingTests.every(test => test.expectedStatus === 'skipped')
+}
+
+function testBeginHandler (test, browserName, shouldCreateTestSpan) {
   const {
     _requireFile: testSuiteAbsolutePath,
     location: {
@@ -297,6 +303,10 @@ function testBeginHandler (test, browserName, isMainProcess) {
   if (_type === 'beforeAll' || _type === 'afterAll') {
     return
   }
+  // this means that a skipped test is being handled
+  if (!remainingTestsByFile[testSuiteAbsolutePath].length) {
+    return
+  }
 
   const isNewTestSuite = !startedSuites.includes(testSuiteAbsolutePath)
 
@@ -313,7 +323,7 @@ function testBeginHandler (test, browserName, isMainProcess) {
   }
 
   // this handles tests that do not go through the worker process (because they're skipped)
-  if (isMainProcess) {
+  if (shouldCreateTestSpan) {
     const testName = getTestFullname(test)
     const testCtx = {
       testName,
@@ -328,8 +338,20 @@ function testBeginHandler (test, browserName, isMainProcess) {
   }
 }
 
-function testEndHandler (test, annotations, testStatus, error, isTimeout, isMainProcess) {
-  const { _requireFile: testSuiteAbsolutePath, results, _type } = test
+function testEndHandler ({
+  test,
+  annotations,
+  testStatus,
+  error,
+  isTimeout,
+  shouldCreateTestSpan,
+  projects
+}) {
+  const {
+    _requireFile: testSuiteAbsolutePath,
+    results,
+    _type,
+  } = test
 
   let annotationTags
   if (annotations.length) {
@@ -368,31 +390,34 @@ function testEndHandler (test, annotations, testStatus, error, isTimeout, isMain
   }
 
   // this handles tests that do not go through the worker process (because they're skipped)
-  if (isMainProcess) {
+  if (shouldCreateTestSpan) {
     const testResult = results.at(-1)
     const testCtx = testToCtx.get(test)
     const isAtrRetry = testResult?.retry > 0 &&
       isFlakyTestRetriesEnabled &&
       !test._ddIsAttemptToFix &&
       !test._ddIsEfdRetry
-    testFinishCh.publish({
-      testStatus,
-      steps: testResult?.steps || [],
-      isRetry: testResult?.retry > 0,
-      error,
-      extraTags: annotationTags,
-      isNew: test._ddIsNew,
-      isAttemptToFix: test._ddIsAttemptToFix,
-      isAttemptToFixRetry: test._ddIsAttemptToFixRetry,
-      isQuarantined: test._ddIsQuarantined,
-      isEfdRetry: test._ddIsEfdRetry,
-      hasFailedAllRetries: test._ddHasFailedAllRetries,
-      hasPassedAttemptToFixRetries: test._ddHasPassedAttemptToFixRetries,
-      hasFailedAttemptToFixRetries: test._ddHasFailedAttemptToFixRetries,
-      isAtrRetry,
-      isModified: test._ddIsModified,
-      ...testCtx.currentStore
-    })
+    // if there is no testCtx, the skipped test will be created later
+    if (testCtx) {
+      testFinishCh.publish({
+        testStatus,
+        steps: testResult?.steps || [],
+        isRetry: testResult?.retry > 0,
+        error,
+        extraTags: annotationTags,
+        isNew: test._ddIsNew,
+        isAttemptToFix: test._ddIsAttemptToFix,
+        isAttemptToFixRetry: test._ddIsAttemptToFixRetry,
+        isQuarantined: test._ddIsQuarantined,
+        isEfdRetry: test._ddIsEfdRetry,
+        hasFailedAllRetries: test._ddHasFailedAllRetries,
+        hasPassedAttemptToFixRetries: test._ddHasPassedAttemptToFixRetries,
+        hasFailedAttemptToFixRetries: test._ddHasFailedAttemptToFixRetries,
+        isAtrRetry,
+        isModified: test._ddIsModified,
+        ...testCtx.currentStore
+      })
+    }
   }
 
   if (testSuiteToTestStatuses.has(testSuiteAbsolutePath)) {
@@ -410,8 +435,25 @@ function testEndHandler (test, annotations, testStatus, error, isTimeout, isMain
       .filter(currentTest => currentTest !== test)
   }
 
-  // Last test, we finish the suite
-  if (!remainingTestsByFile[testSuiteAbsolutePath].length) {
+  if (shouldFinishTestSuite(testSuiteAbsolutePath)) {
+    const skippedTests = remainingTestsByFile[testSuiteAbsolutePath]
+      .filter(test => test.expectedStatus === 'skipped')
+
+    for (const test of skippedTests) {
+      const browserName = getBrowserNameFromProjects(projects, test)
+      testSkipCh.publish({
+        testName: getTestFullname(test),
+        testSuiteAbsolutePath,
+        testSourceLine: test.location.line,
+        browserName,
+        isNew: test._ddIsNew,
+        isDisabled: test._ddIsDisabled,
+        isModified: test._ddIsModified,
+        isQuarantined: test._ddIsQuarantined
+      })
+    }
+    remainingTestsByFile[testSuiteAbsolutePath] = []
+
     const testStatuses = testSuiteToTestStatuses.get(testSuiteAbsolutePath)
     let testSuiteStatus = 'pass'
     if (testStatuses.includes('fail')) {
@@ -450,26 +492,32 @@ function dispatcherHook (dispatcherExport) {
   shimmer.wrap(dispatcherExport.Dispatcher.prototype, '_createWorker', createWorker => function () {
     const dispatcher = this
     const worker = createWorker.apply(this, arguments)
+    const projects = getProjectsFromDispatcher(dispatcher)
+
     worker.process.on('message', ({ method, params }) => {
       if (method === 'testBegin') {
         const { test } = dispatcher._testById.get(params.testId)
-        const projects = getProjectsFromDispatcher(dispatcher)
         const browser = getBrowserNameFromProjects(projects, test)
-        testBeginHandler(test, browser, true)
+        const shouldCreateTestSpan = test.expectedStatus === 'skipped'
+        testBeginHandler(test, browser, shouldCreateTestSpan)
       } else if (method === 'testEnd') {
         const { test } = dispatcher._testById.get(params.testId)
 
         const { results } = test
         const testResult = results.at(-1)
 
         const isTimeout = testResult.status === 'timedOut'
+        const shouldCreateTestSpan = test.expectedStatus === 'skipped'
         testEndHandler(
-          test,
-          params.annotations,
-          STATUS_TO_TEST_STATUS[testResult.status],
-          testResult.error,
-          isTimeout,
-          true
+          {
+            test,
+            annotations: params.annotations,
+            testStatus: STATUS_TO_TEST_STATUS[testResult.status],
+            error: testResult.error,
+            isTimeout,
+            shouldCreateTestSpan,
+            projects
+          }
         )
       }
     })
@@ -484,18 +532,30 @@ function dispatcherHookNew (dispatcherExport, runWrapper) {
   shimmer.wrap(dispatcherExport.Dispatcher.prototype, '_createWorker', createWorker => function () {
     const dispatcher = this
     const worker = createWorker.apply(this, arguments)
+    const projects = getProjectsFromDispatcher(dispatcher)
 
     worker.on('testBegin', ({ testId }) => {
       const test = getTestByTestId(dispatcher, testId)
-      const projects = getProjectsFromDispatcher(dispatcher)
       const browser = getBrowserNameFromProjects(projects, test)
-      testBeginHandler(test, browser, false)
+      const shouldCreateTestSpan = test.expectedStatus === 'skipped'
+      testBeginHandler(test, browser, shouldCreateTestSpan)
     })
     worker.on('testEnd', ({ testId, status, errors, annotations }) => {
       const test = getTestByTestId(dispatcher, testId)
 
       const isTimeout = status === 'timedOut'
-      testEndHandler(test, annotations, STATUS_TO_TEST_STATUS[status], errors && errors[0], isTimeout, false)
+      const shouldCreateTestSpan = test.expectedStatus === 'skipped'
+      testEndHandler(
+        {
+          test,
+          annotations,
+          testStatus: STATUS_TO_TEST_STATUS[status],
+          error: errors && errors[0],
+          isTimeout,
+          shouldCreateTestSpan,
+          projects
+        }
+      )
       const testResult = test.results.at(-1)
       const isAtrRetry = testResult?.retry > 0 &&
         isFlakyTestRetriesEnabled &&
@@ -625,14 +685,25 @@ function runAllTestsWrapper (runAllTests, playwrightVersion) {
 
     let runAllTestsReturn = await runAllTests.apply(this, arguments)
 
+    // Tests that have only skipped tests may reach this point
+    // Skipped tests may or may not go through `testBegin` or `testEnd`
+    // depending on the playwright configuration
     Object.values(remainingTestsByFile).forEach(tests => {
       // `tests` should normally be empty, but if it isn't,
       // there were tests that did not go through `testBegin` or `testEnd`,
       // because they were skipped
       tests.forEach(test => {
         const browser = getBrowserNameFromProjects(projects, test)
         testBeginHandler(test, browser, true)
-        testEndHandler(test, [], 'skip', null, false, true)
+        testEndHandler({
+          test,
+          annotations: [],
+          testStatus: 'skip',
+          error: null,
+          isTimeout: false,
+          shouldCreateTestSpan: true,
+          projects
+        })
       })
     })
 
@@ -1007,6 +1078,9 @@ addHook({
   const stepInfoByStepId = {}
 
   shimmer.wrap(workerPackage.WorkerMain.prototype, '_runTest', _runTest => async function (test) {
+    if (test.expectedStatus === 'skipped') {
+      return _runTest.apply(this, arguments)
+    }
     steps = []
 
     const {