Speed up playwright (pt2: full parallelization) (#1720)

* Fully separate all popup tests * Shard playwright CI
yomidevs · Dec 31, 2024 · 2157d69 · 2157d69
1 parent 26cd2a8
commit 2157d69
Show file tree

Hide file tree

Showing 4 changed files with 164 additions and 100 deletions.
diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml
@@ -6,15 +6,15 @@ on:
 permissions:
   contents: read
 jobs:
-  playwright:
+  playwright-tests:
     timeout-minutes: 60
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        shardIndex: [1, 2, 3, 4, 5]
+        shardTotal: [5]
     steps:
-      - name: Harden Runner
-        uses: step-security/harden-runner@v2
-        with:
-          egress-policy: audit
-
       - name: Remove all fonts
         run: rm -rf /usr/share/fonts
 
@@ -66,43 +66,126 @@ jobs:
       - name: "[PR] Generate new screenshots & compare against master"
         id: playwright
         run: |
-          npx playwright test 2>&1 | tee ./playwright-output || true
+          npx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }} 2>&1 | tee ./playwright-output || true
         continue-on-error: true
         if: github.event_name == 'pull_request' && steps.master-screenshots.outcome != 'failure'
 
       - name: "[Push] Generate new authoritative screenshots for master"
         id: playwright-master
-        run: npx playwright test -u
+        run: npx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }} -u
         if: github.event_name == 'push'
 
       - uses: actions/upload-artifact@v4
         with:
-          name: playwright-screenshots
+          name: playwright-screenshots-${{ matrix.shardIndex }}
           path: test/playwright/__screenshots__/
+          retention-days: 1
 
-      - uses: actions/upload-artifact@v4
+      - name: Upload blob report to GitHub Actions Artifacts
+        if: ${{ !cancelled() }}
+        uses: actions/upload-artifact@v4
         with:
-          name: playwright-report
-          path: playwright-report/
+          name: blob-report-${{ matrix.shardIndex }}
+          path: blob-report
+          retention-days: 1
 
       - name: "[PR] Store steps.master-screenshots.outcome in a file"
         run: echo ${{ steps.master-screenshots.outcome }} > ./master-screenshots-outcome
         if: github.event_name == 'pull_request'
 
       - uses: actions/upload-artifact@v4
         with:
-          name: master-screenshots-outcome
+          name: master-screenshots-outcome-${{ matrix.shardIndex }}
           path: master-screenshots-outcome
         if: github.event_name == 'pull_request'
 
       - uses: actions/upload-artifact@v4
         with:
-          name: playwright-output
+          name: playwright-output-${{ matrix.shardIndex }}
           path: playwright-output
+          retention-days: 1
         if: github.event_name == 'pull_request'
 
+  playwright:
+    timeout-minutes: 60
+    runs-on: ubuntu-latest
+    # Merge reports after playwright-tests, even if some shards have failed
+    if: ${{ !cancelled() }}
+    needs: [playwright-tests]
+    steps:
+      - uses: actions/checkout@v4
+      - uses: ./.github/actions/setup
+
+      ### playwright-report
+      - name: Download blob reports from GitHub Actions Artifacts
+        uses: actions/download-artifact@v4
+        with:
+          path: all-blob-reports
+          pattern: blob-report-*
+          merge-multiple: true
+
+      - name: Merge into HTML Report
+        run: npx playwright merge-reports --reporter html ./all-blob-reports
+
+      - name: Merge into JSON Report
+        run: npx playwright merge-reports --reporter json ./all-blob-reports > playwright-results.json
+
+      - uses: actions/upload-artifact@v4
+        with:
+          name: playwright-report
+          path: playwright-report/
+
       - uses: actions/upload-artifact@v4
         with:
           name: playwright-results-json
           path: playwright-results.json
