From 4767154f2274894303404252070237ececcc4d57 Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Mon, 17 Feb 2020 17:16:23 +0100 Subject: [PATCH] fix(client): prevent race condition in clear context --- client/karma.js | 66 +++++++++++++++++++++++------------ test/client/karma.spec.js | 73 +++++++++++++++++++++++++++++++-------- 2 files changed, 102 insertions(+), 37 deletions(-) diff --git a/client/karma.js b/client/karma.js index edefcdb14..5d97b0816 100644 --- a/client/karma.js +++ b/client/karma.js @@ -89,8 +89,8 @@ function Karma (socket, iframe, opener, navigator, location, document) { childWindow.close() } childWindow = opener(url) - // run context on parent element (client_with_context) - // using window.__karma__.scriptUrls to get the html element strings and load them dynamically + // 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') { var loadScript = function (idx) { if (idx < window.__karma__.scriptUrls.length) { @@ -120,14 +120,33 @@ function Karma (socket, iframe, opener, navigator, location, document) { } loadScript(0) } - // run in iframe + // run in iframe } else { iframe.src = policy.createURL(url) } } + /** + * Schedules te next execution after current context is cleared + * (or is directly started if context is currently not being cleared) + * @param execution {() => void} + * @see https://github.com/karma-runner/karma/issues/3424 + */ + this.scheduleExecution = function (execution) { + if (reloadingContext) { + // A context reload is in progress. Wait for it to complete before executing. + this.scheduledExecution = execution + } else { + execution() + } + } + this.onbeforeunload = function () { - if (!reloadingContext) { + if (reloadingContext) { + reloadingContext = false + self.scheduledExecution && self.scheduledExecution() + self.scheduledExecution = undefined + } else { // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL) self.error('Some of your tests did a full page reload!') } @@ -146,8 +165,6 @@ function Karma (socket, iframe, opener, navigator, location, document) { this.stringify = stringify function clearContext () { - reloadingContext = true - navigateContextTo('about:blank') } @@ -234,6 +251,8 @@ function Karma (socket, iframe, opener, navigator, location, document) { } if (self.config.clearContext) { + reloadingContext = true + // give the browser some time to breath, there could be a page reload, but because a bunch of // tests could run in the same event loop, we wouldn't notice. setTimeout(function () { @@ -259,24 +278,25 @@ function Karma (socket, iframe, opener, navigator, location, document) { } socket.on('execute', function (cfg) { - // reset startEmitted and reload the iframe - startEmitted = false - self.config = cfg - // if not clearing context, reloadingContext always true to prevent beforeUnload error - reloadingContext = !self.config.clearContext - navigateContextTo(constant.CONTEXT_URL) - - if (self.config.clientDisplayNone) { - [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) { - el.style.display = 'none' - }) - } + self.scheduleExecution(() => { + // reset startEmitted and reload the iframe + startEmitted = false + self.config = cfg - // clear the console before run - // works only on FF (Safari, Chrome do not allow to clear console from js source) - if (window.console && window.console.clear) { - window.console.clear() - } + navigateContextTo(constant.CONTEXT_URL) + + if (self.config.clientDisplayNone) { + [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) { + el.style.display = 'none' + }) + } + + // clear the console before run + // works only on FF (Safari, Chrome do not allow to clear console from js source) + if (window.console && window.console.clear) { + window.console.clear() + } + }) }) socket.on('stop', function () { this.complete() diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js index 3920fe3c0..78026e17c 100644 --- a/test/client/karma.spec.js +++ b/test/client/karma.spec.js @@ -85,7 +85,7 @@ describe('Karma', function () { }) it('should remove reference to start even after syntax error', function () { - function ADAPTER_START_FN () {} + function ADAPTER_START_FN () { } ck.start = ADAPTER_START_FN ck.error('syntax error', '/some/file.js', 11) @@ -129,23 +129,68 @@ describe('Karma', function () { assert(mockWindow.onerror != null) }) - it('should error out if a script attempted to reload the browser after setup', function () { - // Perform setup - var config = ck.config = { + it('should schedule execution if clearContext is busy', function () { + // Arrange + ck.config = k.config = { + useIframe: true, clearContext: true } - socket.emit('execute', config) - var mockWindow = {} + const runConfig = { + useIFrame: true + } + const mockWindow = {} ck.setupContext(mockWindow) + k.complete() // set reloading contexts + + // Act + socket.emit('execute', runConfig) + ck.loaded() + + // Assert + assert(!startSpy.calledWith(runConfig)) + assert(!!k.scheduledExecution) + }) + + describe('onbeforeunload', function () { + it('should error out if a script attempted to reload the browser after setup', function () { + // Perform setup + var config = ck.config = { + clearContext: true + } + + socket.emit('execute', config) + var mockWindow = {} + ck.setupContext(mockWindow) - // Spy on our error handler - sinon.spy(k, 'error') + // Spy on our error handler + sinon.spy(k, 'error') - // Emulate an unload event - mockWindow.onbeforeunload() + // Emulate an unload event + mockWindow.onbeforeunload() + + // Assert our spy was called + assert(k.error.calledWith('Some of your tests did a full page reload!')) + }) - // Assert our spy was called - assert(k.error.calledWith('Some of your tests did a full page reload!')) + it('should execute if an earlier execution is scheduled', function () { + // Arrange + const config = ck.config = k.config = { + useIframe: true, + clearContext: true + } + const mockWindow = {} + ck.setupContext(mockWindow) + k.complete() // set reloading contexts + socket.emit('execute', config) + + // Act + mockWindow.onbeforeunload() + ck.loaded() + + // Assert + assert(startSpy.calledWith(config)) + assert(!k.scheduledExecution) + }) }) it('should report navigator name', function () { @@ -318,7 +363,7 @@ describe('Karma', function () { var mockWindow = { console: { - log: function () {} + log: function () { } } } @@ -334,7 +379,7 @@ describe('Karma', function () { var mockWindow = { console: { - log: function () {} + log: function () { } } }