Make duck player page more resilient to race conditions on macOS

.github/workflows/tests.yml

@@ -69,9 +69,15 @@ jobs:
       if: always()
       with:
         name: playwright-report
-        path: playwright-report/
+        path: test-results
         retention-days: 5
     - run: npm run test-int-x
+    - uses: actions/upload-artifact@v2
+      if: always()
+      with:
+        name: playwright-report-pages
+        path: packages/special-pages/test-results
+        retention-days: 5
     - name: Build docs
       run: npm run docs
 

packages/special-pages/.gitignore

@@ -1 +1,2 @@
 test-results
+playwright-report

packages/special-pages/package.json

@@ -6,16 +6,16 @@
   "main": "index.js",
   "scripts": {
     "build": "node index.mjs",
-    "build.dev": "node index.mjs --debug",
+    "build.dev": "node index.mjs --env development",
     "test": "playwright test",
     "test.windows": "npm run test -- --project duckplayer-windows",
     "test.apple": "npm run test -- --project duckplayer-apple",
     "test.headed": "npm run test -- --headed",
     "test.ui": "npm run test -- --ui",
     "pretest": "npm run build.dev",
     "pretest.headed": "npm run build.dev",
-    "test-int-x": "playwright test",
-    "test-int": "playwright test",
+    "test-int-x": "npm run test",
+    "test-int": "npm run test",
     "serve": "http-server -c-1 --port 3210 ../../"
   },
   "license": "ISC",

packages/special-pages/pages/duckplayer/src/js/index.js

@@ -31,7 +31,12 @@
  *
  * Please see {@link DuckPlayerPageMessages} for the up-to-date list
  */
-import { createDuckPlayerPageMessaging, DuckPlayerPageMessages, UserValues } from './messages'
+import {
+    callWithRetry,
+    createDuckPlayerPageMessaging,
+    DuckPlayerPageMessages,
+    UserValues
+} from './messages'
 import { html } from '../../../../../../src/dom-utils'
 import { initStorage } from './storage'
 
