Improve reliability of Click to Load integration tests

While they run reliably locally, some of the Click to Load integration tests are flaking on CI. These improvements aim to help reduce those flakes: - If a test page fails to load, let's mark the test as skipped rather than as a failure. There's not much we can do to get around network failure. - Take care to wait between unblocking a YouTube video and clicking to play it. - Take care to wait for pages to finish closing. - Avoid exceeding Jasmine's 20 second timeout when waiting for the extension configuration to load. If extension configuration fails to load, throw a clear exception to explain that. - When clicking the Facebook Click to Load buttons, ignore buttons that Puppeteer fails to click. - Fix a bug where the test extension configuration would still attempt to be unloaded, even when the test configuration failed to load. - When API schema tests timeout, mark them as skipped with a clear message.
duckduckgo · May 18, 2022 · 897f8d1 · 897f8d1
1 parent d97b3c9
commit 897f8d1
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 47 deletions.
diff --git a/integration-test/background/click-to-load-facebook.js b/integration-test/background/click-to-load-facebook.js
@@ -4,6 +4,7 @@ const harness = require('../helpers/harness')
 const { logPageRequests } = require('../helpers/requests')
 const { loadTestConfig, unloadTestConfig } = require('../helpers/testConfig')
 const backgroundWait = require('../helpers/backgroundWait')
+const pageWait = require('../helpers/pageWait')
 const { setupAPISchemaTest } = require('../helpers/apiSchema')
 
 const testSite = 'https://privacy-test-pages.glitch.me/privacy-protections/click-to-load/'
