From babe2b68fbaf8f2d6326706c698e3afe601b572b Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 29 May 2019 13:38:14 -0700 Subject: [PATCH] test: move beforeunload tests to main runner and fix flake (#18432) --- spec-main/api-web-contents-spec.ts | 23 ++++++++++++++++- spec-main/window-helpers.ts | 40 +++++++++++++++++------------- spec/api-web-contents-spec.js | 25 ------------------- spec/window-helpers.js | 32 ++++++++++++++++-------- 4 files changed, 67 insertions(+), 53 deletions(-) diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 364f193765d3..bb84ad59880a 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -9,7 +9,7 @@ const { expect } = chai chai.use(chaiAsPromised) -const fixturesPath = path.resolve(__dirname, '../spec/fixtures') +const fixturesPath = path.resolve(__dirname, '..', 'spec', 'fixtures') describe('webContents module', () => { let w: BrowserWindow = (null as unknown as BrowserWindow) @@ -52,4 +52,25 @@ describe('webContents module', () => { expect(all[all.length - 1].getType()).to.equal('remote') }) }) + + describe('will-prevent-unload event', () => { + it('does not emit if beforeunload returns undefined', (done) => { + w.once('closed', () => done()) + w.webContents.once('will-prevent-unload', (e) => { + expect.fail('should not have fired') + }) + w.loadFile(path.join(fixturesPath, 'api', 'close-beforeunload-undefined.html')) + }) + + it('emits if beforeunload returns false', (done) => { + w.webContents.once('will-prevent-unload', () => done()) + w.loadFile(path.join(fixturesPath, 'api', 'close-beforeunload-false.html')) + }) + + it('supports calling preventDefault on will-prevent-unload events', (done) => { + w.webContents.once('will-prevent-unload', event => event.preventDefault()) + w.once('closed', () => done()) + w.loadFile(path.join(fixturesPath, 'api', 'close-beforeunload-false.html')) + }) + }) }) diff --git a/spec-main/window-helpers.ts b/spec-main/window-helpers.ts index c2c7fabdcfd3..bce64bbec9b8 100644 --- a/spec-main/window-helpers.ts +++ b/spec-main/window-helpers.ts @@ -1,32 +1,38 @@ import { expect } from 'chai' -import { BrowserWindow, WebContents } from 'electron' +import { BrowserWindow } from 'electron' import { emittedOnce } from './events-helpers'; +async function ensureWindowIsClosed(window: BrowserWindow | null) { + if (window && !window.isDestroyed()) { + if (window.webContents && !window.webContents.isDestroyed()) { + // If a window isn't destroyed already, and it has non-destroyed WebContents, + // then calling destroy() won't immediately destroy it, as it may have + // children which need to be destroyed first. In that case, we + // await the 'closed' event which signals the complete shutdown of the + // window. + const isClosed = emittedOnce(window, 'closed') + window.destroy() + await isClosed + } else { + // If there's no WebContents or if the WebContents is already destroyed, + // then the 'closed' event has already been emitted so there's nothing to + // wait for. + window.destroy() + } + } +} + export const closeWindow = async ( window: BrowserWindow | null = null, { assertNotWindows } = { assertNotWindows: true } ) => { - if (window && !window.isDestroyed()) { - const isClosed = emittedOnce(window, 'closed') - window.setClosable(true) - window.close() - await isClosed - } + await ensureWindowIsClosed(window) if (assertNotWindows) { const windows = BrowserWindow.getAllWindows() for (const win of windows) { - const closePromise = emittedOnce(win, 'closed') - win.close() - await closePromise + await ensureWindowIsClosed(win) } expect(windows).to.have.lengthOf(0) } } - -exports.waitForWebContentsToLoad = async (webContents: WebContents) => { - const didFinishLoadPromise = emittedOnce(webContents, 'did-finish-load') - if (webContents.isLoadingMainFrame()) { - await didFinishLoadPromise - } -} diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index 5dd3ed1e5522..726deadd71f4 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -779,31 +779,6 @@ describe('webContents module', () => { }) }) - describe('will-prevent-unload event', () => { - it('does not emit if beforeunload returns undefined', (done) => { - w.once('closed', () => { - done() - }) - w.webContents.on('will-prevent-unload', (e) => { - expect.fail('should not have fired') - }) - w.loadFile(path.join(fixtures, 'api', 'close-beforeunload-undefined.html')) - }) - - it('emits if beforeunload returns false', (done) => { - w.webContents.on('will-prevent-unload', () => { - done() - }) - w.loadFile(path.join(fixtures, 'api', 'close-beforeunload-false.html')) - }) - - it('supports calling preventDefault on will-prevent-unload events', (done) => { - ipcRenderer.send('prevent-next-will-prevent-unload', w.webContents.id) - w.once('closed', () => done()) - w.loadFile(path.join(fixtures, 'api', 'close-beforeunload-false.html')) - }) - }) - describe('render view deleted events', () => { let server = null diff --git a/spec/window-helpers.js b/spec/window-helpers.js index ad1ea57f2790..8a91ec898356 100644 --- a/spec/window-helpers.js +++ b/spec/window-helpers.js @@ -4,15 +4,29 @@ const { BrowserWindow } = remote const { emittedOnce } = require('./events-helpers') +async function ensureWindowIsClosed (window) { + if (window && !window.isDestroyed()) { + if (window.webContents && !window.webContents.isDestroyed()) { + // If a window isn't destroyed already, and it has non-destroyed WebContents, + // then calling destroy() won't immediately destroy it, as it may have + // children which need to be destroyed first. In that case, we + // await the 'closed' event which signals the complete shutdown of the + // window. + const isClosed = emittedOnce(window, 'closed') + window.destroy() + await isClosed + } else { + // If there's no WebContents or if the WebContents is already destroyed, + // then the 'closed' event has already been emitted so there's nothing to + // wait for. + window.destroy() + } + } +} + exports.closeWindow = async (window = null, { assertSingleWindow } = { assertSingleWindow: true }) => { - const windowExists = (window !== null) && !window.isDestroyed() - if (windowExists) { - const isClosed = emittedOnce(window, 'closed') - window.setClosable(true) - window.close() - await isClosed - } + await ensureWindowIsClosed(window) if (assertSingleWindow) { // Although we want to assert that no windows were left handing around @@ -22,9 +36,7 @@ exports.closeWindow = async (window = null, const windows = BrowserWindow.getAllWindows() for (const win of windows) { if (win.id !== currentId) { - const closePromise = emittedOnce(win, 'closed') - win.close() - await closePromise + await ensureWindowIsClosed(win) } } expect(windows).to.have.lengthOf(1)