From bca56032e0b0252d5e6745fad306ba0b9a26164d Mon Sep 17 00:00:00 2001 From: Steve Gravrock Date: Sat, 4 Oct 2025 12:11:14 -0700 Subject: [PATCH] Expose browser errors uniformly outside of GlobalErrors --- lib/jasmine-core/jasmine.js | 89 +++++++++--------- spec/core/GlobalErrorsSpec.js | 91 ++++++++++--------- spec/core/QueueRunnerSpec.js | 59 ------------ .../integration/GlobalErrorHandlingSpec.js | 2 +- src/core/Env.js | 10 +- src/core/GlobalErrors.js | 60 ++++++------ src/core/QueueRunner.js | 19 +--- 7 files changed, 135 insertions(+), 195 deletions(-) diff --git a/lib/jasmine-core/jasmine.js b/lib/jasmine-core/jasmine.js index f1363687..76d40626 100644 --- a/lib/jasmine-core/jasmine.js +++ b/lib/jasmine-core/jasmine.js @@ -1251,14 +1251,14 @@ getJasmineRequireObj().Env = function(j$) { if (!envOptions.suppressLoadErrors) { installGlobalErrors(); - globalErrors.pushListener(function loadtimeErrorHandler(error, event) { + globalErrors.pushListener(function loadtimeErrorHandler(error) { topSuite.result.failedExpectations.push({ passed: false, globalErrorType: 'load', - message: error ? error.message : event.message, - stack: error && error.stack, - filename: event && event.filename, - lineno: event && event.lineno + message: error.message, + stack: error.stack, + filename: error.filename, + lineno: error.lineno }); }); } @@ -4433,13 +4433,6 @@ getJasmineRequireObj().GlobalErrors = function(j$) { this.#adapter.uninstall(); } - // The listener at the top of the stack will be called with two arguments: - // the error and the event. Either of them may be falsy. - // The error will normally be provided, but will be falsy in the case of - // some browser load-time errors. The event will normally be provided in - // browsers but will be falsy in Node. - // Listeners that are pushed after spec files have been loaded should be - // able to just use the error parameter. pushListener(listener) { this.#handlers.push(listener); } @@ -4471,29 +4464,23 @@ getJasmineRequireObj().GlobalErrors = function(j$) { } reportUnhandledRejections() { - for (const { - reason, - event - } of this.#pendingUnhandledRejections.values()) { - this.#dispatchError(reason, event); + for (const { reason } of this.#pendingUnhandledRejections.values()) { + this.#dispatchError(reason); } this.#pendingUnhandledRejections.clear(); } - // Either error or event may be undefined - #onUncaughtException(error, event) { - this.#dispatchError(error, event); + #onUncaughtException(error) { + this.#dispatchError(error); } - // event or promise may be undefined - // event is passed through for backwards compatibility reasons. It's probably - // unnecessary, but user code could depend on it. - #onUnhandledRejection(reason, promise, event) { + // promise may be undefined + #onUnhandledRejection(reason, promise) { if (this.#detectLateRejectionHandling() && promise) { - this.#pendingUnhandledRejections.set(promise, { reason, event }); + this.#pendingUnhandledRejections.set(promise, { reason }); } else { - this.#dispatchError(reason, event); + this.#dispatchError(reason); } } @@ -4505,8 +4492,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { this.#pendingUnhandledRejections.delete(promise); } - // Either error or event may be undefined - #dispatchError(error, event) { + #dispatchError(error) { if (this.#overrideHandler) { // See discussion of spyOnGlobalErrorsAsync in base.js this.#overrideHandler(error); @@ -4516,7 +4502,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { const handler = this.#handlers[this.#handlers.length - 1]; if (handler) { - handler(error, event); + handler(error); } else { throw error; } @@ -4533,7 +4519,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { constructor(global, dispatch) { this.#global = global; this.#dispatch = dispatch; - this.#onError = event => dispatch.onUncaughtException(event.error, event); + this.#onError = this.#errorHandler.bind(this); this.#onUnhandledRejection = this.#unhandledRejectionHandler.bind(this); this.#onRejectionHandled = this.#rejectionHandledHandler.bind(this); } @@ -4562,6 +4548,28 @@ getJasmineRequireObj().GlobalErrors = function(j$) { ); } + #errorHandler(event) { + let error = event.error; + + // event.error isn't guaranteed to be present in all browser load-time + // error events. + if (!error) { + error = { + message: event.message, + stack: `@${event.filename}:${event.lineno}` + }; + } + + if (event.filename) { + // filename and lineno can be more convenient than stack when reporting + // things like syntax errors. Pass them along. + error.filename = event.filename; + error.lineno = event.lineno; + } + + this.#dispatch.onUncaughtException(error); + } + #unhandledRejectionHandler(event) { const jasmineMessage = 'Unhandled promise rejection: ' + event.reason; let reason; @@ -4573,7 +4581,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { reason = jasmineMessage; } - this.#dispatch.onUnhandledRejection(reason, event.promise, event); + this.#dispatch.onUnhandledRejection(reason, event.promise); } #rejectionHandledHandler(event) { @@ -8387,8 +8395,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { } QueueRunner.prototype.execute = function() { - this.handleFinalError = (error, event) => { - this.onException(errorOrMsgForGlobalError(error, event)); + this.handleFinalError = error => { + this.onException(error); }; this.globalErrors.pushListener(this.handleFinalError); this.run(0); @@ -8418,8 +8426,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { this.recordError_(iterativeIndex); }; - function handleError(error, event) { - onException(errorOrMsgForGlobalError(error, event)); + function handleError(error) { + onException(error); } const cleanup = once(() => { if (timeoutId !== void 0) { @@ -8606,17 +8614,6 @@ getJasmineRequireObj().QueueRunner = function(j$) { }; } - function errorOrMsgForGlobalError(error, event) { - // TODO: In cases where error is a string or undefined, the error message - // that gets sent to reporters will be `${message} thrown`, which could - // be improved to not say "thrown" when the cause wasn't necessarily - // an exception or to provide hints about throwing Errors rather than - // strings. - return ( - error || (event && event.message) || 'Global error event with no message' - ); - } - return QueueRunner; }; diff --git a/spec/core/GlobalErrorsSpec.js b/spec/core/GlobalErrorsSpec.js index d53695ad..f795f25b 100644 --- a/spec/core/GlobalErrorsSpec.js +++ b/spec/core/GlobalErrorsSpec.js @@ -13,10 +13,7 @@ describe('GlobalErrors', function() { const error = new Error('nope'); dispatchEvent(globals.listeners, 'error', { error }); - expect(handler).toHaveBeenCalledWith( - jasmine.is(error), - jasmine.objectContaining({ error: jasmine.is(error) }) - ); + expect(handler).toHaveBeenCalledWith(jasmine.is(error)); }); it('is not affected by overriding global.onerror', function() { @@ -35,10 +32,7 @@ describe('GlobalErrors', function() { const error = new Error('nope'); dispatchEvent(globals.listeners, 'error', { error }); - expect(handler).toHaveBeenCalledWith( - jasmine.is(error), - jasmine.objectContaining({ error: jasmine.is(error) }) - ); + expect(handler).toHaveBeenCalledWith(jasmine.is(error)); }); it('only calls the most recent handler', function() { @@ -58,10 +52,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'error', { error }); expect(handler1).not.toHaveBeenCalled(); - expect(handler2).toHaveBeenCalledWith( - jasmine.is(error), - jasmine.objectContaining({ error: jasmine.is(error) }) - ); + expect(handler2).toHaveBeenCalledWith(jasmine.is(error)); }); it('calls previous handlers when one is removed', function() { @@ -82,10 +73,7 @@ describe('GlobalErrors', function() { const error = new Error('nope'); dispatchEvent(globals.listeners, 'error', { error }); - expect(handler1).toHaveBeenCalledWith( - jasmine.is(error), - jasmine.objectContaining({ error: jasmine.is(error) }) - ); + expect(handler1).toHaveBeenCalledWith(jasmine.is(error)); expect(handler2).not.toHaveBeenCalled(); }); @@ -130,6 +118,32 @@ describe('GlobalErrors', function() { errors.uninstall(); }); + it("reports browser error events that don't have errors", function() { + const globals = browserGlobals(); + const handler = jasmine.createSpy('errorHandler'); + const errors = new privateUnderTest.GlobalErrors( + globals.global, + () => ({}) + ); + errors.install(); + errors.pushListener(handler); + + const event = { + message: 'Uncaught SyntaxError: Unexpected end of input', + error: undefined, + filename: 'borkenSpec.js', + lineno: 42 + }; + dispatchEvent(globals.listeners, 'error', event); + + expect(handler).toHaveBeenCalledWith({ + message: 'Uncaught SyntaxError: Unexpected end of input', + filename: 'borkenSpec.js', + lineno: 42, + stack: '@borkenSpec.js:42' + }); + }); + it('reports uncaught exceptions in node.js', function() { const globals = nodeGlobals(); const errors = new privateUnderTest.GlobalErrors( @@ -152,7 +166,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'uncaughtException', new Error('bar')); - expect(handler).toHaveBeenCalledWith(new Error('bar'), undefined); + expect(handler).toHaveBeenCalledWith(new Error('bar')); expect(handler.calls.argsFor(0)[0].jasmineMessage).toBe( 'Uncaught exception: Error: bar' ); @@ -185,7 +199,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'unhandledRejection', new Error('bar')); - expect(handler).toHaveBeenCalledWith(new Error('bar'), undefined); + expect(handler).toHaveBeenCalledWith(new Error('bar')); expect(handler.calls.argsFor(0)[0].jasmineMessage).toBe( 'Unhandled promise rejection: Error: bar' ); @@ -213,8 +227,7 @@ describe('GlobalErrors', function() { 'Unhandled promise rejection: 17\n' + '(Tip: to get a useful stack trace, use ' + 'Promise.reject(new Error(...)) instead of Promise.reject(...).)' - ), - undefined + ) ); }); @@ -236,8 +249,7 @@ describe('GlobalErrors', function() { 'Unhandled promise rejection with no error or message\n' + '(Tip: to get a useful stack trace, use ' + 'Promise.reject(new Error(...)) instead of Promise.reject().)' - ), - undefined + ) ); }); @@ -281,7 +293,7 @@ describe('GlobalErrors', function() { undefined ); - expect(handler).toHaveBeenCalledWith(new Error('nope'), undefined); + expect(handler).toHaveBeenCalledWith(new Error('nope')); expect(handler.calls.argsFor(0)[0].jasmineMessage).toBe( 'Unhandled promise rejection: Error: nope' ); @@ -324,7 +336,7 @@ describe('GlobalErrors', function() { ); errors.reportUnhandledRejections(); - expect(handler).toHaveBeenCalledWith(new Error('nope'), undefined); + expect(handler).toHaveBeenCalledWith(new Error('nope')); expect(handler.calls.argsFor(0)[0].jasmineMessage).toBe( 'Unhandled promise rejection: Error: nope' ); @@ -407,10 +419,7 @@ describe('GlobalErrors', function() { const event = { reason: 'nope' }; dispatchEvent(globals.listeners, 'unhandledrejection', event); - expect(handler).toHaveBeenCalledWith( - 'Unhandled promise rejection: nope', - event - ); + expect(handler).toHaveBeenCalledWith('Unhandled promise rejection: nope'); }); it('reports rejections whose reason is an Error', function() { @@ -428,13 +437,15 @@ describe('GlobalErrors', function() { const event = { reason }; dispatchEvent(globals.listeners, 'unhandledrejection', event); - expect(handler).toHaveBeenCalledWith( + expect(handler).toHaveBeenCalledTimes(1); + const received = handler.calls.argsFor(0)[0]; + expect(received).toBeInstanceOf(Error); + expect(received).toEqual( jasmine.objectContaining({ jasmineMessage: 'Unhandled promise rejection: Error: bar', message: reason.message, stack: reason.stack - }), - event + }) ); }); @@ -469,8 +480,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'unhandledrejection', event); expect(handler).toHaveBeenCalledWith( - 'Unhandled promise rejection: nope', - event + 'Unhandled promise rejection: nope' ); }); }); @@ -507,8 +517,7 @@ describe('GlobalErrors', function() { errors.reportUnhandledRejections(); expect(handler).toHaveBeenCalledWith( - 'Unhandled promise rejection: nope', - { reason: 'nope', promise } + 'Unhandled promise rejection: nope' ); }); @@ -567,10 +576,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'uncaughtException', 17); - expect(handler).toHaveBeenCalledWith( - new Error('Uncaught exception: 17'), - undefined - ); + expect(handler).toHaveBeenCalledWith(new Error('Uncaught exception: 17')); }); it('substitutes a descriptive message when the error is falsy', function() { @@ -587,8 +593,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'uncaughtException', undefined); expect(handler).toHaveBeenCalledWith( - new Error('Uncaught exception with no error or message'), - undefined + new Error('Uncaught exception with no error or message') ); }); }); @@ -619,7 +624,7 @@ describe('GlobalErrors', function() { const event = { error: 'baz' }; dispatchEvent(globals.listeners, 'error', event); expect(overrideHandler).not.toHaveBeenCalledWith('baz'); - expect(handler1).toHaveBeenCalledWith('baz', event); + expect(handler1).toHaveBeenCalledWith('baz'); }); it('overrides the existing handlers in Node until removed', function() { @@ -648,7 +653,7 @@ describe('GlobalErrors', function() { dispatchEvent(globals.listeners, 'uncaughtException', new Error('bar')); expect(overrideHandler).not.toHaveBeenCalled(); - expect(handler1).toHaveBeenCalledWith(new Error('bar'), undefined); + expect(handler1).toHaveBeenCalledWith(new Error('bar')); }); it('handles unhandled promise rejections in browsers', function() { diff --git a/spec/core/QueueRunnerSpec.js b/spec/core/QueueRunnerSpec.js index 9d5a3673..26004d33 100644 --- a/spec/core/QueueRunnerSpec.js +++ b/spec/core/QueueRunnerSpec.js @@ -471,31 +471,6 @@ describe('QueueRunner', function() { expect(nextQueueableFn.fn).toHaveBeenCalled(); }); - it('handles a global error event with a message but no error', function() { - const queueableFn = { - fn: function(done) { - const currentHandler = globalErrors.pushListener.calls.mostRecent() - .args[0]; - currentHandler(undefined, { message: 'nope' }); - }, - timeout: 1 - }; - const onException = jasmine.createSpy('onException'); - const globalErrors = { - pushListener: jasmine.createSpy('pushListener'), - popListener: jasmine.createSpy('popListener') - }; - const queueRunner = new privateUnderTest.QueueRunner({ - queueableFns: [queueableFn], - onException: onException, - globalErrors: globalErrors - }); - - queueRunner.execute(); - - expect(onException).toHaveBeenCalledWith('nope'); - }); - it('handles exceptions thrown while waiting for the stack to clear', function() { const queueableFn = { fn: function(done) { @@ -529,40 +504,6 @@ describe('QueueRunner', function() { clearStack.calls.argsFor(0)[0](); expect(onException).toHaveBeenCalledWith(error); }); - - it('handles a global error event with no error while waiting for the stack to clear', function() { - const queueableFn = { - fn: function(done) { - done(); - } - }; - const errorListeners = []; - const globalErrors = { - pushListener: function(f) { - errorListeners.push(f); - }, - popListener: function() { - errorListeners.pop(); - } - }; - const clearStack = jasmine.createSpy('clearStack'); - const onException = jasmine.createSpy('onException'); - const queueRunner = new privateUnderTest.QueueRunner({ - queueableFns: [queueableFn], - globalErrors: globalErrors, - clearStack: clearStack, - onException: onException - }); - - queueRunner.execute(); - jasmine.clock().tick(); - expect(clearStack).toHaveBeenCalled(); - expect(errorListeners.length).toEqual(1); - errorListeners[0](undefined, { message: 'nope' }); - - clearStack.calls.argsFor(0)[0](); - expect(onException).toHaveBeenCalledWith('nope'); - }); }); describe('with a function that returns a promise', function() { diff --git a/spec/core/integration/GlobalErrorHandlingSpec.js b/spec/core/integration/GlobalErrorHandlingSpec.js index ef60ea5e..caf59b38 100644 --- a/spec/core/integration/GlobalErrorHandlingSpec.js +++ b/spec/core/integration/GlobalErrorHandlingSpec.js @@ -53,7 +53,7 @@ describe('Global error handling (integration)', function() { passed: false, globalErrorType: 'load', message: 'Uncaught SyntaxError: Unexpected end of input', - stack: undefined, + stack: '@borkenSpec.js:42', filename: 'borkenSpec.js', lineno: 42 }, diff --git a/src/core/Env.js b/src/core/Env.js index cf33696a..c542b2df 100644 --- a/src/core/Env.js +++ b/src/core/Env.js @@ -67,14 +67,14 @@ getJasmineRequireObj().Env = function(j$) { if (!envOptions.suppressLoadErrors) { installGlobalErrors(); - globalErrors.pushListener(function loadtimeErrorHandler(error, event) { + globalErrors.pushListener(function loadtimeErrorHandler(error) { topSuite.result.failedExpectations.push({ passed: false, globalErrorType: 'load', - message: error ? error.message : event.message, - stack: error && error.stack, - filename: event && event.filename, - lineno: event && event.lineno + message: error.message, + stack: error.stack, + filename: error.filename, + lineno: error.lineno }); }); } diff --git a/src/core/GlobalErrors.js b/src/core/GlobalErrors.js index ec167ae5..c489f72e 100644 --- a/src/core/GlobalErrors.js +++ b/src/core/GlobalErrors.js @@ -40,13 +40,6 @@ getJasmineRequireObj().GlobalErrors = function(j$) { this.#adapter.uninstall(); } - // The listener at the top of the stack will be called with two arguments: - // the error and the event. Either of them may be falsy. - // The error will normally be provided, but will be falsy in the case of - // some browser load-time errors. The event will normally be provided in - // browsers but will be falsy in Node. - // Listeners that are pushed after spec files have been loaded should be - // able to just use the error parameter. pushListener(listener) { this.#handlers.push(listener); } @@ -78,29 +71,23 @@ getJasmineRequireObj().GlobalErrors = function(j$) { } reportUnhandledRejections() { - for (const { - reason, - event - } of this.#pendingUnhandledRejections.values()) { - this.#dispatchError(reason, event); + for (const { reason } of this.#pendingUnhandledRejections.values()) { + this.#dispatchError(reason); } this.#pendingUnhandledRejections.clear(); } - // Either error or event may be undefined - #onUncaughtException(error, event) { - this.#dispatchError(error, event); + #onUncaughtException(error) { + this.#dispatchError(error); } - // event or promise may be undefined - // event is passed through for backwards compatibility reasons. It's probably - // unnecessary, but user code could depend on it. - #onUnhandledRejection(reason, promise, event) { + // promise may be undefined + #onUnhandledRejection(reason, promise) { if (this.#detectLateRejectionHandling() && promise) { - this.#pendingUnhandledRejections.set(promise, { reason, event }); + this.#pendingUnhandledRejections.set(promise, { reason }); } else { - this.#dispatchError(reason, event); + this.#dispatchError(reason); } } @@ -112,8 +99,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { this.#pendingUnhandledRejections.delete(promise); } - // Either error or event may be undefined - #dispatchError(error, event) { + #dispatchError(error) { if (this.#overrideHandler) { // See discussion of spyOnGlobalErrorsAsync in base.js this.#overrideHandler(error); @@ -123,7 +109,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { const handler = this.#handlers[this.#handlers.length - 1]; if (handler) { - handler(error, event); + handler(error); } else { throw error; } @@ -140,7 +126,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { constructor(global, dispatch) { this.#global = global; this.#dispatch = dispatch; - this.#onError = event => dispatch.onUncaughtException(event.error, event); + this.#onError = this.#errorHandler.bind(this); this.#onUnhandledRejection = this.#unhandledRejectionHandler.bind(this); this.#onRejectionHandled = this.#rejectionHandledHandler.bind(this); } @@ -169,6 +155,28 @@ getJasmineRequireObj().GlobalErrors = function(j$) { ); } + #errorHandler(event) { + let error = event.error; + + // event.error isn't guaranteed to be present in all browser load-time + // error events. + if (!error) { + error = { + message: event.message, + stack: `@${event.filename}:${event.lineno}` + }; + } + + if (event.filename) { + // filename and lineno can be more convenient than stack when reporting + // things like syntax errors. Pass them along. + error.filename = event.filename; + error.lineno = event.lineno; + } + + this.#dispatch.onUncaughtException(error); + } + #unhandledRejectionHandler(event) { const jasmineMessage = 'Unhandled promise rejection: ' + event.reason; let reason; @@ -180,7 +188,7 @@ getJasmineRequireObj().GlobalErrors = function(j$) { reason = jasmineMessage; } - this.#dispatch.onUnhandledRejection(reason, event.promise, event); + this.#dispatch.onUnhandledRejection(reason, event.promise); } #rejectionHandledHandler(event) { diff --git a/src/core/QueueRunner.js b/src/core/QueueRunner.js index 5a339157..5dff3ee7 100644 --- a/src/core/QueueRunner.js +++ b/src/core/QueueRunner.js @@ -77,8 +77,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { } QueueRunner.prototype.execute = function() { - this.handleFinalError = (error, event) => { - this.onException(errorOrMsgForGlobalError(error, event)); + this.handleFinalError = error => { + this.onException(error); }; this.globalErrors.pushListener(this.handleFinalError); this.run(0); @@ -108,8 +108,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { this.recordError_(iterativeIndex); }; - function handleError(error, event) { - onException(errorOrMsgForGlobalError(error, event)); + function handleError(error) { + onException(error); } const cleanup = once(() => { if (timeoutId !== void 0) { @@ -296,16 +296,5 @@ getJasmineRequireObj().QueueRunner = function(j$) { }; } - function errorOrMsgForGlobalError(error, event) { - // TODO: In cases where error is a string or undefined, the error message - // that gets sent to reporters will be `${message} thrown`, which could - // be improved to not say "thrown" when the cause wasn't necessarily - // an exception or to provide hints about throwing Errors rather than - // strings. - return ( - error || (event && event.message) || 'Global error event with no message' - ); - } - return QueueRunner; };