Skip to content

Commit

Permalink
fix(client): avoid race between execute and clearContext
Browse files Browse the repository at this point in the history 
Add a delay in execute to ensure that reload events and clear context events are completed first.
Fixes #3424
  • Loading branch information
@johnjbarton
johnjbarton committed Aug 10, 2020
1 parent 6cd5a3b commit 11a7bfb
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 90 deletions.
58 changes: 30 additions & 28 deletions client/karma.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 = ''

Expand Down Expand Up @@ -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 () {
Expand All @@ -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()
Expand Down
58 changes: 30 additions & 28 deletions static/karma.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 = ''

Expand Down Expand Up @@ -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 () {
Expand All @@ -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()
Expand Down
86 changes: 52 additions & 34 deletions test/client/karma.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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 () {
Expand Down Expand Up @@ -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)
})

Expand All @@ -455,6 +472,7 @@ describe('Karma', function () {
}

socket.emit('execute', config)
clock.tick(1)
assert(!startSpy.called)

ck.loaded()
Expand Down

0 comments on commit 11a7bfb

Please sign in to comment.