chore: remove retries and fix expectations (#9897)

.github/workflows/changed-packages.yml

@@ -41,6 +41,7 @@ jobs:
               - 'test/**'
               - 'test-d/**'
               - 'tools/mochaRunner/**'
+              - '.mocharc.cjs'
             website:
               - '.github/workflows/ci.yml'
               - 'docs/**'

.mocharc.cjs

@@ -20,7 +20,7 @@ module.exports = {
   require: ['./test/build/mocha-utils.js', 'source-map-support/register'],
   spec: 'test/build/**/*.spec.js',
   exit: !!process.env.CI,
-  retries: process.env.CI ? 2 : 0,
+  retries: 0,
   parallel: !!process.env.PARALLEL,
   timeout: 25_000,
   reporter: process.env.CI ? 'spec' : 'dot',

package.json

@@ -17,6 +17,7 @@
     "docs": "run-s build:docs generate:markdown",
     "format:eslint": "eslint --ext js --ext ts --fix .",
     "format:prettier": "prettier --write .",
+    "format:expectations": "node tools/sort-test-expectations.js",
     "format": "run-s format:*",
     "generate:markdown": "tsx tools/generate_docs.ts",
     "lint:eslint": "([ \"$CI\" = true ] && eslint --ext js --ext ts --quiet -f codeframe . || eslint --ext js --ext ts .)",

test/TestSuites.json

@@ -3,31 +3,31 @@
     {
       "id": "chrome-headless",
       "platforms": ["linux", "win32", "darwin"],
-      "parameters": ["chrome", "headless"],
+      "parameters": ["chrome", "headless", "cdp"],
       "expectedLineCoverage": 93
     },
     {
       "id": "chrome-headful",
       "platforms": ["linux"],
-      "parameters": ["chrome", "headful"],
+      "parameters": ["chrome", "headful", "cdp"],
       "expectedLineCoverage": 93
     },
     {
       "id": "chrome-new-headless",
       "platforms": ["linux"],
-      "parameters": ["chrome", "new-headless"],
+      "parameters": ["chrome", "new-headless", "cdp"],
       "expectedLineCoverage": 93
     },
     {
       "id": "firefox-headless",
       "platforms": ["linux", "darwin"],
-      "parameters": ["firefox", "headless"],
+      "parameters": ["firefox", "headless", "cdp"],
       "expectedLineCoverage": 80
     },
     {
       "id": "firefox-headful",
       "platforms": ["linux"],
-      "parameters": ["firefox", "headful"],
+      "parameters": ["firefox", "headful", "cdp"],
       "expectedLineCoverage": 80
     },
     {
@@ -61,6 +61,7 @@
     },
     "webDriverBiDi": {
       "PUPPETEER_PROTOCOL": "webDriverBiDi"
-    }
+    },
+    "cdp": {}
   }
 }

tools/mochaRunner/src/utils.ts

@@ -72,6 +72,14 @@ export function printSuggestions(
         return item.expectation;
       })
     );
+    console.log(
+      'The recommendations are based on the following applied expectaions:'
+    );
+    prettyPrintJSON(
+      toPrint.map(item => {
+        return item.basedOn;
+      })
+    );
   }
 }
 
@@ -103,6 +111,7 @@ export function findEffectiveExpectationForTest(
 export type RecommendedExpectation = {
   expectation: TestExpectation;
   action: 'remove' | 'add' | 'update';
+  basedOn?: TestExpectation;
 };
 
 export function isWildCardPattern(testIdPattern: string): boolean {
@@ -134,6 +143,7 @@ export function getExpectationUpdates(
       addEntry({
         expectation: expectationEntry,
         action: 'remove',
+        basedOn: expectationEntry,
       });
     }
   }
@@ -149,28 +159,39 @@ export function getExpectationUpdates(
       expectations,
       failure
     );
-    // If the effective explanation is a wildcard, we recommend adding a new
-    // expectation instead of updating the wildcard that might affect multiple
-    // tests.
-    if (
-      expectationEntry &&
-      !isWildCardPattern(expectationEntry.testIdPattern)
-    ) {
+    if (expectationEntry) {
       if (
         !expectationEntry.expectations.includes(
           getTestResultForFailure(failure)
         )
       ) {
-        addEntry({
-          expectation: {
-            ...expectationEntry,
-            expectations: [
-              ...expectationEntry.expectations,
-              getTestResultForFailure(failure),
-            ],
-          },
-          action: 'update',
-        });
+        // If the effective explanation is a wildcard, we recommend adding a new
+        // expectation instead of updating the wildcard that might affect multiple
+        // tests.
+        if (isWildCardPattern(expectationEntry.testIdPattern)) {
+          addEntry({
+            expectation: {
+              testIdPattern: getTestId(failure.file, failure.fullTitle),
+              platforms: context.platforms,
+              parameters: context.parameters,
+              expectations: [getTestResultForFailure(failure)],
+            },
+            action: 'add',
+            basedOn: expectationEntry,
+          });
+        } else {
+          addEntry({
+            expectation: {
+              ...expectationEntry,
+              expectations: [
+                ...expectationEntry.expectations,
+                getTestResultForFailure(failure),
+              ],
+            },
+            action: 'update',
+            basedOn: expectationEntry,
+          });
+        }
       }
     } else {
       addEntry({
@@ -181,6 +202,7 @@ export function getExpectationUpdates(
           expectations: [getTestResultForFailure(failure)],
         },
         action: 'add',
+        basedOn: expectationEntry,
       });
     }
   }

tools/sort-test-expectations.js

@@ -0,0 +1,59 @@
+/**
+ * Copyright 2023 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// TODO: this could be an eslint rule probably.
+
+const fs = require('fs');
+const path = require('path');
+
+const prettier = require('prettier');
+
+const source = 'test/TestExpectations.json';
+
+const testExpectations = JSON.parse(fs.readFileSync(source, 'utf-8'));
+
+function getSpecificity(item) {
+  return (
+    item.parameters.length +
+    (item.testIdPattern.includes('*')
+      ? item.testIdPattern === '*'
+        ? 0
+        : 1
+      : 2)
+  );
+}
+
+testExpectations.sort((a, b) => {
+  const result = getSpecificity(a) - getSpecificity(b);
+  if (result === 0) {
+    return a.testIdPattern.localeCompare(b.testIdPattern);
+  }
+  return result;
+});
+
+testExpectations.forEach(item => {
+  item.parameters.sort();
+  item.expectations.sort();
+  item.platforms.sort();
+});
+
+fs.writeFileSync(
+  source,
+  prettier.format(JSON.stringify(testExpectations), {
+    ...require(path.join(__dirname, '..', '.prettierrc.cjs')),
+    parser: 'json',
+  })
+);