From 9155218464497a3d3f44d00065f94b4949f37f38 Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Sun, 31 Jan 2021 16:16:14 +0100 Subject: [PATCH] fix(client): fix a false positive page reload error in Safari Previous code was susceptible to the race condition in Safari browser. Specifically below piece: ``` iframe.src = policy.createURL(url) karmaNavigating = false ``` There is no guarantee that onbeforeunload event will be triggered synchronously after setting `iframe.src`. It was the case in Chrome and Firefox, but not in Safari, where this caused an error every test run. New approach resets the onbeforeunload handler before navigation is triggered by Karma itself to avoid the need for synchronization and this race condition altogether. The handler will be restored by the new context once it is loaded. PS Code is a bit fragile as there is an implicit dependency on `onbeforeunload` from the context, but I can't think of a cleaner approach. --- client/karma.js | 18 ++++++------------ context/karma.js | 6 +++--- static/context.js | 6 +++--- static/karma.js | 18 ++++++------------ test/client/karma.spec.js | 2 +- 5 files changed, 19 insertions(+), 31 deletions(-) diff --git a/client/karma.js b/client/karma.js index 20a5e19e0..5586bdc9e 100644 --- a/client/karma.js +++ b/client/karma.js @@ -5,7 +5,6 @@ var util = require('../common/util') function Karma (updater, socket, iframe, opener, navigator, location, document) { this.updater = updater var startEmitted = false - var karmaNavigating = false var self = this var queryParams = util.parseQueryParams(location.search) var browserId = queryParams.id || util.generateId('manual-') @@ -83,21 +82,21 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) var childWindow = null function navigateContextTo (url) { - karmaNavigating = true if (self.config.useIframe === false) { // run in new window if (self.config.runInParent === false) { // If there is a window already open, then close it // DEV: In some environments (e.g. Electron), we don't have setter access for location if (childWindow !== null && childWindow.closed !== true) { + // The onbeforeunload listener was added by context to catch + // unexpected navigations while running tests. + childWindow.onbeforeunload = undefined childWindow.close() } childWindow = opener(url) - karmaNavigating = false // run context on parent element (client_with_context) // using window.__karma__.scriptUrls to get the html element strings and load them dynamically } else if (url !== 'about:blank') { - karmaNavigating = false var loadScript = function (idx) { if (idx < window.__karma__.scriptUrls.length) { var parser = new DOMParser() @@ -128,15 +127,10 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) } // run in iframe } else { + // The onbeforeunload listener was added by the context to catch + // unexpected navigations while running tests. + iframe.contentWindow.onbeforeunload = undefined iframe.src = policy.createURL(url) - karmaNavigating = false - } - } - - this.onbeforeunload = function () { - if (!karmaNavigating) { - // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL) - self.error('Some of your tests did a full page reload!') } } diff --git a/context/karma.js b/context/karma.js index 677767c9a..859d56d19 100644 --- a/context/karma.js +++ b/context/karma.js @@ -77,9 +77,9 @@ function ContextKarma (callParentKarmaMethod) { contextWindow.onerror = function () { return self.error.apply(self, arguments) } - // DEV: We must defined a function since we don't want to pass the event object - contextWindow.onbeforeunload = function (e, b) { - callParentKarmaMethod('onbeforeunload', []) + + contextWindow.onbeforeunload = function () { + return self.error('Some of your tests did a full page reload!') } contextWindow.dump = function () { diff --git a/static/context.js b/static/context.js index 8cc67ebf6..417503165 100644 --- a/static/context.js +++ b/static/context.js @@ -214,9 +214,9 @@ function ContextKarma (callParentKarmaMethod) { contextWindow.onerror = function () { return self.error.apply(self, arguments) } - // DEV: We must defined a function since we don't want to pass the event object - contextWindow.onbeforeunload = function (e, b) { - callParentKarmaMethod('onbeforeunload', []) + + contextWindow.onbeforeunload = function () { + return self.error('Some of your tests did a full page reload!') } contextWindow.dump = function () { diff --git a/static/karma.js b/static/karma.js index 1c1111450..23b7c1ae2 100644 --- a/static/karma.js +++ b/static/karma.js @@ -15,7 +15,6 @@ var util = require('../common/util') function Karma (updater, socket, iframe, opener, navigator, location, document) { this.updater = updater var startEmitted = false - var karmaNavigating = false var self = this var queryParams = util.parseQueryParams(location.search) var browserId = queryParams.id || util.generateId('manual-') @@ -93,21 +92,21 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) var childWindow = null function navigateContextTo (url) { - karmaNavigating = true if (self.config.useIframe === false) { // run in new window if (self.config.runInParent === false) { // If there is a window already open, then close it // DEV: In some environments (e.g. Electron), we don't have setter access for location if (childWindow !== null && childWindow.closed !== true) { + // The onbeforeunload listener was added by context to catch + // unexpected navigations while running tests. + childWindow.onbeforeunload = undefined childWindow.close() } childWindow = opener(url) - karmaNavigating = false // run context on parent element (client_with_context) // using window.__karma__.scriptUrls to get the html element strings and load them dynamically } else if (url !== 'about:blank') { - karmaNavigating = false var loadScript = function (idx) { if (idx < window.__karma__.scriptUrls.length) { var parser = new DOMParser() @@ -138,15 +137,10 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) } // run in iframe } else { + // The onbeforeunload listener was added by the context to catch + // unexpected navigations while running tests. + iframe.contentWindow.onbeforeunload = undefined iframe.src = policy.createURL(url) - karmaNavigating = false - } - } - - this.onbeforeunload = function () { - if (!karmaNavigating) { - // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL) - self.error('Some of your tests did a full page reload!') } } diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js index b1c50a00a..cb95ced81 100644 --- a/test/client/karma.spec.js +++ b/test/client/karma.spec.js @@ -22,7 +22,7 @@ describe('Karma', function () { } } socket = new MockSocket() - iframe = {} + iframe = { contentWindow: {} } windowNavigator = {} windowLocation = { search: '' } windowStub = sinon.stub().returns({})