-        if: github.event_name == 'pull_request'
+
+      ### playwright-screenshots
+      - name: Download & merge screenshots from GitHub Actions Artifacts
+        uses: actions/download-artifact@v4
+        with:
+          path: test/playwright/__screenshots__
+          pattern: playwright-screenshots-*
+          merge-multiple: true
+
+      - uses: actions/upload-artifact@v4
+        with:
+          name: playwright-screenshots
+          path: test/playwright/__screenshots__/
+
+      ### master-screenshots-outcome
+      - name: Download master-screenshots-outcome files
+        uses: actions/download-artifact@v4
+        with:
+          path: master-screenshots-outcomes
+          pattern: master-screenshots-outcome-*
+          merge-multiple: true
+
+      - name: Check if any shard failed to download master screenshots
+        run: |
+          if grep -q "failure" master-screenshots-outcomes/*; then
+            echo "failure" > master-screenshots-outcome
+          else
+            echo "success" > master-screenshots-outcome
+          fi
+
+      - uses: actions/upload-artifact@v4
+        with:
+          name: master-screenshots-outcome
+          path: master-screenshots-outcome
+
+      ### playwright-output
+      - name: Download playwright-output files
+        uses: actions/download-artifact@v4
+        with:
+          path: playwright-outputs
+          pattern: playwright-output-*
+          merge-multiple: true
+
+      - name: Merge playwright output files
+        run: cat playwright-outputs/* > playwright-output
+
+      - uses: actions/upload-artifact@v4
+        with:
+          name: playwright-output
+          path: playwright-output
diff --git a/.github/workflows/playwright_comment.yml b/.github/workflows/playwright_comment.yml
@@ -36,14 +36,6 @@ jobs:
           run_id: ${{ github.event.workflow_run.id }}
           name: master-screenshots-outcome
 
-      - name: Grab playwright-results-json from PR run
-        uses: dawidd6/action-download-artifact@80620a5d27ce0ae443b965134db88467fc607b43 # v7
-        continue-on-error: true
-        with:
-          github_token: ${{ secrets.GITHUB_TOKEN }}
-          run_id: ${{ github.event.workflow_run.id }}
-          name: playwright-results-json
-
       - name: Dry-run grab playwright-report from PR run so we have its ID
         uses: dawidd6/action-download-artifact@80620a5d27ce0ae443b965134db88467fc607b43 # v7
         id: playwright-report
@@ -62,12 +54,6 @@ jobs:
           ID=$(echo "$ARTIFACTS_JSON" | jq -r '.[0].id');
           echo "id=$ID" >> "$GITHUB_OUTPUT"
 
-      - name: Generate summary from playwright-results.json (expected to fail to comment)
-        id: playwright-summary
-        uses: daun/playwright-report-summary@v3
-        with:
-          report-file: playwright-results.json
-
       - name: Load artifacts into environment variables
         id: playwright
         run: |
@@ -79,8 +65,8 @@ jobs:
             echo "MASTER_SCREENSHOTS_OUTCOME<<$EOF";
             cat ./master-screenshots-outcome;
             echo "$EOF";
-            echo "FAILED=$(grep -c '^ *[0-9] failed$' ./playwright-output)";
-            echo "FLAKY=$(grep -c '^ *[0-9] flaky$' ./playwright-output)"
+            echo "FAILED=$(jq -r '.stats.unexpected' playwright-results.json)";
+            echo "FLAKY=$(jq -r '.stats.flaky' playwright-results.json)";
           } >> "$GITHUB_OUTPUT"
 
       # this is required because github.event.workflow_run.pull_requests is not available for PRs from forks

diff --git a/playwright.config.js b/playwright.config.js
@@ -47,10 +47,7 @@ export default defineConfig({
     /* Opt out of parallel tests on CI. */
     workers: process.env.CI ? 1 : void 0,
     /* Reporter to use. See https://playwright.dev/docs/test-reporters */
-    reporter: [
-        ['html'],
-        ['json', {outputFile: 'playwright-results.json'}],
-    ],
+    reporter: process.env.CI ? 'blob' : 'html',
     /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
     use: {
         /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */

diff --git a/test/playwright/visual.spec.js b/test/playwright/visual.spec.js
@@ -15,6 +15,7 @@
  * along with this program.  If not, see <https://www.gnu.org/licenses/>.
  */
 
+import {readFileSync} from 'fs';
 import path from 'path';
 import {pathToFileURL} from 'url';
 import {createDictionaryArchiveData} from '../../dev/dictionary-archive-util.js';
@@ -95,71 +96,68 @@ test('settings', async ({page, extensionId}) => {
     });
 });
 
