Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMock): pass failed HTTP expectations to $errorHandler - #16644

Closed
thorn0 wants to merge 1 commit into
angular:masterfrom
thorn0:fix-http-expectations
Closed

fix(ngMock): pass failed HTTP expectations to $errorHandler#16644
thorn0 wants to merge 1 commit into
angular:masterfrom
thorn0:fix-http-expectations

Conversation

@thorn0

@thorn0 thorn0 commented Jul 24, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This was only partially fixed by f18dd29

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)

The test doesn't fail in case of a failed $httpBackend expectation

What is the new behavior (if this is a feature change)?

The test does fail.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

@thorn0
thorn0 force-pushed the fix-http-expectations branch 2 times, most recently from 268dbd4 to d548e10 Compare July 24, 2018 17:19
@thorn0 thorn0 closed this Jul 24, 2018
@thorn0 thorn0 reopened this Jul 24, 2018
@petebacondarwin petebacondarwin added this to the 1.7.x milestone Jul 25, 2018
@petebacondarwin petebacondarwin self-assigned this Jul 25, 2018
petebacondarwin
petebacondarwin approved these changes Jul 25, 2018
View reviewed changes

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - now we just need to convince Travis.

@thorn0

thorn0 commented Jul 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@petebacondarwin It won't be easy. See https://stackoverflow.com/questions/46913856/value-is-not-a-sequence-safari-exception
Basically, any console.* call causes this error in this Safari. And we have such a call in src/Angular.js:

window.console.error('AngularJS: disabling automatic bootstrap. <script> protocol indicates ' +
          'an extension, document.location.href does not match.');

@thorn0 thorn0 mentioned this pull request Jul 25, 2018
Merged
gkalpak
gkalpak suggested changes Jul 25, 2018
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch 👍
What about https://github.com/thorn0/angular.js/blob/d548e1026f137a0fc774e1ece925ad76a3bf9f45/src/ngMock/angular-mocks.js#L1553? 😁

Comment thread src/ngMock/angular-mocks.js
'GOT: ' + data);
}

if (!expectation.matchHeaders(headers)) {

@gkalpak gkalpak Jul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we are now not throwing the if block above, this should be an else if.
(Is there a test that catches this?)

@thorn0 thorn0 Jul 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now, there is. :) It breaks nothing. If both data and headers don't match, we still get an error. Do you insist on else if? I brought throws back.

@thorn0
thorn0 force-pushed the fix-http-expectations branch from d548e10 to 490d1f7 Compare July 25, 2018 16:03
@thorn0

thorn0 commented Jul 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

What about https://github.com/thorn0/angular.js/blob/d548e1026f137a0fc774e1ece925ad76a3bf9f45/src/ngMock/angular-mocks.js#L1553?

I was thinking about it. Decided that it isn't critical and can be fixed separately.

Okay, okay, fixed it too.

@thorn0
thorn0 force-pushed the fix-http-expectations branch 2 times, most recently from 483201b to 377bff8 Compare July 25, 2018 16:21
@thorn0

thorn0 commented Jul 25, 2018

Copy link
Copy Markdown
Contributor Author

@gkalpak Please have a look.

gkalpak
gkalpak suggested changes Jul 25, 2018
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Almost there 😃

Comment thread src/ngMock/angular-mocks.js Outdated

function createFatalError(message) {
var error = new Error(message);
// In addition to be being converted to a rejection, these errors also need to be passed to

@gkalpak gkalpak Jul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

be being --> being

Comment thread test/ngMock/angular-mocksSpec.js

it('should throw error when response is not defined for a backend definition', function() {
expect(function() {
hb.whenGET('/some'); // no .respond(...) !

@gkalpak gkalpak Jul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😱 😛

Comment thread test/ngMock/angular-mocksSpec.js Outdated
hb.expectPOST('/some', {foo: 1}, {X: 'val1'}).respond({});
hb('POST', '/some', {foo: 2}, callback, {X: 'val2'});
hb.flush();
}).toThrowError(/^Expected POST \/some with different/);

@gkalpak gkalpak Jul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Different what? 😃

@thorn0 thorn0 Jul 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it important? :)

@gkalpak gkalpak Jul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is 😃
It ensures that the data error takes precedence. It is not terribly important, but there is no need to break this behavior in the future (as originally happened in this PR).

@thorn0
thorn0 force-pushed the fix-http-expectations branch 2 times, most recently from c63b090 to 7f6e37f Compare July 25, 2018 17:12
@thorn0
thorn0 force-pushed the fix-http-expectations branch from 7f6e37f to b8f8554 Compare July 26, 2018 09:37
@Narretz

Narretz commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

@gkalpak it looks like your change requests have been applied, no?

@gkalpak gkalpak closed this in ebeb1c9 Jul 31, 2018
This was referenced Jul 9, 2023
Closed
Open
Open
Open
Open
Open
This was referenced Jul 16, 2023
Open
Open
Open
Closed
Open
Open
Open
Open
This was referenced Sep 18, 2023
Merged
Open
@renovate-bot renovate-bot mentioned this pull request Oct 26, 2023
Closed
1 task
@renovate renovate Bot mentioned this pull request Dec 4, 2023
Closed
1 task
@renovate renovate Bot mentioned this pull request Jan 2, 2024
Open
1 task
This was referenced Feb 1, 2024
Closed
Closed
Closed
This was referenced Jul 22, 2024
Open
Open
Open
This was referenced Aug 28, 2024
Closed
Merged
Closed
Open
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@petebacondarwin petebacondarwin petebacondarwin approved these changes
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@petebacondarwin petebacondarwin

Labels

cla: yes needs: work type: bug

Projects

None yet

Milestone

1.7.x

Development

Successfully merging this pull request may close these issues.

5 participants

@thorn0 @Narretz @petebacondarwin @gkalpak @googlebot