fix(test-runner): apply fixme v. skip annotations (#15277)

Currently, if `text.fixme()` or `test.skip()` is used within a test, we
add a `fixme` or `skip` annotation. However, if the wrapper style is
used:

```
test.fixme('should work', () => {…})
```

the annotations were missing. This change adds annotations for the
above.

These annotations are important for reporting purposes and knowing
exactly what flavor of "skipped" was used.

Fixes #15239.

packages/playwright-test/src/dispatcher.ts

@@ -215,7 +215,7 @@ export class Dispatcher {
       }));
       result.status = params.status;
       test.expectedStatus = params.expectedStatus;
-      test.annotations = params.annotations;
+      test._annotateWithInheritence(params.annotations);
       test.timeout = params.timeout;
       const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus;
       if (isFailure)

packages/playwright-test/src/test.ts

@@ -134,6 +134,9 @@ export class TestCase extends Base implements reporterTypes.TestCase {
   _workerHash = '';
   _pool: FixturePool | undefined;
   _projectIndex = 0;
+  // Annotations that are not added from within a test (like fixme and skip), should not
+  // be re-added each time we retry a test.
+  _alreadyInheritedAnnotations: boolean = false;
 
   constructor(title: string, fn: Function, testType: TestTypeImpl, location: Location) {
     super(title);
@@ -169,9 +172,20 @@ export class TestCase extends Base implements reporterTypes.TestCase {
     test._only = this._only;
     test._requireFile = this._requireFile;
     test.expectedStatus = this.expectedStatus;
+    test.annotations = this.annotations.slice();
+    test._annotateWithInheritence = this._annotateWithInheritence;
     return test;
   }
 
+  _annotateWithInheritence(annotations: Annotation[]) {
+    if (this._alreadyInheritedAnnotations) {
+      this.annotations = annotations;
+    } else {
+      this._alreadyInheritedAnnotations = true;
+      this.annotations = [...this.annotations, ...annotations];
+    }
+  }
+
   _appendTestResult(): reporterTypes.TestResult {
     const result: reporterTypes.TestResult = {
       retry: this.results.length,

packages/playwright-test/src/testType.ts

@@ -88,8 +88,10 @@ export class TestTypeImpl {
 
     if (type === 'only')
       test._only = true;
-    if (type === 'skip' || type === 'fixme')
+    if (type === 'skip' || type === 'fixme') {
+      test.annotations.push({ type });
       test.expectedStatus = 'skipped';
+    }
     for (let parent: Suite | undefined = suite; parent; parent = parent.parent) {
       if (parent._skipped)
         test.expectedStatus = 'skipped';
@@ -120,8 +122,10 @@ export class TestTypeImpl {
       child._parallelMode = 'serial';
     if (type === 'parallel' || type === 'parallel.only')
       child._parallelMode = 'parallel';
-    if (type === 'skip')
+    if (type === 'skip') {
       child._skipped = true;
+      child._annotations.push({ type: 'skip' });
+    }
 
     for (let parent: Suite | undefined = suite; parent; parent = parent.parent) {
       if (parent._parallelMode === 'serial' && child._parallelMode === 'parallel')

tests/playwright-test/playwright-test-fixtures.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import type { JSONReport, JSONReportSuite, JSONReportTestResult } from '@playwright/test/reporter';
+import type { JSONReport, JSONReportSuite, JSONReportTest, JSONReportTestResult } from '@playwright/test/reporter';
 import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
@@ -25,11 +25,12 @@ import { commonFixtures } from '../config/commonFixtures';
 import type { ServerFixtures, ServerWorkerOptions } from '../config/serverFixtures';
 import { serverFixtures } from '../config/serverFixtures';
 import type { TestInfo } from './stable-test-runner';
+import { expect } from './stable-test-runner';
 import { test as base } from './stable-test-runner';
 
 const removeFolderAsync = promisify(rimraf);
 
-type RunResult = {
+export type RunResult = {
   exitCode: number,
   output: string,
   passed: number,
@@ -319,3 +320,25 @@ export function paintBlackPixels(image: Buffer, blackPixelsCount: number): Buffe
   }
   return PNG.sync.write(png);
 }
+
+export function allTests(result: RunResult) {
+  const tests: { title: string; expectedStatus: JSONReportTest['expectedStatus'], actualStatus: JSONReportTest['status'], annotations: string[] }[] = [];
+  const visit = (suite: JSONReportSuite) => {
+    for (const spec of suite.specs)
+      spec.tests.forEach(t => tests.push({ title: spec.title, expectedStatus: t.expectedStatus, actualStatus: t.status, annotations: t.annotations.map(a => a.type)  }));
+    suite.suites?.forEach(s => visit(s));
+  };
+  visit(result.report.suites[0]);
+  return tests;
+}
+
+export function expectTestHelper(result: RunResult) {
+  return (title: string, expectedStatus: string, status: string, annotations: any) => {
+    const tests = allTests(result).filter(t => t.title === title);
+    for (const test of tests) {
+      expect(test.expectedStatus, `title: ${title}`).toBe(expectedStatus);
+      expect(test.actualStatus, `title: ${title}`).toBe(status);
+      expect(test.annotations, `title: ${title}`).toEqual(annotations);
+    }
+  };
+}

tests/playwright-test/test-modifiers.spec.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { test, expect, stripAnsi } from './playwright-test-fixtures';
+import { test, expect, stripAnsi, expectTestHelper } from './playwright-test-fixtures';
 
 test('test modifiers should work', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -130,6 +130,184 @@ test('test modifiers should work', async ({ runInlineTest }) => {
   expect(result.skipped).toBe(9);
 });
 
+test.describe('test modifier annotations', () => {
+  test('should work', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+
+        test.describe('suite1', () => {
+          test('no marker', () => {});
+          test.skip('skip wrap', () => {});
+          test('skip inner', () => { test.skip(); });
+          test.fixme('fixme wrap', () => {});
+          test('fixme inner', () => { test.fixme(); });
+        });
+
+        test('example', () => {});
+      `,
+    });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(2);
+    expect(result.skipped).toBe(4);
+    expectTest('no marker', 'passed', 'expected', []);
+    expectTest('skip wrap', 'skipped', 'skipped', ['skip']);
+    expectTest('skip inner', 'skipped', 'skipped', ['skip']);
+    expectTest('fixme wrap', 'skipped', 'skipped', ['fixme']);
+    expectTest('fixme inner', 'skipped', 'skipped', ['fixme']);
+    expectTest('example', 'passed', 'expected', []);
+  });
+
+  test('should work alongside top-level modifier', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+
+        test.fixme();
+
+        test.describe('suite1', () => {
+          test('no marker', () => {});
+          test.skip('skip wrap', () => {});
+          test('skip inner', () => { test.skip(); });
+          test.fixme('fixme wrap', () => {});
+          test('fixme inner', () => { test.fixme(); });
+        });
+
+        test('example', () => {});
+      `,
+    });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(0);
+    expect(result.skipped).toBe(6);
+    expectTest('no marker', 'skipped', 'skipped', ['fixme']);
+    expectTest('skip wrap', 'skipped', 'skipped', ['skip', 'fixme']);
+    expectTest('skip inner', 'skipped', 'skipped', ['fixme']);
+    expectTest('fixme wrap', 'skipped', 'skipped', ['fixme','fixme']);
+    expectTest('fixme inner', 'skipped', 'skipped', ['fixme']);
+    expectTest('example', 'skipped', 'skipped', ['fixme']);
+  });
+
+  test('should work alongside top-level modifier wrapper-style', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+
+        test.describe.skip('suite1', () => {
+          test('no marker', () => {});
+          test.skip('skip wrap', () => {});
+          test('skip inner', () => { test.skip(); });
+          test.fixme('fixme wrap', () => {});
+          test('fixme inner', () => { test.fixme(); });
+        });
+
+        test('example', () => {});
+      `,
+    });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(1);
+    expect(result.skipped).toBe(5);
+    expectTest('no marker', 'skipped', 'skipped', ['skip']);
+    expectTest('skip wrap', 'skipped', 'skipped', ['skip', 'skip']);
+    expectTest('skip inner', 'skipped', 'skipped', ['skip']);
+    expectTest('fixme wrap', 'skipped', 'skipped', ['fixme', 'skip']);
+    expectTest('fixme inner', 'skipped', 'skipped', ['skip']);
+    expectTest('example', 'passed', 'expected', []);
+  });
+
+  test('should work with nesting', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+
+        test.fixme();
+
+        test.describe.skip('suite1', () => {
+          test.describe.skip('sub', () => {
+            test.describe('a', () => {
+              test.describe('b', () => {
+                test.fixme();
+
+                test.fixme('fixme wrap', () => {});
+                test('fixme inner', () => { test.fixme(); });
+              })
+            })
+          })
+        });
+      `,
+    });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(0);
+    expect(result.skipped).toBe(2);
+    expectTest('fixme wrap', 'skipped', 'skipped', ['fixme', 'fixme', 'skip', 'skip', 'fixme']);
+    expectTest('fixme inner', 'skipped', 'skipped', ['fixme', 'skip', 'skip', 'fixme']);
+  });
+
+  test('should work with only', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+
+        test.describe.only("suite", () => {
+          test.skip('focused skip by suite', () => {});
+          test.fixme('focused fixme by suite', () => {});
+        });
+
+        test.describe.skip('not focused', () => {
+          test('no marker', () => {});
+        });
+      `,
+    });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(0);
+    expect(result.skipped).toBe(2);
+    expectTest('focused skip by suite', 'skipped', 'skipped', ['skip']);
+    expectTest('focused fixme by suite', 'skipped', 'skipped', ['fixme']);
+  });
+
+  test('should not multiple on retry', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+        test('retry', () => {
+          test.info().annotations.push({ type: 'example' });
+          expect(1).toBe(2);
+        });
+      `,
+    }, { retries: 3 });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(1);
+    expect(result.passed).toBe(0);
+    expectTest('retry', 'passed', 'unexpected', ['example']);
+  });
+
+  test('should not multiply on repeat-each', async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.test.ts': `
+        const { test } = pwt;
+        test('retry', () => {
+          test.info().annotations.push({ type: 'example' });
+        });
+      `,
+    }, { 'repeat-each': 3 });
+    const expectTest = expectTestHelper(result);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(3);
+    expectTest('retry', 'passed', 'expected', ['example']);
+  });
+});
+
 test('test modifiers should check types', async ({ runTSC }) => {
   const result = await runTSC({
     'helper.ts': `