fix timing of skipped tests

Author: juan-fernandez

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,7 +338,7 @@ function testBeginHandler (test, browserName, isMainProcess) {
   }
 }
 
-function testEndHandler (test, annotations, testStatus, error, isTimeout, isMainProcess) {
+function testEndHandler (test, annotations, testStatus, error, isTimeout, shouldCreateTestSpan) {
   const { _requireFile: testSuiteAbsolutePath, results, _type } = test
 
   let annotationTags
@@ -368,31 +378,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 +423,24 @@ 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')
+
+    skippedTests.forEach(test => {
+      testSkipCh.publish({
+        testName: getTestFullname(test),
+        testSuiteAbsolutePath: test._requireFile,
+        testSourceLine: test.location.line,
+        browserName: 'chromium', // TODO: REMOVE HARDCODE
+        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')) {
@@ -455,21 +484,23 @@ function dispatcherHook (dispatcherExport) {
         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
+          shouldCreateTestSpan
         )
       }
     })
@@ -489,13 +520,22 @@ function dispatcherHookNew (dispatcherExport, runWrapper) {
       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,
+        STATUS_TO_TEST_STATUS[status],
+        errors && errors[0],
+        isTimeout,
+        shouldCreateTestSpan
+      )
       const testResult = test.results.at(-1)
       const isAtrRetry = testResult?.retry > 0 &&
         isFlakyTestRetriesEnabled &&
@@ -625,6 +665,9 @@ 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`,
@@ -1007,6 +1050,10 @@ addHook({
   const stepInfoByStepId = {}
 
   shimmer.wrap(workerPackage.WorkerMain.prototype, '_runTest', _runTest => async function (test) {
+    if (test.expectedStatus === 'skipped') {
+      console.log('skipped test', test.title)
+      return _runTest.apply(this, arguments)
+    }
     steps = []
 
     const {

packages/datadog-plugin-playwright/src/index.js

@@ -198,36 +198,6 @@ class PlaywrightPlugin extends CiPlugin {
       }
     })
 
-    this.addBind('ci:playwright:test:start', (ctx) => {
-      const {
-        testName,
-        testSuiteAbsolutePath,
-        testSourceLine,
-        browserName,
-        isDisabled
-      } = ctx
-      const store = storage('legacy').getStore()
-      const testSuite = getTestSuitePath(testSuiteAbsolutePath, this.rootDir)
-      const testSourceFile = getTestSuitePath(testSuiteAbsolutePath, this.repositoryRoot)
-      const span = this.startTestSpan(
-        testName,
-        testSuiteAbsolutePath,
-        testSuite,
-        testSourceFile,
-        testSourceLine,
-        browserName
-      )
-
-      if (isDisabled) {
-        span.setTag(TEST_MANAGEMENT_IS_DISABLED, 'true')
-      }
-
-      ctx.parentStore = store
-      ctx.currentStore = { ...store, span }
-
-      return ctx.currentStore
-    })
-
     this.addSub('ci:playwright:worker:report', (serializedTraces) => {
       const traces = JSON.parse(serializedTraces)
       const formattedTraces = []
@@ -277,6 +247,36 @@ class PlaywrightPlugin extends CiPlugin {
       })
     })
 
+    this.addBind('ci:playwright:test:start', (ctx) => {
+      const {
+        testName,
+        testSuiteAbsolutePath,
+        testSourceLine,
+        browserName,
+        isDisabled
+      } = ctx
+      const store = storage('legacy').getStore()
+      const testSuite = getTestSuitePath(testSuiteAbsolutePath, this.rootDir)
+      const testSourceFile = getTestSuitePath(testSuiteAbsolutePath, this.repositoryRoot)
+      const span = this.startTestSpan(
+        testName,
+        testSuiteAbsolutePath,
+        testSuite,
+        testSourceFile,
+        testSourceLine,
+        browserName
+      )
+
+      if (isDisabled) {
+        span.setTag(TEST_MANAGEMENT_IS_DISABLED, 'true')
+      }
+
+      ctx.parentStore = store
+      ctx.currentStore = { ...store, span }
+
+      return ctx.currentStore
+    })
+
     this.addSub('ci:playwright:test:finish', ({
       span,
       testStatus,
@@ -393,6 +393,42 @@ class PlaywrightPlugin extends CiPlugin {
         this.tracer._exporter.flush(onDone)
       }
     })
+
+    this.addSub('ci:playwright:test:skip', ({
+      testName,
+      testSuiteAbsolutePath,
+      testSourceLine,
+      browserName,
+      isNew,
+      isDisabled,
+      isModified,
+      isQuarantined
+    }) => {
+      const testSuite = getTestSuitePath(testSuiteAbsolutePath, this.rootDir)
+      const testSourceFile = getTestSuitePath(testSuiteAbsolutePath, this.repositoryRoot)
+      const span = this.startTestSpan(
+        testName,
+        testSuiteAbsolutePath,
+        testSuite,
+        testSourceFile,
+        testSourceLine,
+        browserName
+      )
+      if (isNew) {
+        span.setTag(TEST_IS_NEW, 'true')
+      }
+      if (isDisabled) {
+        span.setTag(TEST_MANAGEMENT_IS_DISABLED, 'true')
+      }
+      if (isModified) {
+        span.setTag(TEST_IS_MODIFIED, 'true')
+      }
+      if (isQuarantined) {
+        span.setTag(TEST_MANAGEMENT_IS_QUARANTINED, 'true')
+      }
+
+      span.finish()
+    })
   }
 
   // TODO: this runs both in worker and main process (main process: skipped tests that do not go through _runTest)