Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client): fix a false positive page reload error in Safari #3643

Merged
merged 1 commit into from
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions client/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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-')
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not delete childWindow.onbeforeunload?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onbeforeunload is a property of the window object, i.e. it has getter and setter. window.onbeforeunload = undefined will change the underlying value, while delete window.onbeforeunload will remove the property descriptor. Consider the below screenshot:

image

It does not really matter in this case, but I think the current code is more correct.

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()
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same q

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!')
}
}

Expand Down
6 changes: 3 additions & 3 deletions context/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
6 changes: 3 additions & 3 deletions static/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
18 changes: 6 additions & 12 deletions static/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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-')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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!')
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/client/karma.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Karma', function () {
}
}
socket = new MockSocket()
iframe = {}
iframe = { contentWindow: {} }
windowNavigator = {}
windowLocation = { search: '' }
windowStub = sinon.stub().returns({})
Expand Down