-test('popup', async ({page, extensionId}) => {
-    // Open settings
-    console.log('Open settings');
-    await page.goto(`chrome-extension://${extensionId}/settings.html`);
-
-    await expect(page.locator('id=dictionaries')).toBeVisible();
-
-    // Load in test dictionary
-    const dictionary = await createDictionaryArchiveData(path.join(root, 'test/data/dictionaries/valid-dictionary1'), 'valid-dictionary1');
-    await page.locator('input[id="dictionary-import-file-input"]').setInputFiles({
-        name: 'valid-dictionary1.zip',
-        mimeType: 'application/x-zip',
-        buffer: Buffer.from(dictionary),
+test.describe('popup', () => {
+    test.beforeEach(async ({page, extensionId}) => {
+        // Open settings
+        console.log('Open settings');
+        await page.goto(`chrome-extension://${extensionId}/settings.html`);
+
+        await expect(page.locator('id=dictionaries')).toBeVisible();
+
+        // Load in test dictionary
+        const dictionary = await createDictionaryArchiveData(path.join(root, 'test/data/dictionaries/valid-dictionary1'), 'valid-dictionary1');
+        await page.locator('input[id="dictionary-import-file-input"]').setInputFiles({
+            name: 'valid-dictionary1.zip',
+            mimeType: 'application/x-zip',
+            buffer: Buffer.from(dictionary),
+        });
+        await expect(page.locator('id=dictionaries')).toHaveText('Dictionaries (1 installed, 1 enabled)', {timeout: 1 * 60 * 1000});
+
+        console.log('Open popup-tests.html');
+        await page.goto(pathToFileURL(popupTestsPath).toString());
+        await page.setViewportSize({width: 1000, height: 4500});
+        await expect(page.locator('id=footer')).toBeVisible();
+        await page.keyboard.down('Shift');
     });
-    await expect(page.locator('id=dictionaries')).toHaveText('Dictionaries (1 installed, 1 enabled)', {timeout: 1 * 60 * 1000});
-
-    /**
-     * @param {number} doc_number
-     * @param {number} test_number
-     * @param {import('@playwright/test').Locator} hovertarget_locator
-     * @param {{x: number, y: number}} offset
-     */
-    const screenshot = async (doc_number, test_number, hovertarget_locator, offset) => {
-        const test_name = 'doc' + doc_number + '-test' + test_number;
-        console.log(test_name);
-
-        const box = (await hovertarget_locator.boundingBox()) || {x: 0, y: 0, width: 0, height: 0};
-
-        // Find the popup frame if it exists
-        let popup_frame = page.frames().find((f) => f.url().includes('popup.html'));
-
-        // Otherwise prepare for it to be attached
-        let frame_attached;
-        if (typeof popup_frame === 'undefined') {
-            frame_attached = page.waitForEvent('frameattached');
-        }
-        await page.mouse.move(box.x + offset.x, box.y + offset.y, {steps: 10}); // Hover over the test
-        if (typeof popup_frame === 'undefined') {
-            popup_frame = await frame_attached; // Wait for popup to be attached
-        }
-
-        try {
-            const expectedState = (await hovertarget_locator.locator('..').getAttribute('data-expected-result')) === 'failure' ? 'hidden' : 'visible';
-            await (await /** @type {import('@playwright/test').Frame} */ (popup_frame).frameElement()).waitForElementState(expectedState, {timeout: 500});
-        } catch (error) {
-            console.warn(test_name, 'unexpected popup state');
-        }
-
-        console.log(test_name, 'taking screenshot');
-        await page.bringToFront(); // Bring the page to the foreground so the screenshot doesn't hang; for some reason the frames result in page being in the background
-        await expect.soft(page).toHaveScreenshot(test_name + '.png');
-
-        console.log(test_name, 'clicking away and waiting');
-        await page.mouse.click(0, 0); // Click away so popup disappears
-        await (await /** @type {import('@playwright/test').Frame} */ (popup_frame).frameElement()).waitForElementState('hidden'); // Wait for popup to disappear
-    };
-
-    console.log('Open popup-tests.html');
-    await page.goto(pathToFileURL(path.join(root, 'test/data/html/popup-tests.html')).toString());
-    await page.setViewportSize({width: 1000, height: 4500});
-    await expect(page.locator('id=footer')).toBeVisible();
-    await page.keyboard.down('Shift');
-    let i = 1;
-    for (const test_locator of await page.locator('.hovertarget').all()) {
-        await screenshot(2, i, test_locator, {x: 15, y: 15});
-        i++;
+    const popupTestsPath = path.join(root, 'test/data/html/popup-tests.html');
+    const numberOfTests = (readFileSync(popupTestsPath, 'utf8').match(/hovertarget/g) || []).length;
+    for (let i = 1; i <= numberOfTests; i++) {
+        test(`test${i}`, async ({page}) => {
+            const test_name = 'doc2-test' + i;
+            console.log(test_name);
+
+            // Find the test element
+            const test_locator = page.locator('.hovertarget').nth(i - 1);
+
+            const box = (await test_locator.boundingBox()) || {x: 0, y: 0, width: 0, height: 0};
+
+            const expectedState = (await test_locator.locator('..').getAttribute('data-expected-result')) === 'failure' ? 'hidden' : 'visible';
+
+            try {
+                const frame_attached = page.waitForEvent('frameattached', {timeout: 5000});
+                await page.mouse.move(box.x - 5, box.y - 5); // Hover near the test
+                await page.mouse.move(box.x + 15, box.y + 15, {steps: 10}); // Hover over the test
+                if (expectedState === 'visible') {
+                    const popup_frame = await frame_attached;
+                    await (await /** @type {import('@playwright/test').Frame} */ (popup_frame).frameElement())
+                        .waitForElementState(expectedState, {timeout: 500});
+                } else {
+                    expect(
+                        await Promise.race([
+                            frame_attached,
+                            new Promise((resolve) => {
+                                setTimeout(() => resolve('timeout'), 2000);
+                            }),
+                        ]),
+                    ).toStrictEqual('timeout');
+                }
+            } catch (error) {
+                console.warn(test_name, 'unexpected popup state', error);
+            }
+
+            console.log(test_name, 'taking screenshot');
+            await page.bringToFront(); // Bring the page to the foreground so the screenshot doesn't hang; for some reason the frames result in page being in the background
+            await expect.soft(page).toHaveScreenshot(test_name + '.png');
+        });
     }
 });