ugly solution for unexpected request fail on verify no outstanding request #16150

marcin-wosinek · 2017-08-06T19:05:11Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#15855

What is the new behavior (if this is a feature change)?
same as before the regression

Does this PR introduce a breaking change?
no

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

Narretz · 2017-08-07T11:00:16Z

Looks like this break the promises-aplus-tests.

gkalpak

Unfortunately, I couldn't come up with anything "less ugly" 😃
So, happy to go with this approach.

Thx for looking into it @marcin-wosinek 👍

gkalpak · 2017-08-10T18:56:06Z

src/ngMock/angular-mocks.js

        new Error('No response defined !') :
        new Error('Unexpected request: ' + method + ' ' + url + '\n' +
                  (expectation ? 'Expected ' + expectation : 'No more request expected'));
+
+    // TODO ugly!
+    error.rethrow = true;


Please rename to $$rethrow or $$passToExceptionHandler.

gkalpak · 2017-08-10T18:59:20Z

src/ngMock/angular-mocks.js

        new Error('No response defined !') :
        new Error('Unexpected request: ' + method + ' ' + url + '\n' +
                  (expectation ? 'Expected ' + expectation : 'No more request expected'));
+
+    // TODO ugly!


Remove the ugly comment 😛
An actual comment would fine actually. Something like:

In addition to be being converted to a rejection, this error also needs to be passed to
the $exceptionHandler and be rethrown (so that the test fails).

gkalpak · 2017-08-10T19:00:39Z

src/ng/q.js

