chore(e2e): handle and surface retries + more fixes (#567)

* chore(e2e): fix parsing of JUnit XML file numbers

These are all returned as strings, so it's easy to introduce bugs downstream if we don't immediately
convert them explicitly to numbers.

This fixes some cases where we render `NaN` on the report page.

* chore(e2e): fix rendering of skipped tests in main section

If all the tests in a suite were individually skipped, we ended up with an empty section and a `Nan%`.

* ci(e2e): fix default for workflow_dispatch versions input

* chore(e2e): handle retries in next.js e2e results

Remove all suites known to be fully passing from the `e2e-skip-retry.json`, add logic to handle
retries in the test results, thread this through to the final results, and show retry info on the
report page.

Every suite was added to this list in order to tame the test run time, but now that we're around a
98% pass rate, the impact of enabling retries should be minimal.

* refactor(e2e): simplify and clarify junit2json skip handling

* chore(e2e): don't duplicate tests skipped via config

These were ending up inserted because they're skipped and inserted a second time if they also ran
and failed. The same could technically happen if it passed and was skipped.

Author: serhalp

.github/workflows/test-e2e.yml

@@ -9,9 +9,11 @@ on:
   workflow_dispatch:
     inputs:
       versions:
-        description: 'The versions of Next.js to test against (quoted and comma separated)'
+        description: 'The versions of Next.js to test against (escape-quoted and comma separated)'
         required: false
-        default: "\"latest\""
+        # TODO(serhalp) Ideally this would simply accept bare quotes but we're having trouble
+        # parsing that so this will do for now.
+        default: "\\\"latest\\\""
 concurrency:
   group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.ref }}
   cancel-in-progress: true

e2e-report/app/globals.scss

