From bb752d1b701b38dd2635af2db5a977fbf77ffe2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 11:45:17 +0100 Subject: [PATCH 1/7] refactor(ngMock window.inject test): create room for planned tests Split up `ngMock` `window.inject` tests into tests on browsers supporting fetching current stack trace information and those not supporting it, as we plan on adding new test specs to the first group. Required making the test inject caller function more flexible so it can be easily configured in different tests without unnecessary code duplication. --- test/ngMock/angular-mocksSpec.js | 75 ++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index ce81adf74deb..f57a0c174f55 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -920,50 +920,67 @@ describe('ngMock', function() { }).toThrow('test message'); })); - describe('error stack trace when called outside of spec context', function() { - // - Chrome, Firefox, Edge, Opera give us the stack trace as soon as an Error is created - // - IE10+, PhantomJS give us the stack trace only once the error is thrown - // - IE9 does not provide stack traces - var stackTraceSupported = (function() { - var error = new Error(); - if (!error.stack) { - try { - throw error; - } catch (e) {} - } + // - Chrome, Firefox, Edge, Opera give us the stack trace as soon as an Error is created + // - IE10+, PhantomJS give us the stack trace only once the error is thrown + // - IE9 does not provide stack traces + var stackTraceSupported = (function() { + var error = new Error(); + if (!error.stack) { + try { + throw error; + } catch (e) {} + } - return !!error.stack; - })(); + return !!error.stack; + })(); - function testCaller() { + function testInjectCaller() { + var shouldThrow; + var injectingCall = (function internalInjectCaller() { return inject(function() { - throw new Error(); + if (shouldThrow) + throw new Error(); }); - } - var throwErrorFromInjectCallback = testCaller(); + })(); + injectingCall.setThrow = function(value) { + shouldThrow = value; + }; + return injectingCall; + } - if (stackTraceSupported) { - describe('on browsers supporting stack traces', function() { - it('should update thrown Error stack trace with inject call location', function() { + if (!stackTraceSupported) { + describe('on browsers not supporting stack traces', function() { + describe('when called outside of test spec context', function() { + var injectingCall = testInjectCaller(); + + it('should not add stack trace information to thrown injection Error', function() { + injectingCall.setThrow(true); try { - throwErrorFromInjectCallback(); + injectingCall(); } catch (e) { - expect(e.stack).toMatch('testCaller'); + expect(e.stack).toBeUndefined(); } }); }); - } else { - describe('on browsers not supporting stack traces', function() { - it('should not add stack trace information to thrown Error', function() { + }); + } + + if (stackTraceSupported) { + describe('on browsers supporting stack traces', function() { + describe('when called outside of test spec context and initial inject callback invocation fails', function() { + var throwingInjectingCall = testInjectCaller(); + throwingInjectingCall.setThrow(true); + + it('should update thrown Error stack trace with inject call location', function() { try { - throwErrorFromInjectCallback(); + throwingInjectingCall(); } catch (e) { - expect(e.stack).toBeUndefined(); + expect(e.stack).toMatch('testInjectCaller'); } }); }); - } - }); + }); + } }); }); From 1dd4d4094d344695daf76707c42c1c0d7e671f44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 21:54:57 +0100 Subject: [PATCH 2/7] refactor(ngMock window.inject test): add testInjectCaller() usage comment The results of this function, when called outside of a specific test spec context, should not be reused in multiple tests as they may have stored state that can cause unwanted test spec interaction. This explains why we may need to wrap some tests into their own separate test suites instead of grouping them all under a single shared one. --- test/ngMock/angular-mocksSpec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index f57a0c174f55..2c0a7b1b9171 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -934,6 +934,9 @@ describe('ngMock', function() { return !!error.stack; })(); + // function returned by inject(), when called outside of test spec + // context, may have stored state so do not reuse the result from this + // call in multiple test specs function testInjectCaller() { var shouldThrow; var injectingCall = (function internalInjectCaller() { From bd3208c5740e6a91ca762f6ac519f21170e48f1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 22:05:24 +0100 Subject: [PATCH 3/7] refactor(ngMock window.inject test): comment on important function wrapper usage Explicitly commented on why we use an extra function wrapper around the test inject Error throwing code, and how not using it would make our tests give us false positives on certain browsers, e.g. Firefox. --- test/ngMock/angular-mocksSpec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 2c0a7b1b9171..ac75c6dcf2c2 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -939,6 +939,9 @@ describe('ngMock', function() { // call in multiple test specs function testInjectCaller() { var shouldThrow; + // using an extra internalInjectCaller() wrapper here avoids stack trace + // constructed by some browsers (e.g. FireFox) from containing the name + // of the external caller function var injectingCall = (function internalInjectCaller() { return inject(function() { if (shouldThrow) From 7deb83c2f97d5bb7e1c8bc690f4e40a364f27971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 22:13:02 +0100 Subject: [PATCH 4/7] refactor(ngMock window.inject test): mark regression test for issue #13591 --- test/ngMock/angular-mocksSpec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index ac75c6dcf2c2..ffd951c3ee4b 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -977,6 +977,7 @@ describe('ngMock', function() { var throwingInjectingCall = testInjectCaller(); throwingInjectingCall.setThrow(true); + // regression test for issue #13591 when run on IE10+ or PhantomJS it('should update thrown Error stack trace with inject call location', function() { try { throwingInjectingCall(); From bd739b0ada104347389e8577e662a0d0ee0f3052 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 21:25:43 +0100 Subject: [PATCH 5/7] fix(ngMock window.inject): correct error stack trace update on all invocations Failed injection functions should consistently include their respective window.inject() call location information in their stack trace, even on repeated invocations & when they have been passed as a non-initial parameter to their window.inject() call. --- src/ngMock/angular-mocks.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 4441df1c69f9..63533e8e16dc 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -2521,8 +2521,6 @@ if (window.jasmine || window.mocha) { throw new ErrorAddingDeclarationLocationStack(e, errorForStack); } throw e; - } finally { - errorForStack = null; } } } From f0dc534a3830575846bf0b9a5e255512faf5dbbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 11:52:20 +0100 Subject: [PATCH 6/7] test(ngMock window.inject): error stack trace on repeated injection calls Injection function throwing an Error should update the thrown Error's stack trace information with the window.inject() call location information, on its initial as well as repeated invocations. --- test/ngMock/angular-mocksSpec.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index ffd951c3ee4b..528d2dde7a26 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -986,6 +986,28 @@ describe('ngMock', function() { } }); }); + + describe('when called outside of test spec context', function() { + var injectingCall = testInjectCaller(); + + // regression test for issue #13594 + // regression test for issue #13591 when run on IE10+ or PhantomJS + it('should update thrown Error stack when repeated inject callback invocations fail', function() { + injectingCall.setThrow(false); + injectingCall(); // initial call that will not throw + injectingCall.setThrow(true); + try { + injectingCall(); // non-initial call, but first failing one + } catch (e) { + expect(e.stack).toMatch('testInjectCaller'); + } + try { + injectingCall(); // repeated failing call + } catch (e) { + expect(e.stack).toMatch('testInjectCaller'); + } + }); + }); }); } }); From e1e32747c0e9f1b3bed01d5a92833986d2a5322e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Sun, 20 Dec 2015 20:24:56 +0100 Subject: [PATCH 7/7] test(ngMock window.inject): error stack trace on inject with multiple functions Injection function throwing an Error should update the thrown Error's stack trace information with the window.inject() call location information even when multiple injection functions are passed to the window.inject() call and non-initial provided function fails. Closes #13594. --- test/ngMock/angular-mocksSpec.js | 49 ++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 528d2dde7a26..475f614e4664 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -937,19 +937,28 @@ describe('ngMock', function() { // function returned by inject(), when called outside of test spec // context, may have stored state so do not reuse the result from this // call in multiple test specs - function testInjectCaller() { - var shouldThrow; - // using an extra internalInjectCaller() wrapper here avoids stack trace - // constructed by some browsers (e.g. FireFox) from containing the name - // of the external caller function - var injectingCall = (function internalInjectCaller() { - return inject(function() { - if (shouldThrow) + function testInjectCaller(injectionFunctionCount) { + var shouldThrow = []; + // using an extra named function wrapper around the Error throw avoids + // stack trace constructed by some browsers (e.g. FireFox) from + // containing the name of the external caller function + function injectionFunction(index) { + return function() { + if (shouldThrow[index]) throw new Error(); - }); - })(); - injectingCall.setThrow = function(value) { - shouldThrow = value; + }; + } + var injectionFunctions = []; + for (var i = 0; i < (injectionFunctionCount || 1); ++i) { + injectionFunctions.push(injectionFunction(i)); + } + var injectingCall = inject.apply(window, injectionFunctions); + injectingCall.setThrow = function(index, value) { + if (!isDefined(value)) { + value = index; + index = 0; + } + shouldThrow[index] = value; }; return injectingCall; } @@ -1008,6 +1017,22 @@ describe('ngMock', function() { } }); }); + + describe('when called outside of test spec context with multiple injected functions', function() { + var injectingCall = testInjectCaller(2); + + // regression test for issue #13594 + // regression test for issue #13591 when run on IE10+ or PhantomJS + it('should update thrown Error stack when second injected function fails', function() { + injectingCall.setThrow(0, false); + injectingCall.setThrow(1, true); + try { + injectingCall(); + } catch (e) { + expect(e.stack).toMatch('testInjectCaller'); + } + }); + }); }); } });