From 7724bc3a4c85c2a2b4f3c6c1176483c33c7888d5 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 943407b9c3b5..bcebe212e5fd 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -915,50 +915,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 22d7209390f75bff1c523b0d1f035dcf3284cb46 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 bcebe212e5fd..5ebf12a01aa9 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -929,6 +929,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 4e78a7f9dd027de6a7976f2b605aaf794b7e3446 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 5ebf12a01aa9..363996de2e35 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -934,6 +934,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 c63a1e37abff9026f7fd5eda3f3d6144c7796cac 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 363996de2e35..16fc3230d452 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -972,6 +972,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 4b0763fcafc7cce4d70b39efb2cdee0330ca88bb 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 cb50624541c2..269f1cd2b787 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -2960,8 +2960,6 @@ angular.mock.$RootScopeDecorator = ['$delegate', function($delegate) { throw new ErrorAddingDeclarationLocationStack(e, errorForStack); } throw e; - } finally { - errorForStack = null; } } } From 48f222ce568fc133c09c7b6db16fe40cd63f3e59 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 16fc3230d452..e073cfc06707 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -981,6 +981,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 3a09c6daebebf050ce08d81538f6d87f5f482380 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 e073cfc06707..8643933015e7 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -932,19 +932,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; } @@ -1003,6 +1012,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'); + } + }); + }); }); } });