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 Mar 25, 2020
1 parent ffad7fa commit 4e1c6b9
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 78 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 navigte 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
27 changes: 14 additions & 13 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,20 +416,20 @@
"ua-parser-js": "0.7.21"
},
"devDependencies": {
"@commitlint/cli": "^8.3.4",
"@commitlint/config-conventional": "^8.3.4",
"browserify": "^16.2.3",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"chai-subset": "^1.2.2",
"@commitlint/cli": "^8.3.4",
"@commitlint/config-conventional": "^8.3.4",
"cucumber": "^3.1.0",
"eslint": "^5.16.0",
"eslint-config-standard": "^12.0.0",
"eslint-plugin-import": "^2.17.2",
"eslint-plugin-node": "^9.0.1",
"eslint-plugin-promise": "^4.1.1",
"eslint-plugin-standard": "^4.0.0",
"grunt": "^1.0.4",
"grunt": "^1.1.0",
"grunt-auto-release": "^0.0.7",
"grunt-browserify": "^5.0.0",
"grunt-bump": "^0.8.0",
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 4e1c6b9

Please sign in to comment.