@@ -325,23 +330,31 @@ const Comms = {
      * @param {ImportMeta['env']} opts.env
      * @param {ImportMeta['injectName']} opts.injectName
      */
-    init: (opts) => {
-        Comms.messaging = createDuckPlayerPageMessaging(opts)
-        // eslint-disable-next-line promise/prefer-await-to-then
-        Comms.messaging.getUserValues().then((value) => {
-            if ('enabled' in value.privatePlayerMode) {
-                Setting.setState(true)
-            } else {
-                Setting.setState(false)
-            }
+    init: async (opts) => {
+        const messaging = createDuckPlayerPageMessaging(opts)
+        // try to make communication with the native side.
+        const result = await callWithRetry(() => {
+            return messaging.getUserValues()
         })
-        Comms.messaging?.onUserValuesChanged(value => {
-            if ('enabled' in value.privatePlayerMode) {
+        // if we received a connection, use the initial values
+        if ('value' in result) {
+            Comms.messaging = messaging
+            if ('enabled' in result.value.privatePlayerMode) {
                 Setting.setState(true)
             } else {
                 Setting.setState(false)
             }
-        })
+            // eslint-disable-next-line promise/prefer-await-to-then
+            Comms.messaging?.onUserValuesChanged(value => {
+                if ('enabled' in value.privatePlayerMode) {
+                    Setting.setState(true)
+                } else {
+                    Setting.setState(false)
+                }
+            })
+        } else {
+            console.error(result.error)
+        }
     },
     /**
      * From the player page, all we can do is 'setUserValues' to {enabled: {}}
@@ -700,14 +713,18 @@ const MouseMove = {
 /**
  * Initializes all parts of the page on load.
  */
-document.addEventListener('DOMContentLoaded', () => {
+document.addEventListener('DOMContentLoaded', async () => {
     Setting.init({
         settingsUrl: settingsUrl(import.meta.injectName)
     })
-    Comms.init({
+    await Comms.init({
         injectName: import.meta.injectName,
         env: import.meta.env
     })
+    if (!Comms.messaging) {
+        console.warn('cannot continue as messaging was not resolved')
+        return
+    }
     VideoPlayer.init()
     Tooltip.init()
     PlayOnYouTube.init({

packages/special-pages/pages/duckplayer/src/js/messages.js

@@ -159,3 +159,40 @@ export function createDuckPlayerPageMessaging (opts) {
     }
     throw new Error('unreachable - platform not supported')
 }
+
+/**
+ * This will return either { value: awaited value },
+ *                         { error: error message }
+ *
+ * It will execute the given function in uniform intervals
+ * until either:
+ *   1: the given function stops throwing errors
+ *   2: the maxAttempts limit is reached
+ *
+ * This is useful for situations where you don't want to continue
+ * until a result is found - normally to work around race-conditions
+ *
+ * @template {(...args: any[]) => any} FN
+ * @param {FN} fn
+ * @param {{maxAttempts?: number, intervalMs?: number}} params
+ * @returns {Promise<{ value: Awaited<ReturnType<FN>>, attempt: number } | { error: string }>}
+ */
+export async function callWithRetry (fn, params = {}) {
+    const { maxAttempts = 10, intervalMs = 300 } = params
+    let attempt = 1
+
+    while (attempt <= maxAttempts) {
+        try {
+            return { value: await fn(), attempt }
+        } catch (error) {
+            if (attempt === maxAttempts) {
+                return { error: `Max attempts reached: ${error}` }
+            }
+
+            await new Promise((resolve) => setTimeout(resolve, intervalMs))
+            attempt++
+        }
+    }
+
+    return { error: 'Unreachable: value not retrieved' }
+}

packages/special-pages/playwright.config.js

@@ -14,11 +14,20 @@ export default defineConfig({
         }
     ],
     fullyParallel: !process.env.CI,
+    /* Retry on CI only */
+    retries: process.env.CI ? 2 : 0,
+    /* Opt out of parallel tests on CI. */
+    workers: process.env.CI ? 1 : undefined,
+    reporter: process.env.CI ? 'github' : [['html', { open: 'never' }]],
     // @ts-expect-error - Type 'undefined' is not assignable to type 'string'. process.env
     webServer: {
         command: 'npm run serve',
         port: 3210,
         reuseExistingServer: true,
         env: process.env
+    },
+    use: {
+        actionTimeout: 1000,
+        trace: 'on-first-retry'
     }
 })

packages/special-pages/tests/duckplayer.spec.js

@@ -82,7 +82,7 @@ test.describe('duckplayer toolbar', () => {
 })
 
 test.describe('duckplayer settings', () => {
-    test.skip('always open setting', async ({ page }, workerInfo) => {
+    test('always open setting', async ({ page }, workerInfo) => {
         const duckplayer = DuckPlayerPage.create(page, workerInfo)
         // load as normal
         await duckplayer.openWithVideoID()
@@ -98,7 +98,7 @@ test.describe('duckplayer settings', () => {
         await duckplayer.toggleAlwaysOpenSetting()
         await duckplayer.sentUpdatedSettings()
     })
-    test.skip('when a new value arrives via subscription', async ({ page }, workerInfo) => {
+    test('when a new value arrives via subscription', async ({ page }, workerInfo) => {
         const duckplayer = DuckPlayerPage.create(page, workerInfo)
         // load as normal
         await duckplayer.openWithVideoID()

playwright.config.js

@@ -41,6 +41,7 @@ export default defineConfig({
     retries: process.env.CI ? 2 : 0,
     /* Opt out of parallel tests on CI. */
     workers: process.env.CI ? 1 : undefined,
+    reporter: process.env.CI ? 'github' : [['html', { open: 'never' }]],
     /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
     webServer: {
         reuseExistingServer: true,