@@ -76,8 +77,7 @@ describe('Test Facebook Click To Load', () => {
         // Initially there should be a bunch of requests. The SDK should
         // be redirected to our surrogate but otherwise Facebook requests should
         // be blocked.
-        await page.goto(testSite, { waitUntil: 'networkidle0' })
-        await page.waitForNetworkIdle({ idleTime: 1000 })
+        await pageWait.forGoto(page, testSite)
         {
             const {
                 requestCount, blockCount, allowCount, facebookSDKRedirect
@@ -102,7 +102,9 @@ describe('Test Facebook Click To Load', () => {
         })
         for (let i = 0; i < buttonCount; i++) {
             const button = await page.evaluateHandle(i => window.buttons[i], i)
-            await button.click()
+            try {
+                await button.click()
+            } catch (e) { }
         }
         await page.waitForNetworkIdle({ idleTime: 1000 })
         {
@@ -119,8 +121,7 @@ describe('Test Facebook Click To Load', () => {
 
         // When the page is reloaded, requests should be blocked again.
         clearRequests()
-        await page.reload({ waitUntil: 'networkidle0' })
-        await page.waitForNetworkIdle({ idleTime: 1000 })
+        await pageWait.forReload(page)
         {
             const {
                 requestCount, blockCount, allowCount, facebookSDKRedirect
@@ -135,7 +136,7 @@ describe('Test Facebook Click To Load', () => {
             expect(allowCount).toEqual(0)
         }
 
-        page.close()
+        await page.close()
     })
 })
 
@@ -152,7 +153,7 @@ describe('Facebook SDK schema', () => {
 
     it('CTL: Facebook SDK schema hasn\'t changed', async () => {
         const page = await browser.newPage()
-        await page.goto(testSite, { waitUntil: 'networkidle2' })
+        await pageWait.forGoto(page, testSite)
 
         // Note: If these tests fail, update the
         //       /integration-test/data/api_schemas/facebook-sdk.json file
@@ -170,6 +171,6 @@ describe('Facebook SDK schema', () => {
         )
         expect(actualSchema).toEqual(expectedSchema)
 
-        page.close()
+        await page.close()
     })
 })
diff --git a/integration-test/background/click-to-load-youtube.js b/integration-test/background/click-to-load-youtube.js
@@ -2,6 +2,7 @@ const harness = require('../helpers/harness')
 const { logPageRequests } = require('../helpers/requests')
 const { loadTestConfig, unloadTestConfig } = require('../helpers/testConfig')
 const backgroundWait = require('../helpers/backgroundWait')
+const pageWait = require('../helpers/pageWait')
 const { setupAPISchemaTest } = require('../helpers/apiSchema')
 
 const testSite = 'https://privacy-test-pages.glitch.me/privacy-protections/youtube-click-to-load/'
@@ -81,7 +82,7 @@ describe('Test YouTube Click To Load', () => {
         // Initially there should be a bunch of requests. The iframe_api should
         // be redirected to our surrogate but otherwise YouTube requests should
         // be blocked.
-        await page.goto(testSite, { waitUntil: 'networkidle0' })
+        await pageWait.forGoto(page, testSite)
         {
             const {
                 youTubeIframeApi, youTubeStandard, youTubeNocookie
@@ -119,7 +120,7 @@ describe('Test YouTube Click To Load', () => {
 
         // When the page is reloaded, requests should be blocked again.
         clearRequests()
-        await page.reload({ waitUntil: 'networkidle0' })
+        await pageWait.forReload(page)
         {
             const {
                 youTubeIframeApi, youTubeStandard, youTubeNocookie
@@ -154,12 +155,12 @@ describe('Test YouTube Click To Load', () => {
             expect(youTubeNocookie.allowed).toBeGreaterThanOrEqual(1)
         }
 
-        page.close()
+        await page.close()
     })
 
     it('CTL: YouTube interacting with iframe API', async () => {
         const page = await browser.newPage()
-        await page.goto(testSite, { waitUntil: 'networkidle0' })
+        await pageWait.forGoto(page, testSite)
 
         // Test the Iframe API controls and events function correctly, even when
         // used with an existing video.
@@ -179,6 +180,7 @@ describe('Test YouTube Click To Load', () => {
             )
             await button.click()
             await waitForExpectedBorder('orange')
+            await page.waitForNetworkIdle({ idleTime: 1000 })
 
             await page.click('#play-existing-video')
             await waitForExpectedBorder('green')
@@ -210,6 +212,7 @@ describe('Test YouTube Click To Load', () => {
             await button.focus()
             await button.click()
             await waitForExpectedRoll('0.0000')
+            await page.waitForNetworkIdle({ idleTime: 1000 })
 
             // Play video and keep clicking roll button until it flips. The
             // video doesn't flip until its finished loading, so this way we
@@ -221,7 +224,7 @@ describe('Test YouTube Click To Load', () => {
             await waitForExpectedRoll('0.0000')
         }
 
-        page.close()
+        await page.close()
     })
 })
 
@@ -239,7 +242,7 @@ describe('YouTube Iframe Player API schema', () => {
 
     it('CTL: Iframe Player API schema hasn\'t changed', async () => {
         const page = await browser.newPage()
-        await page.goto(testSite, { waitUntil: 'networkidle2' })
+        await pageWait.forGoto(page, testSite)
 
         // Note: If this test fails, update
         //       /integration-test/data/api_schemas/youtube-iframe-api.json file
@@ -255,6 +258,6 @@ describe('YouTube Iframe Player API schema', () => {
         )
         expect(actualSchema).toEqual(expectedSchema)
 
-        page.close()
+        await page.close()
     })
 })
diff --git a/integration-test/helpers/apiSchema.js b/integration-test/helpers/apiSchema.js
@@ -1,6 +1,8 @@
 const fs = require('fs')
 const path = require('path')
 
+const puppeteer = require('puppeteer')
+
 /**
  * Returns the schema of a given JavaScript Object in the given Page. Schema is
  * similar to the output of Object.getOwnPropertyDescriptors.
@@ -100,7 +102,15 @@ async function setupAPISchemaTest (page, schemaFilename, targetObjectNames) {
 
     const actualSchema = Object.create(null)
     for (const objectName of targetObjectNames) {
-        actualSchema[objectName] = await getObjectSchema(page, objectName)
+        try {
+            actualSchema[objectName] = await getObjectSchema(page, objectName)
+        } catch (e) {
+            if (e instanceof puppeteer.errors.TimeoutError) {
+                pending('Timed out setting up API schema test.')
+            } else {
+                throw e
+            }
+        }
     }
 
     // Write the actual schema to a file as a test artifact.

diff --git a/integration-test/helpers/backgroundWait.js b/integration-test/helpers/backgroundWait.js
@@ -1,10 +1,13 @@
 // Helpers that aid in waiting for the background page's state.
-// Note: Puppeteer's `waitForFunction` uses `requestAnimationFrame` by default,
-//       but that appears to be broken for extension background pages. Polling
-//       every 10ms works however.
-
+// Notes:
+//   - Puppeteer's `waitForFunction` uses `requestAnimationFrame` by default,
+//     but that appears to be broken for extension background pages. Polling
+//     every 10ms works however.
+//   - Jasmine's timeout is 20 seconds, so go with a timeout of 15 seconds
+//     (rather than the default of 30 seconds) to improve the error output on
+//     timeout.
 function forFunction (bgPage, func, ...args) {
-    return bgPage.waitForFunction(func, { polling: 10 }, ...args)
+    return bgPage.waitForFunction(func, { polling: 10, timeout: 15000 }, ...args)
 }
 
 function forSetting (bgPage, key) {
@@ -15,34 +18,38 @@ function forSetting (bgPage, key) {
 
 // Note: This is likely incomplete. Please add further checks as they come up!
 async function forAllConfiguration (bgPage) {
-    await forFunction(bgPage, () =>
-        // Expected configuration Objects/properties exist.
-        window.dbg?.https &&
-        window.dbg?.tds?.ClickToLoadConfig &&
-        window.dbg?.tds?.config?.features?.clickToPlay?.state &&
-        window.dbg?.tds?.tds?.domains &&
-        window.dbg?.tds?.tds?.entities &&
-        window.dbg?.tds?.tds?.trackers &&
+    try {
+        await forFunction(bgPage, () =>
+            // Expected configuration Objects/properties exist.
+            window.dbg?.https &&
+            window.dbg?.tds?.ClickToLoadConfig &&
+            window.dbg?.tds?.config?.features?.clickToPlay?.state &&
+            window.dbg?.tds?.tds?.domains &&
+            window.dbg?.tds?.tds?.entities &&
+            window.dbg?.tds?.tds?.trackers &&
 
-        // Expected configuration state.
-        window.dbg.tds.isInstalling === false &&
-        window.dbg.https.isReady === true &&
-        Object.keys(window.dbg.tds.ClickToLoadConfig).length > 0 &&
-        Object.keys(window.dbg.tds.tds.domains).length > 0 &&
-        Object.keys(window.dbg.tds.tds.entities).length > 0
-    )
+            // Expected configuration state.
+            window.dbg.tds.isInstalling === false &&
+            window.dbg.https.isReady === true &&
+            Object.keys(window.dbg.tds.ClickToLoadConfig).length > 0 &&
+            Object.keys(window.dbg.tds.tds.domains).length > 0 &&
+            Object.keys(window.dbg.tds.tds.entities).length > 0)
 
-    await forFunction(bgPage, () => {
-        const firstEntity = Object.keys(window.dbg.tds.tds.entities)[0]
-        const { domains } = window.dbg.tds.tds.entities[firstEntity]
-        return firstEntity &&
-               domains && domains.length > 0 &&
-               window.dbg.tds.tds.domains[domains[0]] === firstEntity
-    })
+        await Promise.all([
+            forFunction(bgPage, () => {
+                const firstEntity = Object.keys(window.dbg.tds.tds.entities)[0]
+                const { domains } = window.dbg.tds.tds.entities[firstEntity]
+                return firstEntity &&
+                    domains && domains.length > 0 &&
+                    window.dbg.tds.tds.domains[domains[0]] === firstEntity
+            }),
 
-    // This is a synchronous way to check that the Promise returned by
-    // `settings.ready()` has resolved.
-    await forSetting(bgPage, 'atb')
+            // Ensures that `settings.ready()` has resolved.
+            forSetting(bgPage, 'atb')
+        ])
+    } catch (e) {
+        throw new Error('Failed to load extension configuration.')
+    }
 }
 
 module.exports = {

diff --git a/integration-test/helpers/pageWait.js b/integration-test/helpers/pageWait.js
@@ -0,0 +1,22 @@
+async function forGoto (page, url) {
+    try {
+        await page.goto(url, { waitUntil: 'networkidle0' })
+    } catch (e) {
+        pending('Failed to load URL: ' + url)
+    }
+    await page.waitForNetworkIdle({ idleTime: 1000 })
+}
+
+async function forReload (page) {
+    try {
+        await page.reload({ waitUntil: 'networkidle0' })
+    } catch (e) {
+        pending('Failed to reload page: ' + page.url())
+    }
+    await page.waitForNetworkIdle({ idleTime: 1000 })
+}
+
+module.exports = {
+    forGoto,
+    forReload
+}
diff --git a/integration-test/helpers/testConfig.js b/integration-test/helpers/testConfig.js
@@ -83,7 +83,11 @@ async function loadTestConfig (bgPage, testConfigFilename) {
  */
 async function unloadTestConfig (bgPage) {
     await bgPage.evaluate(parsePathString => {
-        const { configBackup } = window || {}
+        const { configBackup } = window
+        if (!configBackup) {
+            return
+        }
+
         // eslint-disable-next-line no-eval
         eval(parsePathString)