@@ -364,6 +364,10 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
          }
        } catch (e) {
          rejectPromise(promise, e);
+          // TODO ugly!
+          if (e && e.rethrow === true) {
+            exceptionHandler(e);


A comment would be nice here too. E.g. something like:

This error is explicitly marked for being passed to the $exceptionHandler.

marcin-wosinek · 2017-08-15T08:35:56Z

Thanks for the feedback! looks that it fixed the issues on tests too:)

gkalpak · 2017-08-15T12:26:19Z

src/ng/q.js

@@ -364,6 +364,10 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
          }
        } catch (e) {
          rejectPromise(promise, e);
+          // This error is explicitly marked for being passed to the $exceptionHandler
+          if (e && e.$$passToExceptionHandle === true) {


...Handler (with an r in the end).

gkalpak · 2017-08-15T12:26:45Z

src/ngMock/angular-mocks.js

        new Error('No response defined !') :
        new Error('Unexpected request: ' + method + ' ' + url + '\n' +
                  (expectation ? 'Expected ' + expectation : 'No more request expected'));
+
+    // In addition to be being converted to a rejection, this error also need to be passed to


need --> needs

Narretz · 2017-08-15T13:31:04Z

We talked about this yesterday in the meeting and @petebacondarwin had another idea how to implement this, maybe he can outline it here

petebacondarwin · 2017-08-16T15:27:55Z

The general concern was with potential collisions with user code by checking a "custom" property on the error objects. I accept that this is mitigated by the use of $$ on the property. But still, I think it would be safer to use the other approach (suggested here: #15855 (comment)) where we throw a "private" error class instead.

Since ngMock is in a separate closure to the $q service this error class would need to be exposed somewhere. We thought the safest place would be on $q itself (e.g. $q.$$PassToExceptionHandlerError). For now we would keep this double dollared ($$) to indicate it is private but it could be made public if there was interest in it outside of ngMock.

@mgol suggested that an even cleaner approach would be to override the $q service in ngMock with the functionality described above, which would mean that production $q is not changed but that we have the desired effect when testing. We thought this was interesting but we concerned that the cost of changing $q to expose the necessary parts to support the override would be more complicating than just supporting it natively.

@mgol also pointed out that this "special" behaviour would not strictly be in keeping with the Promises A+ spec, even though it would pass the tests...

gkalpak · 2017-08-17T14:52:55Z

@mgol also pointed out that this "special" behaviour would not strictly be in keeping with the Promises A+ spec, even though it would pass the tests...

If it's passing the test... 😛 Why would it not be compiant with Promises/A+?
Our previous discussions about this "special" behavior of $q (i.e. passing stuff to the $exceptionHandler, which sometimes throws) were non-conclusive 😁

_{Happy to go with the special Error class, btw.}

mgol · 2017-08-17T16:49:47Z

If it's passing the test... 😛 Why would it not be compiant with Promises/A+?

Tests can almost never cover 100% of the spec. In this case, the spec doesn't allow to treat some rejections/errors differently.

gkalpak · 2017-08-17T18:49:08Z

In this case, the spec doesn't allow to treat some rejections/errors differently.

OOC, where does it say so?

mgol · 2017-08-17T19:02:03Z

The spec describes what needs to happen when an exception is thrown in a handler. It doesn't specify any requirements for those exceptions, hence all the rules have to apply for every exception type.

gkalpak · 2017-08-18T11:22:22Z

What the spec describes does happen with all exceptions. But for some exceptions, additional stuff may happen.

But I do see your point. (I feel like we've had this conversation before 😛)

mgol · 2017-08-18T11:36:23Z

@gkalpak Additional stuff may happen (we can then only discuss if it lies in the spirit of the spec) but that's not the point here. The spec mandates all exceptions need to be passed to the catch handlers and here the idea is to create a special exception class that avoids it. From the discussion at the last team meeting just re-throwing in the exceptionHandler wouldn't work as then it would be caught again by the catch handlers anyway. Or is there a way to make it work & not break the spec compliance?

gkalpak · 2017-08-18T11:58:55Z

The promise will be rejected as usual, but the error will also be passed to the $exceptionHandler. This is essentially the same that happened for all Error instances until not long ago. It will now happen for specific errors only (that will only be thrown during tests).

petebacondarwin · 2017-09-04T18:11:38Z

Has anyone implemented our agreed solution here?

Narretz · 2017-09-05T09:42:53Z

I don't think so. I think we wanted to wait for @gkalpak to return, as he reviewed this initially

gkalpak · 2017-09-05T12:33:59Z

As per #16150 (comment), I am happy to go with the special Error class. @marcin-wosinek, would you be up to updating the PR with the new implementation (as described in #16150 (comment))?

marcin-wosinek · 2017-09-05T12:50:11Z

yes, I'm about to update the implementation.

marcin-wosinek · 2017-09-06T07:08:08Z

Looks it failed before due to some e2e glitch. It's ready to final review.

gkalpak · 2017-09-06T07:42:06Z

src/ngMock/angular-mocks.js

                  (expectation ? 'Expected ' + expectation : 'No more request expected'));
+
+    // In addition to be being converted to a rejection, this error also needs to be passed to
+    // the $exceptionHandler and be rethrown (so that the test fails).


Can you move that comment to where the error is created (i.e. right above var error = wasExpected ?).

gkalpak · 2017-09-06T08:25:16Z

src/ng/q.js

+    this.name = '$$PassToExceptionHandlerError';
+    this.message = message;
+    this.stack = (new Error()).stack;
+  }


"Subclassing" Error is actually tricky. For example, different browsers use different properties (such as line, sourceId, stackArray). Other browsers do not populate the stack property until the error is actually thrown (so (new Error).stack will be empty). And finally, the stack trace will be a little confusing by showing a slightly different location:

# Expected Error: Some error message at <where-the-error-was-actually-thrown>:<line>:<column> ... # Actual Error at PassToExceptionHandlerError (<file>:<line>:<column>) at <where-the-error-was-actually-thrown>:<line>:<column> ...

Here is what we do in ngMock to mitigate these issues:

angular.js/src/ngMock/angular-mocks.js

Lines 3109 to 3118 in ecc09a4

var ErrorAddingDeclarationLocationStack = function ErrorAddingDeclarationLocationStack(e, errorForStack) {

this.message = e.message;

this.name = e.name;

if (e.line) this.line = e.line;

if (e.sourceId) this.sourceId = e.sourceId;

if (e.stack && errorForStack)

this.stack = e.stack + '\n' + errorForStack.stack;

if (e.stackArray) this.stackArray = e.stackArray;

};

ErrorAddingDeclarationLocationStack.prototype = Error.prototype;

angular.js/src/ngMock/angular-mocks.js

Lines 3122 to 3128 in ecc09a4

var errorForStack = new Error('Declaration Location');

// IE10+ and PhanthomJS do not set stack trace information, until the error is thrown

if (!errorForStack.stack) {

try {

throw errorForStack;

} catch (e) { /* empty */ }

}

angular.js/src/ngMock/angular-mocks.js

Line 3162 in ecc09a4

throw new ErrorAddingDeclarationLocationStack(e, errorForStack);

Another idea would be wrap the error in a non-error object and unwrap it in $q:

src/ng/q.js

+$Q.$$PassToExceptionHandlerWrapper = function PassToExceptionHandlerWrapper(error) { this.error = error; } ... function processQueue(state) { ... } catch (e) { + var passToExceptionHandler = e && e instanceof PassToExceptionHandlerWrapper; + e = passToExceptionHandler ? e.error : e; rejectPromise(promise, e); + if (passToExceptionHandler) { + exceptionHandler(e); + } }

src/ngMock/angular-mocks.js

+try { + // Throw the error to ensure the `stack` property is populated. throw wasExpected ? new Error('No response defined !') : new Error('Unexpected request: ' + method + ' ' + url + '\n' + (expectation ? 'Expected ' + expectation : 'No more request expected')); +} catch (err) { + throw new $q.$$PassToExceptionHandlerWrapper(err); +}

Then again, @mgol will say this violates the Promises/A+ spec and he will be right (since we will be rejecting with a different object than the one that was thrown) 😟

Given all that, I think I might prefer the original "custom $$passToExceptionHandler property on the error" approach 😞

What do others think?

Are these issues with custom Errors still valid on the browser we support? I thought they were more IE8ish??

The "no stack until thrown" issue is IE10+ (and PhantomJS which is not officially supported).
I am not sure about the extra properties, but iirc they are Safari-specific (could be wrong though).
The "extra line in stack trace" issue is by design, but should be easy to work around.

Basically all the issues are "work-around-able". The question is, is it worth the extra complexity?
_{(I don't feel strongly about it, though. Happy to go either way.)}

So there are 3 options:

Field on normal error (previous solution

Class inheriting from error (current code in PR, plus maybe changes to the way class defined)

Class wrapping error

Which way should be implemented?

…standing request

…outstanding request

marcin-wosinek · 2017-09-13T16:41:42Z

master was updated with first suggestion from:
#16150 (comment)

the second suggestion is applied in:
https://github.com/marcin-wosinek/angular.js/tree/16150-b

the original proposition is in:
https://github.com/marcin-wosinek/angular.js/tree/16150-c

petebacondarwin · 2017-09-18T19:36:35Z

@marcin-wosinek - thank you so much for you work and patience on this PR.

We had a long chat again today and have finally decided, after all to go with your original proposal (https://github.com/marcin-wosinek/angular.js/tree/16150-c). I will squash and merge the two commits from that branch, so there is no need to update this PR any further.

I am sorry it has taken so long to deal with this.

marcin-wosinek · 2017-09-18T19:50:50Z

Cool; great to have it finally land:)

…e error handler Closes #16150 Closes #15855

googlebot added cla: yes labels Aug 6, 2017

marcin-wosinek force-pushed the master branch 2 times, most recently from 56e8cc8 to 76e5a15 Compare August 7, 2017 20:59

gkalpak suggested changes Aug 10, 2017

View reviewed changes

marcin-wosinek force-pushed the master branch from 76e5a15 to a010625 Compare August 14, 2017 20:45

gkalpak reviewed Aug 15, 2017

View reviewed changes

marcin-wosinek force-pushed the master branch from a010625 to 4e80e7e Compare August 15, 2017 12:37

marcin-wosinek force-pushed the master branch from 4e80e7e to 8f99e67 Compare September 5, 2017 07:00

marcin-wosinek force-pushed the master branch 2 times, most recently from 51a9af9 to 8f480de Compare September 6, 2017 06:33

gkalpak suggested changes Sep 6, 2017

View reviewed changes

marcin-wosinek force-pushed the master branch 2 times, most recently from 73ad3f0 to 0b894c5 Compare September 7, 2017 10:06

test($httpBackend): check if unexpected request fail on verify no out…

e735973

…standing request

marcin-wosinek force-pushed the master branch from 0b894c5 to bd53d3a Compare September 13, 2017 15:38

fix($httpBackend): solution for unexpected request fail on verify no …

94ce1de

…outstanding request

marcin-wosinek force-pushed the master branch from bd53d3a to 94ce1de Compare September 13, 2017 16:07

petebacondarwin closed this in f18dd29 Sep 18, 2017

petebacondarwin pushed a commit that referenced this pull request Sep 18, 2017

fix(ngMock): pass unexpected request failures in $httpBackend to th…

1555a49

…e error handler Closes #16150 Closes #15855

dependencies bot mentioned this pull request Dec 5, 2017

Update 26 dependencies from npm JetBrains/ring-ui#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ugly solution for unexpected request fail on verify no outstanding request #16150

ugly solution for unexpected request fail on verify no outstanding request #16150

marcin-wosinek commented Aug 6, 2017 •

edited

Loading

Narretz commented Aug 7, 2017

gkalpak left a comment

gkalpak Aug 10, 2017

gkalpak Aug 10, 2017 •

edited

Loading

gkalpak Aug 10, 2017

marcin-wosinek commented Aug 15, 2017

gkalpak Aug 15, 2017

gkalpak Aug 15, 2017

Narretz commented Aug 15, 2017

petebacondarwin commented Aug 16, 2017

gkalpak commented Aug 17, 2017 •

edited

Loading

mgol commented Aug 17, 2017

gkalpak commented Aug 17, 2017

mgol commented Aug 17, 2017 via email

gkalpak commented Aug 18, 2017

mgol commented Aug 18, 2017

gkalpak commented Aug 18, 2017

petebacondarwin commented Sep 4, 2017

Narretz commented Sep 5, 2017

gkalpak commented Sep 5, 2017

marcin-wosinek commented Sep 5, 2017

marcin-wosinek commented Sep 6, 2017

gkalpak Sep 6, 2017

marcin-wosinek Sep 7, 2017

gkalpak Sep 6, 2017

petebacondarwin Sep 6, 2017 •

edited

Loading

gkalpak Sep 6, 2017

marcin-wosinek Sep 7, 2017

marcin-wosinek commented Sep 13, 2017

petebacondarwin commented Sep 18, 2017

marcin-wosinek commented Sep 18, 2017

	var ErrorAddingDeclarationLocationStack = function ErrorAddingDeclarationLocationStack(e, errorForStack) {
	this.message = e.message;
	this.name = e.name;
	if (e.line) this.line = e.line;
	if (e.sourceId) this.sourceId = e.sourceId;
	if (e.stack && errorForStack)
	this.stack = e.stack + '\n' + errorForStack.stack;
	if (e.stackArray) this.stackArray = e.stackArray;
	};
	ErrorAddingDeclarationLocationStack.prototype = Error.prototype;

	var errorForStack = new Error('Declaration Location');
	// IE10+ and PhanthomJS do not set stack trace information, until the error is thrown
	if (!errorForStack.stack) {
	try {
	throw errorForStack;
	} catch (e) { /* empty */ }
	}

ugly solution for unexpected request fail on verify no outstanding request #16150

ugly solution for unexpected request fail on verify no outstanding request #16150

Conversation

marcin-wosinek commented Aug 6, 2017 • edited Loading

Narretz commented Aug 7, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-wosinek commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Aug 15, 2017

petebacondarwin commented Aug 16, 2017

gkalpak commented Aug 17, 2017 • edited Loading

mgol commented Aug 17, 2017

gkalpak commented Aug 17, 2017

mgol commented Aug 17, 2017 via email

gkalpak commented Aug 18, 2017

mgol commented Aug 18, 2017

gkalpak commented Aug 18, 2017

petebacondarwin commented Sep 4, 2017

Narretz commented Sep 5, 2017

gkalpak commented Sep 5, 2017

marcin-wosinek commented Sep 5, 2017

marcin-wosinek commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-wosinek commented Sep 13, 2017

petebacondarwin commented Sep 18, 2017

marcin-wosinek commented Sep 18, 2017

marcin-wosinek commented Aug 6, 2017 •

edited

Loading

gkalpak Aug 10, 2017 •

edited

Loading

gkalpak commented Aug 17, 2017 •

edited

Loading

petebacondarwin Sep 6, 2017 •

edited

Loading