[test optimization] Fix disable logic in playwright (#6954)

Author: juan-fernandez

integration-tests/ci-visibility/playwright-tests-test-management/disabled-2-test.js

@@ -0,0 +1,42 @@
+'use strict'
+
+/* eslint-disable no-console */
+
+const { test, expect } = require('@playwright/test')
+
+test.beforeEach(async ({ page }) => {
+  await page.goto(process.env.PW_BASE_URL)
+})
+
+test.describe('disable', () => {
+  test('should disable test', async ({ page }) => {
+    console.log('SHOULD NOT BE EXECUTED')
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello Warld'
+    ])
+  })
+})
+
+test.describe('not disabled', () => {
+  test('should not disable test', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})
+
+test.describe('not disabled 2', () => {
+  test('should not disable test 2', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})
+
+test.describe('not disabled 3', () => {
+  test('should not disable test 3', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})

integration-tests/ci-visibility/playwright-tests-test-management/disabled-test.js

@@ -1,5 +1,7 @@
 'use strict'
 
+/* eslint-disable no-console */
+
 const { test, expect } = require('@playwright/test')
 
 test.beforeEach(async ({ page }) => {
@@ -8,8 +10,33 @@ test.beforeEach(async ({ page }) => {
 
 test.describe('disable', () => {
   test('should disable test', async ({ page }) => {
+    console.log('SHOULD NOT BE EXECUTED')
     await expect(page.locator('.hello-world')).toHaveText([
       'Hello Warld'
     ])
   })
 })
+
+test.describe('not disabled', () => {
+  test('should not disable test', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})
+
+test.describe('not disabled 2', () => {
+  test('should not disable test 2', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})
+
+test.describe('not disabled 3', () => {
+  test('should not disable test 3', async ({ page }) => {
+    await expect(page.locator('.hello-world')).toHaveText([
+      'Hello World'
+    ])
+  })
+})

integration-tests/playwright/playwright.spec.js

@@ -1352,7 +1352,9 @@ versions.forEach((version) => {
       })
 
       context('disabled', () => {
+        let testOutput = ''
         beforeEach(() => {
+          testOutput = ''
           receiver.setTestManagementTests({
             playwright: {
               suites: {
@@ -1364,6 +1366,15 @@ versions.forEach((version) => {
                       }
                     }
                   }
+                },
+                'disabled-2-test.js': {
+                  tests: {
+                    'disable should disable test': {
+                      properties: {
+                        disabled: true
+                      }
+                    }
+                  }
                 }
               }
             }
@@ -1375,29 +1386,47 @@ versions.forEach((version) => {
             .gatherPayloadsMaxTimeout(({ url }) => url === '/api/v2/citestcycle', (payloads) => {
               const events = payloads.flatMap(({ payload }) => payload.events)
 
+              const resourceNames = events.filter(event => event.type === 'test').map(event => event.content.resource)
+              assert.includeMembers(resourceNames, [
+                'disabled-test.js.disable should disable test',
+                'disabled-test.js.not disabled should not disable test',
+                'disabled-test.js.not disabled 2 should not disable test 2',
+                'disabled-test.js.not disabled 3 should not disable test 3',
+                'disabled-2-test.js.disable should disable test',
+                'disabled-2-test.js.not disabled should not disable test',
+                'disabled-2-test.js.not disabled 2 should not disable test 2',
+                'disabled-2-test.js.not disabled 3 should not disable test 3',
+              ])
+
               const testSession = events.find(event => event.type === 'test_session_end').content
               if (isDisabling) {
                 assert.propertyVal(testSession.meta, TEST_MANAGEMENT_ENABLED, 'true')
               } else {
                 assert.notProperty(testSession.meta, TEST_MANAGEMENT_ENABLED)
               }
 
-              const skippedTest = events.find(event => event.type === 'test').content
+              const tests = events.filter(event => event.type === 'test').map(event => event.content)
+              assert.equal(tests.length, 8)
 
-              if (isDisabling) {
-                assert.equal(skippedTest.meta[TEST_STATUS], 'skip')
-                assert.propertyVal(skippedTest.meta, TEST_MANAGEMENT_IS_DISABLED, 'true')
-              } else {
-                assert.equal(skippedTest.meta[TEST_STATUS], 'fail')
-                assert.notProperty(skippedTest.meta, TEST_MANAGEMENT_IS_DISABLED)
-              }
+              const disabledTests = tests.filter(test => test.meta[TEST_NAME] === 'disable should disable test')
+              assert.equal(disabledTests.length, 2)
+
+              disabledTests.forEach(test => {
+                if (isDisabling) {
+                  assert.equal(test.meta[TEST_STATUS], 'skip')
+                  assert.propertyVal(test.meta, TEST_MANAGEMENT_IS_DISABLED, 'true')
+                } else {
+                  assert.equal(test.meta[TEST_STATUS], 'fail')
+                  assert.notProperty(test.meta, TEST_MANAGEMENT_IS_DISABLED)
+                }
+              })
             }, 25000)
 
         const runDisableTest = async (isDisabling, extraEnvVars) => {
           const testAssertionsPromise = getTestAssertions(isDisabling)
 
           childProcess = exec(
-            './node_modules/.bin/playwright test -c playwright.config.js disabled-test.js',
+            './node_modules/.bin/playwright test -c playwright.config.js disabled-test.js disabled-2-test.js',
             {
               cwd,
               env: {
@@ -1410,14 +1439,26 @@ versions.forEach((version) => {
             }
           )
 
+          childProcess.stdout.on('data', (chunk) => {
+            testOutput += chunk.toString()
+          })
+          childProcess.stderr.on('data', (chunk) => {
+            testOutput += chunk.toString()
+          })
+
           const [[exitCode]] = await Promise.all([
             once(childProcess, 'exit'),
+            once(childProcess.stdout, 'end'),
+            once(childProcess.stderr, 'end'),
             testAssertionsPromise
           ])
 
+          // the testOutput checks whether the test is actually skipped
           if (isDisabling) {
+            assert.notInclude(testOutput, 'SHOULD NOT BE EXECUTED')
             assert.equal(exitCode, 0)
           } else {
+            assert.include(testOutput, 'SHOULD NOT BE EXECUTED')
             assert.equal(exitCode, 1)
           }
         }
@@ -1428,6 +1469,12 @@ versions.forEach((version) => {
           await runDisableTest(true)
         })
 
+        it('can disable tests in fullyParallel mode', async () => {
+          receiver.setSettings({ test_management: { enabled: true } })
+
+          await runDisableTest(true, { FULLY_PARALLEL: true, PLAYWRIGHT_WORKERS: '3' })
+        })
+
         it('fails if disable is not enabled', async () => {
           receiver.setSettings({ test_management: { enabled: false } })
 

packages/datadog-instrumentations/src/playwright.js

@@ -481,6 +481,15 @@ function dispatcherRunWrapper (run) {
 
 function dispatcherRunWrapperNew (run) {
   return function (testGroups) {
+    // Filter out disabled tests from testGroups before they get scheduled
+    if (isTestManagementTestsEnabled) {
+      testGroups.forEach(group => {
+        group.tests = group.tests.filter(test => !test._ddIsDisabled)
+      })
+      // Remove empty groups
+      testGroups = testGroups.filter(group => group.tests.length > 0)
+    }
+
     if (!this._allTests) {
       // Removed in https://github.com/microsoft/playwright/commit/1e52c37b254a441cccf332520f60225a5acc14c7
       // Not available from >=1.44.0
@@ -893,6 +902,9 @@ addHook({
         if (testProperties.disabled) {
           test._ddIsDisabled = true
           test.expectedStatus = 'skipped'
+          // setting test.expectedStatus to 'skipped' does not work for every case,
+          // so we need to filter out disabled tests in dispatcherRunWrapperNew,
+          // so they don't get to the workers
           continue
         }
         if (testProperties.quarantined) {
@@ -1270,6 +1282,10 @@ function generateSummaryWrapper (generateSummary) {
         const {
           _requireFile: testSuiteAbsolutePath,
           location: { line: testSourceLine },
+          _ddIsNew: isNew,
+          _ddIsDisabled: isDisabled,
+          _ddIsModified: isModified,
+          _ddIsQuarantined: isQuarantined
         } = test
         const browserName = getBrowserNameFromProjects(sessionProjects, test)
 
@@ -1278,6 +1294,10 @@ function generateSummaryWrapper (generateSummary) {
           testSuiteAbsolutePath,
           testSourceLine,
           browserName,
+          isNew,
+          isDisabled,
+          isModified,
+          isQuarantined
         })
       }
     }