@@ -341,6 +341,11 @@ button.nav {
   td h4 {
     font-size: clamp(1rem, 0.5127rem + 1.6242vw, 1.4rem);
   }
+
+  .retries {
+    font-style: italic;
+    font-size: inherit;
+  }
 }
 
 .card {

e2e-report/components/filter-data.js

@@ -87,10 +87,13 @@ export const OpenIssues = ({ testCases }) => {
             <th>Reason</th>
           </tr>
           {testCases.map((testCase, index) => {
-            const { name, link, reason = 'Reason not yet assigned' } = testCase
+            const { name, link, reason = 'Reason not yet assigned', retries } = testCase
             return (
               <tr key={index}>
-                <td>{name}</td>
+                <td>
+                  {name}
+                  {retries > 0 ? ` (🔁 retries: ${retries})` : null}
+                </td>
                 <td>
                   <p>
                     {link ? (

e2e-report/components/grouped-tests.js

@@ -48,6 +48,12 @@ export default function GroupedTests({ testData }) {
             </div>
             <div className={`testGroup ${slider[testGroup.id] ? 'open' : 'close'}`}>
               {testGroup.results
+                // We don't want to show skipped tests in this section
+                .map((suite) => ({
+                  ...suite,
+                  testCases: suite.testCases?.filter(({ status }) => status !== 'skipped'),
+                }))
+                .filter((suite) => suite.testCases?.length > 0)
                 .sort(
                   (aa, bb) =>
                     (bb.passed || 0) + (bb.failed || 0) - ((aa.passed || 0) + (aa.failed || 0)),

e2e-report/components/table.js

@@ -1,4 +1,4 @@
-export default function Table({ th, name, suitesTotal, total, passed }) {
+export default function Table({ th, name, suitesTotal, total, passed, retries }) {
   return (
     <table>
       <tbody>
@@ -21,6 +21,7 @@ export default function Table({ th, name, suitesTotal, total, passed }) {
           </td>
           <td>
             <h4>{Math.round((passed / total) * 100)}%</h4>
+            {retries > 0 ? <span className="retries"> (⚠️ retries: {retries})</span> : null}
           </td>
         </tr>
       </tbody>

e2e-report/components/test-suites.js

@@ -1,7 +1,7 @@
 import Table from './table.js'
 
 export default function TestSuites({ suite, slider, handleSelect, arrows }) {
-  const { name, file, passed, failed, testCases } = suite
+  const { name, file, passed, failed, testCases, retries } = suite
 
   return (
     <>
@@ -11,17 +11,14 @@ export default function TestSuites({ suite, slider, handleSelect, arrows }) {
           name={name}
           passed={passed}
           total={passed + failed}
+          retries={retries}
         />
         {arrows(slider[name])}
       </div>
       <ul className={`testCases ${slider[name] ? 'open' : 'close'}`}>
         {testCases?.map((testCase, index) => {
           const { status, link, reason } = testCase
 
-          if (status === 'skipped') {
-            return
-          }
-
           return (
             <li key={`${name}${index}`}>
               <a target="_blank" href={`https://github.com/vercel/next.js/tree/canary/${file}`}>

tests/e2e-skip-retry.json

@@ -39,6 +39,7 @@ interface TestSuite {
   skipped: number
   total: number
   testCases: TestCase[]
+  retries: number
 }
 
 interface SkippedTestSuite {
@@ -53,6 +54,7 @@ interface TestCase {
   status: 'passed' | 'failed' | 'skipped'
   reason?: string
   link?: string
+  retries: number
 }
 
 async function parseXMLFile(filePath: string): Promise<{ testsuites: JUnitTestSuites }> {
@@ -66,9 +68,7 @@ const testCount = {
   passed: 0,
 }
 
-function junitToJson(xmlData: {
-  testsuites: JUnitTestSuites
-}): Array<TestSuite | SkippedTestSuite> {
+function junitToJson(xmlData: { testsuites: JUnitTestSuites }): Array<TestSuite> {
   if (!xmlData.testsuites) {
     return []
   }
@@ -78,38 +78,57 @@ function junitToJson(xmlData: {
     : [xmlData.testsuites.testsuite]
 
   return testSuites.map((suite) => {
-    const { '@tests': tests, '@failures': failed, '@name': name } = suite
-
-    const passed = tests - failed - suite['@skipped']
-
+    const total = Number(suite['@tests'])
+    const failed = Number(suite['@failures']) + Number(suite['@errors'])
+    const name = suite['@name']
     const testCases = Array.isArray(suite.testcase) ? suite.testcase : [suite.testcase]
 
+    const passed = total - failed - suite['@skipped']
     const testSuite: TestSuite = {
       name,
       file: testCases[0]?.['@file'],
       passed,
-      failed: Number(failed),
+      failed,
+      // The XML file contains a count of "skipped" tests, but we actually want to report on what WE
+      // want to "skip" (i.e. the tests that are marked as skipped in `test-config.json`). This is
+      // confusing and we should probably use a different term for our concept.
       skipped: 0,
-      total: tests,
+      total,
       testCases: [],
+      // This is computed below by detecting duplicates
+      retries: 0,
     }
-    const skippedTestsForFile = testConfig.skipped.find(
+    const skipConfigForFile = testConfig.skipped.find(
       (skippedTest) => skippedTest.file === testSuite.file,
     )
+    const isEntireSuiteSkipped = skipConfigForFile != null && skipConfigForFile.tests == null
+    const skippedTestsForFile =
+      skipConfigForFile?.tests?.map((skippedTest) => {
+        // The config supports both a bare string and an object
+        if (typeof skippedTest === 'string') {
+          return { name: skippedTest, reason: skipConfigForFile.reason ?? null }
+        }
+        return skippedTest
+      }) ?? []
 
     // If the skipped file has no `tests`, all tests in the file are skipped
-    testSuite.skipped =
-      skippedTestsForFile != null ? (skippedTestsForFile.tests ?? testCases).length : 0
+    testSuite.skipped = isEntireSuiteSkipped ? testCases.length : skippedTestsForFile.length
 
     for (const testCase of testCases) {
+      // Omit tests skipped in the Next.js repo itself
       if ('skipped' in testCase) {
         continue
       }
+      // Omit tests we've marked as "skipped" in `test-config.json`
+      if (skippedTestsForFile?.some(({ name }) => name === testCase['@name'])) {
+        continue
+      }
       const status = testCase.failure ? 'failed' : 'passed'
       testCount[status]++
       const test: TestCase = {
         name: testCase['@name'],
         status,
+        retries: 0,
       }
       if (status === 'failed') {
         const failure = testConfig.failures.find(
@@ -123,36 +142,74 @@ function junitToJson(xmlData: {
       testSuite.testCases.push(test)
     }
 
-    if (skippedTestsForFile?.tests) {
-      testCount.skipped += skippedTestsForFile.tests.length
+    if (!isEntireSuiteSkipped && skippedTestsForFile.length > 0) {
+      testCount.skipped += skippedTestsForFile.length
       testSuite.testCases.push(
-        ...skippedTestsForFile.tests.map((test): TestCase => {
-          if (typeof test === 'string') {
-            return {
-              name: test,
-              status: 'skipped',
-              reason: skippedTestsForFile.reason,
-            }
-          }
-          return {
+        ...skippedTestsForFile.map(
+          (test): TestCase => ({
             name: test.name,
             status: 'skipped',
             reason: test.reason,
-          }
-        }),
+            retries: 0,
+          }),
+        ),
       )
-    } else if (skippedTestsForFile != null) {
-      // If `tests` is omitted, all tests in the file are skipped
+    } else if (isEntireSuiteSkipped) {
       testCount.skipped += testSuite.total
     }
     return testSuite
   })
 }
 
+function mergeTestResults(result1: TestSuite, result2: TestSuite): TestSuite {
+  if (result1.file !== result2.file) {
+    throw new Error('Cannot merge results for different files')
+  }
+  if (result1.name !== result2.name) {
+    throw new Error('Cannot merge results for different suites')
+  }
+  if (result1.total !== result2.total) {
+    throw new Error('Cannot merge results with different total test counts')
+  }
+
+  // Return the run result with the fewest failures.
+  // We could merge at the individual test level across runs, but then we'd need to re-calculate
+  // all the total counts, and that probably isn't worth the complexity.
+  const bestResult = result1.failed < result2.failed ? result1 : result2
+  const retries = result1.retries + result2.retries + 1
+  return {
+    ...bestResult,
+    retries,
+    ...(bestResult.testCases == null
+      ? {}
+      : {
+          testCases: bestResult.testCases.map((testCase) => ({
+            ...testCase,
+            retries,
+          })),
+        }),
+  }
+}
+
+// When a test is run multiple times (due to retries), the test runner outputs a separate entry
+// for each run. Merge them into a single entry.
+function dedupeTestResults(results: Array<TestSuite>): Array<TestSuite> {
+  const resultsByFile = new Map<string, TestSuite>()
+  for (const result of results) {
+    const existingResult = resultsByFile.get(result.file)
+    if (existingResult == null) {
+      resultsByFile.set(result.file, result)
+    } else {
+      resultsByFile.set(result.file, mergeTestResults(existingResult, result))
+    }
+  }
+  return [...resultsByFile.values()]
+}
+
 async function processJUnitFiles(
   directoryPath: string,
 ): Promise<Array<TestSuite | SkippedTestSuite>> {
-  const results: (TestSuite | SkippedTestSuite)[] = []
+  const results: TestSuite[] = []
   for await (const file of expandGlob(`${directoryPath}/**/*.xml`)) {
     const xmlData = await parseXMLFile(file.path)
     results.push(...junitToJson(xmlData))
@@ -170,9 +227,8 @@ async function processJUnitFiles(
         skipped: true,
       }),
     )
-  results.push(...skippedSuites)
 
-  return results
+  return [...dedupeTestResults(results), ...skippedSuites]
 }
 
 // Get the directory path from the command-line arguments