chore: followup on static annotations (#35579)

Signed-off-by: Simon Knott <info@simonknott.de>
Co-authored-by: Dmitry Gozman <dgozman@gmail.com>

docs/src/test-reporter-api/class-testcase.md

@@ -11,13 +11,7 @@
   - `description` ?<[string]> Optional description.
   - `location` ?<[Location]> Optional location in the source where the annotation is added.
 
-The list of annotations applicable to the current test. Includes:
+[`property: TestResult.annotations`] of the last test run.
-* annotations defined on the test or suite via [`method: Test.(call)`] and [`method: Test.describe`];
-* annotations implicitly added by methods [`method: Test.skip`], [`method: Test.fixme`] and [`method: Test.fail`] prior to test execution.
-
-Annotations are available during test execution through [`property: TestInfo.annotations`].
-
-Learn more about [test annotations](../test-annotations.md).
 
 ## property: TestCase.expectedStatus
 * since: v1.10

docs/src/test-reporter-api/class-testresult.md

@@ -21,9 +21,10 @@ The list of files or buffers attached during the test execution through [`proper
   - `description` ?<[string]> Optional description.
   - `location` ?<[Location]> Optional location in the source where the annotation is added.
 
-The list of annotations appended during test execution. Includes:
+The list of annotations applicable to the current test. Includes:
-* annotations implicitly added by methods [`method: Test.skip`], [`method: Test.fixme`] and [`method: Test.fail`] during test execution;
+* annotations defined on the test or suite via [`method: Test.(call)`] and [`method: Test.describe`];
-* annotations appended to [`property: TestInfo.annotations`].
+* annotations implicitly added by methods [`method: Test.skip`], [`method: Test.fixme`] and [`method: Test.fail`];
+* annotations appended to [`property: TestInfo.annotations`] during the test execution.
 
 Annotations are available during test execution through [`property: TestInfo.annotations`].
 

packages/html-reporter/src/testCaseView.spec.tsx

@@ -42,7 +42,11 @@ const result: TestResult = {
     }],
     attachments: [],
   }],
-  annotations: [],
+  annotations: [
+    { type: 'annotation', description: 'Annotation text' },
+    { type: 'annotation', description: 'Another annotation text' },
+    { type: '_annotation', description: 'Hidden annotation' },
+  ],
   attachments: [],
   status: 'passed',
 };
@@ -53,11 +57,7 @@ const testCase: TestCase = {
   path: [],
   projectName: 'chromium',
   location: { file: 'test.spec.ts', line: 42, column: 0 },
-  annotations: [
+  annotations: result.annotations,
-    { type: 'annotation', description: 'Annotation text' },
-    { type: 'annotation', description: 'Another annotation text' },
-    { type: '_annotation', description: 'Hidden annotation' },
-  ],
   tags: [],
   outcome: 'expected',
   duration: 200,
@@ -98,16 +98,18 @@ const annotationLinkRenderingTestCase: TestCase = {
   path: [],
   projectName: 'chromium',
   location: { file: 'test.spec.ts', line: 42, column: 0 },
-  annotations: [
+  annotations: [],
-    { type: 'more info', description: 'read https://playwright.dev/docs/intro and https://playwright.dev/docs/api/class-playwright' },
-    { type: 'related issues', description: 'https://github.com/microsoft/playwright/issues/23180, https://github.com/microsoft/playwright/issues/23181' },
-
-  ],
   tags: [],
   outcome: 'expected',
   duration: 10,
   ok: true,
-  results: [result]
+  results: [{
+    ...result,
+    annotations: [
+      { type: 'more info', description: 'read https://playwright.dev/docs/intro and https://playwright.dev/docs/api/class-playwright' },
+      { type: 'related issues', description: 'https://github.com/microsoft/playwright/issues/23180, https://github.com/microsoft/playwright/issues/23181' },
+    ]
+  }]
 };
 
 test('should correctly render links in annotations', async ({ mount }) => {

packages/html-reporter/src/testCaseView.tsx

@@ -46,14 +46,7 @@ export const TestCaseView: React.FC<{
     return test.tags;
   }, [test]);
 
-  const visibleAnnotations = React.useMemo(() => {
+  const visibleTestAnnotations = test?.annotations.filter(a => !a.type.startsWith('_')) ?? [];
-    if (!test)
-      return [];
-    const annotations = [...test.annotations];
-    if (test.results[selectedResultIndex])
-      annotations.push(...test.results[selectedResultIndex].annotations);
-    return annotations.filter(annotation => !annotation.type.startsWith('_'));
-  }, [test, selectedResultIndex]);
 
   return <div className='test-case-column vbox'>
     {test && <div className='hbox'>
@@ -77,8 +70,8 @@ export const TestCaseView: React.FC<{
       {test && !!test.projectName && <ProjectLink projectNames={projectNames} projectName={test.projectName}></ProjectLink>}
       {labels && <LabelsLinkView labels={labels} />}
     </div>}
-    {!!visibleAnnotations.length && <AutoChip header='Annotations' dataTestId='test-case-annotations'>
+    {test?.results.length === 0 && visibleTestAnnotations.length !== 0 && <AutoChip header='Annotations' dataTestId='test-case-annotations'>
-      {visibleAnnotations.map((annotation, index) => <TestCaseAnnotationView key={index} annotation={annotation} />)}
+      {visibleTestAnnotations.map((annotation, index) => <TestCaseAnnotationView key={index} annotation={annotation} />)}
     </AutoChip>}
     {test && <TabbedPane tabs={
       test.results.map((result, index) => ({
@@ -87,7 +80,15 @@ export const TestCaseView: React.FC<{
           {statusIcon(result.status)} {retryLabel(index)}
           {(test.results.length > 1) && <span className='test-case-run-duration'>{msToString(result.duration)}</span>}
         </div>,
-        render: () => <TestResultView test={test!} result={result} />
+        render: () => {
+          const visibleAnnotations = result.annotations.filter(annotation => !annotation.type.startsWith('_'));
+          return <>
+            {!!visibleAnnotations.length && <AutoChip header='Annotations' dataTestId='test-case-annotations'>
+              {visibleAnnotations.map((annotation, index) => <TestCaseAnnotationView key={index} annotation={annotation} />)}
+            </AutoChip>}
+            <TestResultView test={test!} result={result} />
+          </>;
+        },
       })) || []} selectedTab={String(selectedResultIndex)} setSelectedTab={id => setSelectedResultIndex(+id)} />}
   </div>;
 };

packages/playwright/src/isomorphic/teleReceiver.ts

@@ -235,17 +235,16 @@ export class TeleReporterReceiver {
     const test = this._tests.get(testEndPayload.testId)!;
     test.timeout = testEndPayload.timeout;
     test.expectedStatus = testEndPayload.expectedStatus;
-    // Should be empty array, but if it's not, it represents all annotations for that test
-    if (testEndPayload.annotations.length > 0)
-      test.annotations = this._absoluteAnnotationLocations(testEndPayload.annotations);
     const result = test.results.find(r => r._id === payload.id)!;
     result.duration = payload.duration;
     result.status = payload.status;
     result.errors = payload.errors;
     result.error = result.errors?.[0];
     result.attachments = this._parseAttachments(payload.attachments);
-    if (payload.annotations)
+    if (payload.annotations) {
       result.annotations = this._absoluteAnnotationLocations(payload.annotations);
+      test.annotations = result.annotations;
+    }
     this._reporter.onTestEnd?.(test, result);
     // Free up the memory as won't see these step ids.
     result._stepMap = new Map();

packages/playwright/src/reporters/base.ts

@@ -320,7 +320,7 @@ export function formatFailure(screen: Screen, config: FullConfig, test: TestCase
   const header = formatTestHeader(screen, config, test, { indent: '  ', index, mode: 'error' });
   lines.push(screen.colors.red(header));
   for (const result of test.results) {
-    const warnings = [...result.annotations, ...test.annotations].filter(a => a.type === 'warning');
+    const warnings = result.annotations.filter(a => a.type === 'warning');
     const resultLines: string[] = [];
     const errors = formatResultFailure(screen, test, result, '    ');
     if (!errors.length)

packages/playwright/src/reporters/html.ts

@@ -417,7 +417,7 @@ class HtmlBuilder {
         projectName,
         location,
         duration,
-        annotations: this._serializeAnnotations([...test.annotations, ...results.flatMap(r => r.annotations)]),
+        annotations: this._serializeAnnotations(test.annotations),
         tags: test.tags,
         outcome: test.outcome(),
         path,

packages/playwright/src/reporters/junit.ts

@@ -166,8 +166,7 @@ class JUnitReporter implements ReporterV2 {
       children: [] as XMLEntry[]
     };
 
-    const annotations = [...test.annotations, ...test.results.flatMap(r => r.annotations)];
+    for (const annotation of test.annotations) {
-    for (const annotation of annotations) {
       const property: XMLEntry = {
         name: 'property',
         attributes: {

packages/playwright/src/runner/dispatcher.ts

@@ -326,6 +326,7 @@ class JobDispatcher {
     result.error = result.errors[0];
     result.status = params.status;
     result.annotations = params.annotations;
+    test.annotations = [...params.annotations]; // last test result wins
     test.expectedStatus = params.expectedStatus;
     test.timeout = params.timeout;
     const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus;

packages/playwright/src/worker/workerMain.ts

@@ -293,8 +293,6 @@ export class WorkerMain extends ProcessRunner {
     for (const annotation of test.annotations)
       processAnnotation(annotation);
 
-    const staticAnnotations = new Set(testInfo.annotations);
-
     // Process existing annotations dynamically set for parent suites.
     for (const suite of suites) {
       const extraAnnotations = this._activeSuites.get(suite) || [];
@@ -313,7 +311,7 @@ export class WorkerMain extends ProcessRunner {
     if (isSkipped && nextTest && !hasAfterAllToRunBeforeNextTest) {
       // Fast path - this test is skipped, and there are more tests that will handle cleanup.
       testInfo.status = 'skipped';
-      this.dispatchEvent('testEnd', buildTestEndPayload(testInfo, staticAnnotations));
+      this.dispatchEvent('testEnd', buildTestEndPayload(testInfo));
       return;
     }
 
@@ -495,7 +493,7 @@ export class WorkerMain extends ProcessRunner {
 
     this._currentTest = null;
     setCurrentTestInfo(null);
-    this.dispatchEvent('testEnd', buildTestEndPayload(testInfo, staticAnnotations));
+    this.dispatchEvent('testEnd', buildTestEndPayload(testInfo));
 
     const preserveOutput = this._config.config.preserveOutput === 'always' ||
       (this._config.config.preserveOutput === 'failures-only' && testInfo._isFailure());
@@ -615,7 +613,7 @@ function buildTestBeginPayload(testInfo: TestInfoImpl): TestBeginPayload {
   };
 }
 
-function buildTestEndPayload(testInfo: TestInfoImpl, staticAnnotations: Set<TestAnnotation>): TestEndPayload {
+function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload {
   return {
     testId: testInfo.testId,
     duration: testInfo.duration,
@@ -623,7 +621,7 @@ function buildTestEndPayload(testInfo: TestInfoImpl, staticAnnotations: Set<Test
     errors: testInfo.errors,
     hasNonRetriableError: testInfo._hasNonRetriableError,
     expectedStatus: testInfo.expectedStatus,
-    annotations: testInfo.annotations.filter(a => !staticAnnotations.has(a)),
+    annotations: testInfo.annotations,
     timeout: testInfo.timeout,
   };
 }

packages/playwright/types/testReporter.d.ts

@@ -438,21 +438,8 @@ export interface TestCase {
   titlePath(): Array<string>;
 
   /**
-   * The list of annotations applicable to the current test. Includes:
+   * [testResult.annotations](https://playwright.dev/docs/api/class-testresult#test-result-annotations) of the last test
-   * - annotations defined on the test or suite via
+   * run.
-   *   [test.(call)(title[, details, body])](https://playwright.dev/docs/api/class-test#test-call) and
-   *   [test.describe([title, details, callback])](https://playwright.dev/docs/api/class-test#test-describe);
-   * - annotations implicitly added by methods
-   *   [test.skip([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-skip),
-   *   [test.fixme([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-fixme)
-   *   and
-   *   [test.fail([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-fail)
-   *   prior to test execution.
-   *
-   * Annotations are available during test execution through
-   * [testInfo.annotations](https://playwright.dev/docs/api/class-testinfo#test-info-annotations).
-   *
-   * Learn more about [test annotations](https://playwright.dev/docs/test-annotations).
    */
   annotations: Array<{
     /**
@@ -597,15 +584,18 @@ export interface TestError {
  */
 export interface TestResult {
   /**
-   * The list of annotations appended during test execution. Includes:
+   * The list of annotations applicable to the current test. Includes:
+   * - annotations defined on the test or suite via
+   *   [test.(call)(title[, details, body])](https://playwright.dev/docs/api/class-test#test-call) and
+   *   [test.describe([title, details, callback])](https://playwright.dev/docs/api/class-test#test-describe);
    * - annotations implicitly added by methods
    *   [test.skip([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-skip),
    *   [test.fixme([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-fixme)
    *   and
-   *   [test.fail([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-fail)
+   *   [test.fail([title, details, body, condition, callback, description])](https://playwright.dev/docs/api/class-test#test-fail);
-   *   during test execution;
    * - annotations appended to
-   *   [testInfo.annotations](https://playwright.dev/docs/api/class-testinfo#test-info-annotations).
+   *   [testInfo.annotations](https://playwright.dev/docs/api/class-testinfo#test-info-annotations) during the test
+   *   execution.
    *
    * Annotations are available during test execution through
    * [testInfo.annotations](https://playwright.dev/docs/api/class-testinfo#test-info-annotations).

packages/trace-viewer/src/ui/uiModeTraceView.tsx

@@ -88,8 +88,6 @@ export const TraceView: React.FC<{
     };
   }, [outputDir, item, setModel, counter, setCounter, pathSeparator]);
 
-  const annotations = item.testCase ? [...item.testCase.annotations, ...(item.testCase.results[0]?.annotations ?? [])] : [];
-
   return <Workbench
     key='workbench'
     model={model?.model}
@@ -98,7 +96,7 @@ export const TraceView: React.FC<{
     fallbackLocation={item.testFile}
     isLive={model?.isLive}
     status={item.treeItem?.status}
-    annotations={annotations}
+    annotations={item.testCase?.annotations ?? []}
     onOpenExternally={onOpenExternally}
     revealSource={revealSource}
   />;

tests/bidi/csvReporter.ts

@@ -46,8 +46,7 @@ class CsvReporter implements Reporter {
       for (const file of project.suites) {
         for (const test of file.allTests()) {
           // Report fixme tests as failing.
-          const annotations = [...test.annotations, ...test.results.map(r => r.annotations).flat()];
+          const fixme = test.annotations.find(a => a.type === 'fixme');
-          const fixme = annotations.find(a => a.type === 'fixme');
           if (test.ok() && !fixme)
             continue;
           const row = [];

tests/playwright-test/access-data.spec.ts

@@ -60,7 +60,7 @@ test('should access annotations in fixture', async ({ runInlineTest }) => {
   });
   expect(exitCode).toBe(0);
   const test = report.suites[0].specs[0].tests[0];
-  expect(test.results[0].annotations).toEqual([
+  expect(test.annotations).toEqual([
     { type: 'slow', description: 'just slow', location: { file: expect.any(String), line: 10, column: 14 } },
     { type: 'myname', description: 'hello' }
   ]);

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

@@ -434,7 +434,7 @@ export function expectTestHelper(result: RunResult) {
     for (const test of tests) {
       expect(test.expectedStatus, `title: ${title}`).toBe(expectedStatus);
       expect(test.status, `title: ${title}`).toBe(status);
-      expect([...test.annotations, ...test.results.flatMap(r => r.annotations)].map(a => a.type), `title: ${title}`).toEqual(annotations);
+      expect(test.annotations.map(a => a.type), `title: ${title}`).toEqual(annotations);
     }
   };
 }

tests/playwright-test/retry.spec.ts

@@ -260,5 +260,5 @@ test('failed and skipped on retry should be marked as flaky', async ({ runInline
   expect(result.failed).toBe(0);
   expect(result.flaky).toBe(1);
   expect(result.output).toContain('Failed on first run');
-  expect(result.report.suites[0].specs[0].tests[0].results[1].annotations).toEqual([{ type: 'skip', description: 'Skipped on first retry', location: expect.anything() }]);
+  expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'skip', description: 'Skipped on first retry', location: expect.anything() }]);
 });

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

@@ -106,7 +106,7 @@ test('test modifiers should work', async ({ runInlineTest }) => {
     const test = spec.tests[0];
     expect(test.expectedStatus).toBe(expectedStatus);
     expect(test.results[0].status).toBe(status);
-    expect([...test.annotations, ...test.results.flatMap(r => r.annotations)]).toEqual(annotations);
+    expect(test.annotations).toEqual(annotations);
   };
   expectTest('passed1', 'passed', 'passed', []);
   expectTest('passed2', 'passed', 'passed', []);
@@ -407,7 +407,7 @@ test('should skip inside fixture', async ({ runInlineTest }) => {
   });
   expect(result.exitCode).toBe(0);
   expect(result.skipped).toBe(1);
-  expect(result.report.suites[0].specs[0].tests[0].results[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 20 } }]);
+  expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 20 } }]);
 });
 
 test('modifier with a function should throw in the test', async ({ runInlineTest }) => {
@@ -460,8 +460,8 @@ test('test.skip with worker fixtures only should skip before hooks and tests', a
   expect(result.passed).toBe(1);
   expect(result.skipped).toBe(2);
   expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([]);
-  expect(result.report.suites[0].suites![0].specs[0].tests[0].results[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 14, column: 14 } }]);
+  expect(result.report.suites[0].suites![0].specs[0].tests[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 14, column: 14 } }]);
-  expect(result.report.suites[0].suites![0].suites![0].specs[0].tests[0].results[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 14, column: 14 } }]);
+  expect(result.report.suites[0].suites![0].suites![0].specs[0].tests[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 14, column: 14 } }]);
   expect(result.outputLines).toEqual([
     'beforeEach',
     'passed',
@@ -598,8 +598,8 @@ test('should skip all tests from beforeAll', async ({ runInlineTest }) => {
     'beforeAll',
     'afterAll',
   ]);
-  expect(result.report.suites[0].specs[0].tests[0].results[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 14 } }]);
+  expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 14 } }]);
-  expect(result.report.suites[0].specs[1].tests[0].results[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 14 } }]);
+  expect(result.report.suites[0].specs[1].tests[0].annotations).toEqual([{ type: 'skip', description: 'reason', location: { file: expect.any(String), line: 5, column: 14 } }]);
 });
 
 test('should report skipped tests in-order with correct properties', async ({ runInlineTest }) => {
@@ -695,7 +695,7 @@ test('static modifiers should be added in serial mode', async ({ runInlineTest }
   expect(result.passed).toBe(0);
   expect(result.skipped).toBe(2);
   expect(result.didNotRun).toBe(1);
-  expect(result.report.suites[0].specs[0].tests[0].results[0].annotations).toEqual([{ type: 'slow', location: { file: expect.any(String), line: 6, column: 14 } }]);
+  expect(result.report.suites[0].specs[0].tests[0].annotations).toEqual([{ type: 'slow', location: { file: expect.any(String), line: 6, column: 14 } }]);
   expect(result.report.suites[0].specs[1].tests[0].annotations).toEqual([{ type: 'fixme', location: { file: expect.any(String), line: 9, column: 12 } }]);
   expect(result.report.suites[0].specs[2].tests[0].annotations).toEqual([{ type: 'skip', location: { file: expect.any(String), line: 11, column: 12 } }]);
   expect(result.report.suites[0].specs[3].tests[0].annotations).toEqual([]);