From 4fa1030af697f8c9f84dbd0d3c944bb59ab00d1b Mon Sep 17 00:00:00 2001
From: Pavel Feldman <pavel.feldman@gmail.com>
Date: Thu, 25 Apr 2024 12:48:41 -0700
Subject: [PATCH] chore: hide skipped tests by default (#30546)

Fixes https://github.com/microsoft/playwright/issues/30540
---
 packages/html-reporter/src/filter.ts          | 51 +++++++++++--------
 .../html-reporter/src/headerView.spec.tsx     |  2 +-
 packages/html-reporter/src/headerView.tsx     |  2 +-
 tests/playwright-test/reporter-blob.spec.ts   | 20 +++++---
 tests/playwright-test/reporter-html.spec.ts   | 28 +++-------
 5 files changed, 51 insertions(+), 52 deletions(-)

diff --git a/packages/html-reporter/src/filter.ts b/packages/html-reporter/src/filter.ts
index 97cf675936..f8c61ba902 100644
--- a/packages/html-reporter/src/filter.ts
+++ b/packages/html-reporter/src/filter.ts
@@ -98,27 +98,7 @@ export class Filter {
   }
 
   matches(test: TestCaseSummary): boolean {
-    if (!(test as any).searchValues) {
-      let status = 'passed';
-      if (test.outcome === 'unexpected')
-        status = 'failed';
-      if (test.outcome === 'flaky')
-        status = 'flaky';
-      if (test.outcome === 'skipped')
-        status = 'skipped';
-      const searchValues: SearchValues = {
-        text: (status + ' ' + test.projectName + ' ' + test.tags.join(' ') + ' ' + test.location.file + ' ' + test.path.join(' ') + ' ' + test.title).toLowerCase(),
-        project: test.projectName.toLowerCase(),
-        status: status as any,
-        file: test.location.file,
-        line: String(test.location.line),
-        column: String(test.location.column),
-        labels: test.tags.map(tag => tag.toLowerCase()),
-      };
-      (test as any).searchValues = searchValues;
-    }
-
-    const searchValues = (test as any).searchValues as SearchValues;
+    const searchValues = cacheSearchValues(test);
     if (this.project.length) {
       const matches = !!this.project.find(p => searchValues.project.includes(p));
       if (!matches)
@@ -128,6 +108,9 @@ export class Filter {
       const matches = !!this.status.find(s => searchValues.status.includes(s));
       if (!matches)
         return false;
+    } else {
+      if (searchValues.status === 'skipped')
+        return false;
     }
     if (this.text.length) {
       for (const text of this.text) {
@@ -159,3 +142,29 @@ type SearchValues = {
   labels: string[];
 };
 
+const searchValuesSymbol = Symbol('searchValues');
+
+function cacheSearchValues(test: TestCaseSummary): SearchValues {
+  const cached = (test as any)[searchValuesSymbol] as SearchValues | undefined;
+  if (cached)
+    return cached;
+
+  let status: SearchValues['status'] = 'passed';
+  if (test.outcome === 'unexpected')
+    status = 'failed';
+  if (test.outcome === 'flaky')
+    status = 'flaky';
+  if (test.outcome === 'skipped')
+    status = 'skipped';
+  const searchValues: SearchValues = {
+    text: (status + ' ' + test.projectName + ' ' + test.tags.join(' ') + ' ' + test.location.file + ' ' + test.path.join(' ') + ' ' + test.title).toLowerCase(),
+    project: test.projectName.toLowerCase(),
+    status,
+    file: test.location.file,
+    line: String(test.location.line),
+    column: String(test.location.column),
+    labels: test.tags.map(tag => tag.toLowerCase()),
+  };
+  (test as any)[searchValuesSymbol] = searchValues;
+  return searchValues;
+}
diff --git a/packages/html-reporter/src/headerView.spec.tsx b/packages/html-reporter/src/headerView.spec.tsx
index a0b27e6943..64ed858475 100644
--- a/packages/html-reporter/src/headerView.spec.tsx
+++ b/packages/html-reporter/src/headerView.spec.tsx
@@ -28,7 +28,7 @@ test('should render counters', async ({ mount }) => {
     skipped: 10,
     ok: false,
   }} filterText='' setFilterText={() => {}}></HeaderView>);
-  await expect(component.locator('a', { hasText: 'All' }).locator('.counter')).toHaveText('100');
+  await expect(component.locator('a', { hasText: 'All' }).locator('.counter')).toHaveText('90');
   await expect(component.locator('a', { hasText: 'Passed' }).locator('.counter')).toHaveText('42');
   await expect(component.locator('a', { hasText: 'Failed' }).locator('.counter')).toHaveText('31');
   await expect(component.locator('a', { hasText: 'Flaky' }).locator('.counter')).toHaveText('17');
diff --git a/packages/html-reporter/src/headerView.tsx b/packages/html-reporter/src/headerView.tsx
index 04513eb7dc..1dc5420eec 100644
--- a/packages/html-reporter/src/headerView.tsx
+++ b/packages/html-reporter/src/headerView.tsx
@@ -66,7 +66,7 @@ const StatsNavView: React.FC<{
 }> = ({ stats }) => {
   return <nav>
     <Link className='subnav-item' href='#?'>
-      All <span className='d-inline counter'>{stats.total}</span>
+      All <span className='d-inline counter'>{stats.total - stats.skipped}</span>
     </Link>
     <Link className='subnav-item' href='#?q=s:passed'>
       Passed <span className='d-inline counter'>{stats.expected}</span>
diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts
index eb62661d95..c940a6f0d6 100644
--- a/tests/playwright-test/reporter-blob.spec.ts
+++ b/tests/playwright-test/reporter-blob.spec.ts
@@ -216,16 +216,16 @@ test('should merge into html with dependencies', async ({ runInlineTest, mergeRe
 
   await showReport();
 
-  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('13');
+  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('10');
   await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('6');
   await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('2');
   await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('2');
   await expect(page.locator('.subnav-item:has-text("Skipped") .counter')).toHaveText('3');
 
   await expect(page.locator('.test-file-test .test-file-title')).toHaveText([
-    'failing 1', 'flaky 1', 'math 1', 'skipped 1',
-    'failing 2', 'math 2', 'skipped 2',
-    'flaky 2', 'math 3', 'skipped 3',
+    'failing 1', 'flaky 1', 'math 1',
+    'failing 2', 'math 2',
+    'flaky 2', 'math 3',
     'login once', 'login once', 'login once',
   ]);
 
@@ -284,7 +284,7 @@ test('should merge blob into blob', async ({ runInlineTest, mergeReports, showRe
     expect(exitCode).toBe(0);
     expect(fs.existsSync(test.info().outputPath('report.json'))).toBe(true);
     await showReport();
-    await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('7');
+    await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('5');
     await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('2');
     await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('2');
     await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('1');
@@ -340,7 +340,7 @@ test('be able to merge incomplete shards', async ({ runInlineTest, mergeReports,
 
   await showReport();
 
-  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('6');
+  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('4');
   await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('2');
   await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('1');
   await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('1');
@@ -912,7 +912,7 @@ test('onError in the report', async ({ runInlineTest, mergeReports, showReport,
 
   await showReport();
 
-  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('3');
+  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('2');
   await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('2');
   await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('0');
   await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('0');
@@ -1418,7 +1418,7 @@ test('should merge blob reports with same name', async ({ runInlineTest, mergeRe
   const { exitCode } = await mergeReports(allReportsDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
   await showReport();
-  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('14');
+  await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('10');
   await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('4');
   await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('4');
   await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('2');
@@ -1809,6 +1809,10 @@ test('preserve static annotations when tests did not run', async ({ runInlineTes
   const { exitCode } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
   await showReport();
+
+  // Show skipped tests.
+  await page.getByText('Skipped').click();
+
   // Check first annotation.
   {
     await page.getByRole('link', { name: 'first' }).click();
diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts
index 76aa6a3e8a..afc8fa5dda 100644
--- a/tests/playwright-test/reporter-html.spec.ts
+++ b/tests/playwright-test/reporter-html.spec.ts
@@ -69,7 +69,7 @@ for (const useIntermediateMergeReport of [false, true] as const) {
 
       await showReport();
 
-      await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('4');
+      await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('3');
       await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('1');
       await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('1');
       await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('1');
@@ -78,7 +78,7 @@ for (const useIntermediateMergeReport of [false, true] as const) {
       await expect(page.locator('.test-file-test-outcome-unexpected >> text=fails')).toBeVisible();
       await expect(page.locator('.test-file-test-outcome-flaky >> text=flaky')).toBeVisible();
       await expect(page.locator('.test-file-test-outcome-expected >> text=passes')).toBeVisible();
-      await expect(page.locator('.test-file-test-outcome-skipped >> text=skipped')).toBeVisible();
+      await expect(page.locator('.test-file-test-outcome-skipped >> text=skipped')).not.toBeVisible();
 
       await expect(page.getByTestId('overall-duration'), 'should contain humanized total time with at most 1 decimal place').toContainText(/^Total time: \d+(\.\d)?(ms|s|m)$/);
       await expect(page.getByTestId('project-name'), 'should contain project name').toContainText('project-name');
@@ -690,17 +690,17 @@ for (const useIntermediateMergeReport of [false, true] as const) {
         `,
         'a.test.js': `
           import { test, expect } from '@playwright/test';
-          test('skipped test', async ({ page }) => {
-            test.skip(true, 'I am not interested in this test');
+          test('annotated test', async ({ page }) => {
+            test.info().annotations.push({ type: 'issue', description: 'I am not interested in this test' });
           });
         `,
       }, { reporter: 'dot,html' }, { PW_TEST_HTML_REPORT_OPEN: 'never' });
       expect(result.exitCode).toBe(0);
-      expect(result.skipped).toBe(1);
+      expect(result.passed).toBe(1);
 
       await showReport();
-      await page.click('text=skipped test');
-      await expect(page.locator('.test-case-annotation')).toHaveText('skip: I am not interested in this test');
+      await page.click('text=annotated test');
+      await expect(page.locator('.test-case-annotation')).toHaveText('issue: I am not interested in this test');
     });
 
     test('should render annotations as link if needed', async ({ runInlineTest, page, showReport, server }) => {
@@ -1231,20 +1231,6 @@ for (const useIntermediateMergeReport of [false, true] as const) {
           'regression',
           'flaky',
         ]);
-        await expect(page.locator('.test-file-test', { has: page.getByText('@regression skipped', { exact: true }) }).locator('.label')).toHaveText([
-          'chromium',
-          'regression',
-          'foo',
-          'bar',
-          'firefox',
-          'regression',
-          'foo',
-          'bar',
-          'webkit',
-          'regression',
-          'foo',
-          'bar',
-        ]);
         await expect(page.locator('.test-file-test', { has: page.getByText('@smoke @passed passed', { exact: true }) }).locator('.label')).toHaveText([
           'chromium',
           'smoke',