From 9c15dc7dcc29799c2aee5a5491245f95dd263e73 Mon Sep 17 00:00:00 2001 From: Jeffrey Posnick Date: Sun, 14 Feb 2021 11:39:25 -0500 Subject: [PATCH] Address tab-related flakiness in integration tests (#2754) * Redo webdriver tab stuff * whitespace * Cleanup * Wait on activation and controlling * Skip a test in Safari --- infra/testing/webdriver/TabManager.js | 49 ++++++++ .../testing/webdriver/getLastWindowHandle.js | 52 -------- infra/testing/webdriver/openNewTab.js | 36 ------ infra/testing/webdriver/windowLoaded.js | 3 +- .../sw/test-BackgroundSyncPlugin.mjs | 7 +- .../integration/test-all.js | 93 ++++++-------- test/workbox-window/integration/test-all.js | 113 +++++++++--------- test/workbox-window/static/index.html | 12 ++ 8 files changed, 161 insertions(+), 204 deletions(-) create mode 100644 infra/testing/webdriver/TabManager.js delete mode 100644 infra/testing/webdriver/getLastWindowHandle.js delete mode 100644 infra/testing/webdriver/openNewTab.js diff --git a/infra/testing/webdriver/TabManager.js b/infra/testing/webdriver/TabManager.js new file mode 100644 index 000000000..ba4914a6b --- /dev/null +++ b/infra/testing/webdriver/TabManager.js @@ -0,0 +1,49 @@ +/* + Copyright 2021 Google LLC + + Use of this source code is governed by an MIT-style + license that can be found in the LICENSE file or at + https://opensource.org/licenses/MIT. +*/ + +/** + * Wraps methods in the underlying webdriver API to create, switch between, + * and close tabs in a browser. + */ +class TabManager { + /** + * @param {WebDriver} driver + * + * @private + */ + constructor(driver) { + this._driver = driver; + this._openedHandles = new Set(); + this._initialHandle = null; + } + + async openTab(url) { + if (this._initialHandle === null) { + this._initialHandle = await this._driver.getWindowHandle(); + } + + await this._driver.switchTo().newWindow('tab'); + this._openedHandles.add(await this._driver.getWindowHandle()); + + await this._driver.get(url); + } + + async closeOpenedTabs() { + for (const handle of this._openedHandles) { + await this._driver.switchTo().window(handle); + await this._driver.close(); + } + this._openedHandles = new Set(); + + if (this._initialHandle) { + await this._driver.switchTo().window(this._initialHandle); + } + } +} + +module.exports = {TabManager}; diff --git a/infra/testing/webdriver/getLastWindowHandle.js b/infra/testing/webdriver/getLastWindowHandle.js deleted file mode 100644 index 820cea931..000000000 --- a/infra/testing/webdriver/getLastWindowHandle.js +++ /dev/null @@ -1,52 +0,0 @@ -/* - Copyright 2019 Google LLC - - Use of this source code is governed by an MIT-style - license that can be found in the LICENSE file or at - https://opensource.org/licenses/MIT. -*/ - - -// Store local references of these globals. -const {server, webdriver} = global.__workbox; - -const testServerOrigin = server.getAddress(); - -/** - * Gets the window handle of the last opened tab. - * - * @return {string} - */ -const getLastWindowHandle = async () => { - let lastWindowHandle; - - // Save the handle so that we can switch back before returning. - const currentWindowHandle = await webdriver.getWindowHandle(); - - const allWindowHandles = await webdriver.getAllWindowHandles(); - // reverse() the list so that we will iterate through the last one first. - allWindowHandles.reverse(); - - for (const handle of allWindowHandles) { - await webdriver.switchTo().window(handle); - const currentUrl = await webdriver.getCurrentUrl(); - if (currentUrl.startsWith(testServerOrigin)) { - lastWindowHandle = handle; - break; - } else { - // Used for debugging failing tests with unexpected windows openning. - // eslint-disable-next-line no-console - console.log(`Unexpected window opened: ${currentUrl}`); - } - } - - await webdriver.switchTo().window(currentWindowHandle); - if (lastWindowHandle) { - return lastWindowHandle; - } - - // If we can't find anything, treat that as a fatal error. - throw new Error(`Unable to a window with origin ${testServerOrigin}.`); -}; - -module.exports = {getLastWindowHandle}; diff --git a/infra/testing/webdriver/openNewTab.js b/infra/testing/webdriver/openNewTab.js deleted file mode 100644 index 4bb022431..000000000 --- a/infra/testing/webdriver/openNewTab.js +++ /dev/null @@ -1,36 +0,0 @@ -/* - Copyright 2019 Google LLC - - Use of this source code is governed by an MIT-style - license that can be found in the LICENSE file or at - https://opensource.org/licenses/MIT. -*/ - - -const {getLastWindowHandle} = require('./getLastWindowHandle'); - -// Store local references of these globals. -const {webdriver} = global.__workbox; - -/** - * Opens a new window for the passed URL. If no URL is passed, a blank page - * is opened. - * - * @param {string} url - * @param {Object} options - * @return {string} - */ -const openNewTab = async (url) => { - await webdriver.executeAsyncScript((url, cb) => { - window.open(url); - cb(); - }, url); - - const lastHandle = await getLastWindowHandle(); - await webdriver.switchTo().window(lastHandle); - - // Return the handle of the window that was just opened. - return lastHandle; -}; - -module.exports = {openNewTab}; diff --git a/infra/testing/webdriver/windowLoaded.js b/infra/testing/webdriver/windowLoaded.js index 0a37ded10..03a74e76f 100644 --- a/infra/testing/webdriver/windowLoaded.js +++ b/infra/testing/webdriver/windowLoaded.js @@ -17,8 +17,7 @@ const windowLoaded = async () => { await executeAsyncAndCatch(async (cb) => { const loaded = () => { if (!window.Workbox) { - const error = new Error('Workbox not yet loaded...'); - cb({error: error.stack}); + cb({error: `window.Workbox is undefined; location is ${location.href}`}); } else { cb(); } diff --git a/test/workbox-background-sync/sw/test-BackgroundSyncPlugin.mjs b/test/workbox-background-sync/sw/test-BackgroundSyncPlugin.mjs index d530bd066..77cd62121 100644 --- a/test/workbox-background-sync/sw/test-BackgroundSyncPlugin.mjs +++ b/test/workbox-background-sync/sw/test-BackgroundSyncPlugin.mjs @@ -9,6 +9,11 @@ import {Queue} from 'workbox-background-sync/Queue.mjs'; import {BackgroundSyncPlugin} from 'workbox-background-sync/BackgroundSyncPlugin.mjs'; +let count = 0; +function getUniqueQueueName() { + return `queue-${count++}`; +} + describe(`BackgroundSyncPlugin`, function() { const sandbox = sinon.createSandbox(); @@ -27,7 +32,7 @@ describe(`BackgroundSyncPlugin`, function() { describe(`constructor`, function() { it(`should implement fetchDidFail and add requests to the queue`, async function() { const stub = sandbox.stub(Queue.prototype, 'pushRequest'); - const queuePlugin = new BackgroundSyncPlugin('a'); + const queuePlugin = new BackgroundSyncPlugin(getUniqueQueueName()); queuePlugin.fetchDidFail({request: new Request('/one')}); expect(stub.callCount).to.equal(1); diff --git a/test/workbox-broadcast-update/integration/test-all.js b/test/workbox-broadcast-update/integration/test-all.js index 0ce2fd34f..99410b266 100644 --- a/test/workbox-broadcast-update/integration/test-all.js +++ b/test/workbox-broadcast-update/integration/test-all.js @@ -7,15 +7,15 @@ */ const expect = require('chai').expect; -const activateAndControlSW = require('../../../infra/testing/activate-and-control'); + const {runUnitTests} = require('../../../infra/testing/webdriver/runUnitTests'); -const {openNewTab} = require('../../../infra/testing/webdriver/openNewTab'); -const {getLastWindowHandle} = require('../../../infra/testing/webdriver/getLastWindowHandle'); +const {TabManager} = require('../../../infra/testing/webdriver/TabManager'); +const activateAndControlSW = require('../../../infra/testing/activate-and-control'); +const cleanSWEnv = require('../../../infra/testing/clean-sw'); const templateData = require('../../../infra/testing/server/template-data'); - // Store local references of these globals. -const {webdriver, server} = global.__workbox; +const {webdriver, server, seleniumBrowser} = global.__workbox; describe(`[workbox-broadcast-update]`, function() { it(`passes all SW unit tests`, async function() { @@ -29,11 +29,13 @@ describe(`[workbox-broadcast-update] Plugin`, function() { const swURL = `${testingURL}sw.js`; const apiURL = `${testServerAddress}/__WORKBOX/uniqueETag`; - it(`should broadcast a message when there's a cache update to a regular request`, async function() { - await webdriver.get(testingURL); + beforeEach(async function() { + // Navigate to our test page and clear all caches before this test runs. + await cleanSWEnv(global.__workbox.webdriver, testingURL); await activateAndControlSW(swURL); - await clearAllCaches(); + }); + it(`should broadcast a message when there's a cache update to a regular request`, async function() { // Fetch `apiURL`, which should put it in the cache (but not trigger an update) const err1 = await webdriver.executeAsyncScript((apiURL, cb) => { fetch(apiURL).then(() => cb()).catch((err) => cb(err.message)); @@ -68,13 +70,9 @@ describe(`[workbox-broadcast-update] Plugin`, function() { }); it(`should broadcast a message when there's a cache update to a navigation request`, async function() { - await webdriver.get(testingURL); - await activateAndControlSW(swURL); - await clearAllCaches(); - templateData.assign({ title: 'Broadcast Cache Update Test', - body: '', + body: 'Second test, initial body.', script: ` window.__messages = []; navigator.serviceWorker.addEventListener('message', (event) => { @@ -83,7 +81,7 @@ describe(`[workbox-broadcast-update] Plugin`, function() { `, }); - const dynamicPageURL = testingURL + 'integration.html.njk'; + const dynamicPageURL = testingURL + 'integration.html.njk?second'; // Navigate to a dynamic page whose content can be updated from with this // test, and wait until the cache is populated. @@ -95,9 +93,9 @@ describe(`[workbox-broadcast-update] Plugin`, function() { }); // Update the template data with new content, - // then refresh and wait until the udpate message is received. + // then refresh and wait until the update message is received. templateData.assign({ - body: 'New content to change Content-Length!', + body: 'Second test, with and updated body.', }); await webdriver.get(webdriver.getCurrentUrl()); @@ -124,13 +122,17 @@ describe(`[workbox-broadcast-update] Plugin`, function() { }); it(`should broadcast a message to all open window clients`, async function() { - await webdriver.get(testingURL); - await activateAndControlSW(swURL); - await clearAllCaches(); + // This test doesn't work in Safari: + // https://github.com/GoogleChrome/workbox/issues/2755 + if (seleniumBrowser.getId() === 'safari') { + this.skip(); + } + + const tabManager = new TabManager(webdriver); templateData.assign({ title: 'Broadcast Cache Update Test', - body: '', + body: 'Third test, initial body.', script: ` window.__messages = []; navigator.serviceWorker.addEventListener('message', (event) => { @@ -139,71 +141,44 @@ describe(`[workbox-broadcast-update] Plugin`, function() { `, }); - const dynamicPageURL = testingURL + 'integration.html.njk'; + const dynamicPageURL = testingURL + 'integration.html.njk?third'; // Navigate to a dynamic page whose content can be updated from with this // test, and wait until the cache is populated. await webdriver.get(dynamicPageURL); - const tab1Handle = await getLastWindowHandle(); await webdriver.wait(async () => { return webdriver.executeAsyncScript(async (url, cb) => { cb(await caches.match(url)); }, dynamicPageURL); }); - // Update the template data with new content, - // then open a new tab and wait until the udpate message is received. + // Update the template data, then open a new tab to trigger the update. templateData.assign({ - body: 'New content to change Content-Length!', + body: 'Third test, with an updated body.', }); - await openNewTab(dynamicPageURL); + + await tabManager.openTab(dynamicPageURL); + + // Go back to the initial tab to assert the message was received there. + await tabManager.closeOpenedTabs(); + await webdriver.wait(() => { return webdriver.executeScript(() => { return window.__messages.length > 0; }); }); - const tab2Messsages = await webdriver.executeScript(() => { + const tab1Messages = await webdriver.executeScript(() => { return window.__messages; }); - expect(tab2Messsages.length).to.equal(1); - expect(tab2Messsages[0]).to.deep.equal({ + expect(tab1Messages).to.eql([{ type: 'CACHE_UPDATED', meta: 'workbox-broadcast-update', payload: { cacheName: 'bcu-integration-test', updatedURL: dynamicPageURL, }, - }); - - // Also assert a message was received on the first tab. - await webdriver.switchTo().window(tab1Handle); - const tab1Messsages = await webdriver.executeScript(() => { - return window.__messages; - }); - - expect(tab1Messsages.length).to.equal(1); - expect(tab1Messsages[0]).to.deep.equal({ - type: 'CACHE_UPDATED', - meta: 'workbox-broadcast-update', - payload: { - cacheName: 'bcu-integration-test', - updatedURL: dynamicPageURL, - }, - }); + }]); }); }); - -/** - * Clears all caches for the origin of the currently open page. - */ -async function clearAllCaches() { - await webdriver.executeAsyncScript(async (cb) => { - const cacheNames = await caches.keys(); - for (const name of cacheNames) { - await caches.delete(name); - } - cb(); - }); -} diff --git a/test/workbox-window/integration/test-all.js b/test/workbox-window/integration/test-all.js index c2eda1989..cdfd0ce3d 100644 --- a/test/workbox-window/integration/test-all.js +++ b/test/workbox-window/integration/test-all.js @@ -7,13 +7,13 @@ */ const {expect} = require('chai'); -const templateData = require('../../../infra/testing/server/template-data'); + const {executeAsyncAndCatch} = require('../../../infra/testing/webdriver/executeAsyncAndCatch'); -const {getLastWindowHandle} = require('../../../infra/testing/webdriver/getLastWindowHandle'); -const {openNewTab} = require('../../../infra/testing/webdriver/openNewTab'); const {runUnitTests} = require('../../../infra/testing/webdriver/runUnitTests'); +const {TabManager} = require('../../../infra/testing/webdriver/TabManager'); const {unregisterAllSWs} = require('../../../infra/testing/webdriver/unregisterAllSWs'); const {windowLoaded} = require('../../../infra/testing/webdriver/windowLoaded'); +const templateData = require('../../../infra/testing/server/template-data'); // Store local references of these globals. const {webdriver, server, seleniumBrowser} = global.__workbox; @@ -65,7 +65,6 @@ describe(`[workbox-window] Workbox`, function() { const result = await executeAsyncAndCatch(async (cb) => { try { const wb = new Workbox('sw-clients-claim.js.njk'); - await wb.register(); const installedSpy = sinon.spy(); const waitingSpy = sinon.spy(); @@ -74,17 +73,18 @@ describe(`[workbox-window] Workbox`, function() { wb.addEventListener('installed', installedSpy); wb.addEventListener('waiting', waitingSpy); - wb.addEventListener('controlling', controllingSpy); wb.addEventListener('activated', activatedSpy); + wb.addEventListener('controlling', controllingSpy); + + await wb.register(); - wb.addEventListener('activated', () => { - cb({ - isUpdate: installedSpy.args[0][0].isUpdate, - installedSpyCallCount: installedSpy.callCount, - waitingSpyCallCount: waitingSpy.callCount, - controllingSpyCallCount: controllingSpy.callCount, - activatedSpyCallCount: activatedSpy.callCount, - }); + await window.activatedAndControlling(wb); + cb({ + isUpdate: installedSpy.args[0][0].isUpdate, + installedSpyCallCount: installedSpy.callCount, + waitingSpyCallCount: waitingSpy.callCount, + controllingSpyCallCount: controllingSpy.callCount, + activatedSpyCallCount: activatedSpy.callCount, }); } catch (error) { cb({error: error.stack}); @@ -110,10 +110,9 @@ describe(`[workbox-window] Workbox`, function() { wb1.addEventListener('redundant', redundantSpy); await wb1.register(); - await wb1.controlling; + await window.activatedAndControlling(wb1); const wb2 = new Workbox('sw-clients-claim.js.njk?v=2'); - await wb2.register(); const installedSpy = sinon.spy(); const waitingSpy = sinon.spy(); @@ -122,18 +121,20 @@ describe(`[workbox-window] Workbox`, function() { wb2.addEventListener('installed', installedSpy); wb2.addEventListener('waiting', waitingSpy); - wb2.addEventListener('controlling', controllingSpy); wb2.addEventListener('activated', activatedSpy); + wb2.addEventListener('controlling', controllingSpy); - wb2.addEventListener('activated', () => { - cb({ - wb1IsUpdate: redundantSpy.args[0][0].isUpdate, - wb2IsUpdate: installedSpy.args[0][0].isUpdate, - installedSpyCallCount: installedSpy.callCount, - waitingSpyCallCount: waitingSpy.callCount, - controllingSpyCallCount: controllingSpy.callCount, - activatedSpyCallCount: activatedSpy.callCount, - }); + await wb2.register(); + + // Once the newly updated SW is in control, report back. + await window.activatedAndControlling(wb2); + cb({ + wb1IsUpdate: redundantSpy.args[0][0].isUpdate, + wb2IsUpdate: installedSpy.args[0][0].isUpdate, + installedSpyCallCount: installedSpy.callCount, + waitingSpyCallCount: waitingSpy.callCount, + controllingSpyCallCount: controllingSpy.callCount, + activatedSpyCallCount: activatedSpy.callCount, }); } catch (error) { cb({error: error.stack}); @@ -151,14 +152,13 @@ describe(`[workbox-window] Workbox`, function() { }); it(`reports all events for an external SW registration`, async function() { - // Skip this test in Safari due to this flakiness issue: - // https://github.com/GoogleChrome/workbox/issues/2150 + // This test doesn't work in Safari: + // https://github.com/GoogleChrome/workbox/issues/2755 if (seleniumBrowser.getId() === 'safari') { this.skip(); } - const firstTab = await getLastWindowHandle(); - await webdriver.switchTo().window(firstTab); + const tabManager = new TabManager(webdriver); await executeAsyncAndCatch(async (cb) => { try { @@ -175,13 +175,14 @@ describe(`[workbox-window] Workbox`, function() { wb.addEventListener('installed', self.__spies.installedSpy); wb.addEventListener('waiting', self.__spies.waitingSpy); - wb.addEventListener('controlling', self.__spies.controllingSpy); wb.addEventListener('activated', self.__spies.activatedSpy); - - // Resolve this execution block once the SW is activated. - wb.addEventListener('activated', () => cb()); + wb.addEventListener('controlling', self.__spies.controllingSpy); await wb.register(); + + // Resolve this execution block once the SW is in control. + await window.activatedAndControlling(wb); + cb(); } catch (error) { cb({error: error.stack}); } @@ -190,38 +191,39 @@ describe(`[workbox-window] Workbox`, function() { // Update the version in sw.js to trigger a new installation. templateData.assign({version: '2'}); - await openNewTab(testPath); + const secondTabPath = `${testPath}?second`; + await tabManager.openTab(secondTabPath); await windowLoaded(); - await executeAsyncAndCatch(async (cb) => { + const location = await executeAsyncAndCatch(async (cb) => { try { const wb = new Workbox('sw-clients-claim.js.njk'); - // Resolve this execution block once the SW has activated. - wb.addEventListener('activated', () => cb()); - await wb.register(); + + // Resolve this execution block once the updated SW is in control. + await window.activatedAndControlling(wb); + cb(location.href); } catch (error) { cb({error: error.stack}); } }); + // Just confirm we're operating on the page we expect. + expect(location).to.eql(secondTabPath); + // Close the second tab and switch back to the first tab before // executing the following block. - await webdriver.close(); - await webdriver.switchTo().window(firstTab); + await tabManager.closeOpenedTabs(); const result = await executeAsyncAndCatch(async (cb) => { - try { - cb({ - installedSpyArgs: JSON.stringify(self.__spies.installedSpy.args), - waitingSpyArgs: JSON.stringify(self.__spies.waitingSpy.args), - activatedSpyArgs: JSON.stringify(self.__spies.activatedSpy.args), - controllingSpyArgs: JSON.stringify(self.__spies.controllingSpy.args), - }); - } catch (error) { - cb({error: error.stack}); - } + cb({ + location: location.href, + installedSpyArgs: JSON.stringify(self.__spies.installedSpy.args), + waitingSpyArgs: JSON.stringify(self.__spies.waitingSpy.args), + activatedSpyArgs: JSON.stringify(self.__spies.activatedSpy.args), + controllingSpyArgs: JSON.stringify(self.__spies.controllingSpy.args), + }); }); const installedSpyArgs = JSON.parse(result.installedSpyArgs); @@ -229,10 +231,13 @@ describe(`[workbox-window] Workbox`, function() { const activatedSpyArgs = JSON.parse(result.activatedSpyArgs); const controllingSpyArgs = JSON.parse(result.controllingSpyArgs); - expect(installedSpyArgs.length).to.eql(2); - expect(waitingSpyArgs.length).to.eql(0); - expect(activatedSpyArgs.length).to.eql(2); - expect(controllingSpyArgs.length).to.eql(1); + // Just confirm we're operating on the page we expect. + expect(result.location).to.eql(testPath); + + expect(installedSpyArgs.length, 'installedSpy').to.eql(2); + expect(waitingSpyArgs.length, 'waitingSpy').to.eql(0); + expect(activatedSpyArgs.length, 'activatedSpy').to.eql(2); + expect(controllingSpyArgs.length, 'controllingSpy').to.eql(1); expect(installedSpyArgs[0][0].isExternal).to.eql(false); expect(activatedSpyArgs[0][0].isExternal).to.eql(false); diff --git a/test/workbox-window/static/index.html b/test/workbox-window/static/index.html index 37ab3bb5d..8a1ef4a8f 100644 --- a/test/workbox-window/static/index.html +++ b/test/workbox-window/static/index.html @@ -26,6 +26,18 @@ // Expose on the global object so it can be referenced by webdriver. window.Workbox = Workbox; + + // Returns a promise that resolves when both the activated and controlling + // events have fired on the Workbox object. + window.activatedAndControlling = (wb) => { + const activatedPromise = new Promise((resolve) => { + wb.addEventListener('activated', () => resolve()); + }); + const controllingPromise = new Promise((resolve) => { + wb.addEventListener('controlling', () => resolve()); + }); + return Promise.all([activatedPromise, controllingPromise]); + }