From 11a7bfb39d0713609c20b4a374693df5ebe0948f Mon Sep 17 00:00:00 2001 From: johnjbarton Date: Wed, 25 Mar 2020 10:53:45 -0700 Subject: [PATCH] fix(client): avoid race between execute and clearContext Add a delay in execute to ensure that reload events and clear context events are completed first. Fixes #3424 --- client/karma.js | 58 +++++++++++++------------- static/karma.js | 58 +++++++++++++------------- test/client/karma.spec.js | 86 +++++++++++++++++++++++---------------- 3 files changed, 112 insertions(+), 90 deletions(-) diff --git a/client/karma.js b/client/karma.js index 71b2fa346..2bd4f2cf3 100644 --- a/client/karma.js +++ b/client/karma.js @@ -131,6 +131,13 @@ function Karma (socket, iframe, opener, navigator, location, document) { // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL) self.error('Some of your tests did a full page reload!') } + reloadingContext = false + } + + function clearContext () { + reloadingContext = true + + navigateContextTo('about:blank') } this.log = function (type, args) { @@ -145,12 +152,6 @@ function Karma (socket, iframe, opener, navigator, location, document) { this.stringify = stringify - function clearContext () { - reloadingContext = true - - navigateContextTo('about:blank') - } - function getLocation (url, lineno, colno) { var location = '' @@ -234,11 +235,10 @@ function Karma (socket, iframe, opener, navigator, location, document) { } if (self.config.clearContext) { - // 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 () { - clearContext() - }, 0) + // A test could have incorrectly issued a navigate. To clear the context + // we will navigate the iframe. Delay ours to ensure the error from an + // incorrect navigate is processed. + setTimeout(clearContext) } socket.emit('complete', result || {}, function () { @@ -259,24 +259,26 @@ 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' - }) - } + // Delay our navigation to the next event in case the clearContext has not completed. + setTimeout(function allowClearContextToComplete () { + // reset startEmitted and reload the iframe + startEmitted = false + self.config = cfg + + 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() - } + // 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/static/karma.js b/static/karma.js index 7e49c7c31..bb3630d98 100644 --- a/static/karma.js +++ b/static/karma.js @@ -141,6 +141,13 @@ function Karma (socket, iframe, opener, navigator, location, document) { // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL) self.error('Some of your tests did a full page reload!') } + reloadingContext = false + } + + function clearContext () { + reloadingContext = true + + navigateContextTo('about:blank') } this.log = function (type, args) { @@ -155,12 +162,6 @@ function Karma (socket, iframe, opener, navigator, location, document) { this.stringify = stringify - function clearContext () { - reloadingContext = true - - navigateContextTo('about:blank') - } - function getLocation (url, lineno, colno) { var location = '' @@ -244,11 +245,10 @@ function Karma (socket, iframe, opener, navigator, location, document) { } if (self.config.clearContext) { - // 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 () { - clearContext() - }, 0) + // A test could have incorrectly issued a navigate. To clear the context + // we will navigate the iframe. Delay ours to ensure the error from an + // incorrect navigate is processed. + setTimeout(clearContext) } socket.emit('complete', result || {}, function () { @@ -269,24 +269,26 @@ 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' - }) - } + // Delay our navigation to the next event in case the clearContext has not completed. + setTimeout(function allowClearContextToComplete () { + // reset startEmitted and reload the iframe + startEmitted = false + self.config = cfg + + 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() - } + // 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..bc3ad81f2 100644 --- a/test/client/karma.spec.js +++ b/test/client/karma.spec.js @@ -44,31 +44,40 @@ describe('Karma', function () { assert(startSpy.calledWith(config)) }) - it('should open a new window when useIFrame is false', function () { + it('should open a new window when useIFrame is false', function (done) { var config = ck.config = { useIframe: false, runInParent: false } socket.emit('execute', config) - assert(!ck.start.called) + setTimeout(function nextEventLoop () { + assert(!ck.start.called) - ck.loaded() - assert(startSpy.calledWith(config)) - assert(windowStub.calledWith('context.html')) + ck.loaded() + assert(startSpy.calledWith(config)) + assert(windowStub.calledWith('context.html')) + done() + }) }) - it('should not set style on elements', function () { + it('should not set style on elements', function (done) { var config = {} socket.emit('execute', config) - assert(Object.keys(elements[0].style).length === 0) + setTimeout(function nextEventLoop () { + assert(Object.keys(elements[0].style).length === 0) + done() + }) }) - it('should set display none on elements if clientDisplayNone', function () { + it('should set display none on elements if clientDisplayNone', function (done) { var config = { clientDisplayNone: true } socket.emit('execute', config) - assert(elements[0].style.display === 'none') - assert(elements[1].style.display === 'none') + setTimeout(function nextEventLoop () { + assert(elements[0].style.display === 'none') + assert(elements[1].style.display === 'none') + done() + }) }) it('should stop execution', function () { @@ -97,55 +106,65 @@ describe('Karma', function () { assert.notStrictEqual(k.start, ADAPTER_START_FN) }) - it('should not set up context if there was an error', function () { + it('should not set up context if there was an error', function (done) { var config = ck.config = { clearContext: true } socket.emit('execute', config) - var mockWindow = {} + setTimeout(function nextEventLoop () { + var mockWindow = {} - ck.error('page reload') - ck.setupContext(mockWindow) + ck.error('page reload') + ck.setupContext(mockWindow) - assert(mockWindow.onbeforeunload == null) - assert(mockWindow.onerror == null) + assert(mockWindow.onbeforeunload == null) + assert(mockWindow.onerror == null) + done() + }) }) - it('should setup context if there was error but clearContext config is false', function () { + it('should setup context if there was error but clearContext config is false', function (done) { var config = ck.config = { clearContext: false } socket.emit('execute', config) - var mockWindow = {} + setTimeout(function nextEventLoop () { + var mockWindow = {} - ck.error('page reload') - ck.setupContext(mockWindow) + ck.error('page reload') + ck.setupContext(mockWindow) - assert(mockWindow.onbeforeunload != null) - assert(mockWindow.onerror != null) + assert(mockWindow.onbeforeunload != null) + assert(mockWindow.onerror != null) + done() + }) }) - it('should error out if a script attempted to reload the browser after setup', function () { + it('should error out if a script attempted to reload the browser after setup', function (done) { // 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') + setTimeout(function nextEventLoop () { + var mockWindow = {} + ck.setupContext(mockWindow) + + // 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!')) + done() + }) }) it('should report navigator name', function () { @@ -439,12 +458,10 @@ describe('Karma', function () { } socket.emit('execute', config) + clock.tick(1) var CURRENT_URL = iframe.src - ck.complete() - clock.tick(1) - assert.strictEqual(iframe.src, CURRENT_URL) }) @@ -455,6 +472,7 @@ describe('Karma', function () { } socket.emit('execute', config) + clock.tick(1) assert(!startSpy.called) ck.loaded()