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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

thorn0
Copy link
Contributor

@thorn0 thorn0 commented Jul 24, 2018 •
edited
Loading

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
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

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
Copy link
Contributor Author

thorn0 commented Jul 25, 2018 •
edited
Loading

@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
Copy link
Member

@gkalpak gkalpak left a comment

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? 😁

src/ngMock/angular-mocks.js
'EXPECTED: ' + prettyPrint(expectation.data) + '\nGOT: ' + data);
expectationError = new Error('Expected ' + expectation + ' with different data\n' +
'EXPECTED: ' + prettyPrint(expectation.data) + '\n' +
'GOT: ' + data);
}

if (!expectation.matchHeaders(headers)) {
Copy link
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?)

Copy link
Contributor Author

@thorn0 thorn0 Jul 25, 2018 •
edited
Loading

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
Copy link
Contributor Author

thorn0 commented Jul 25, 2018 •
edited
Loading

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
Copy link
Contributor Author

thorn0 commented Jul 25, 2018

@gkalpak Please have a look.

gkalpak
gkalpak suggested changes Jul 25, 2018
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Almost there 😃

src/ngMock/angular-mocks.js Outdated
@@ -1510,16 +1510,25 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
}
}

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
Copy link
Member

Choose a reason for hiding this comment

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

be being --> being

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(...) !
Copy link
Member

Choose a reason for hiding this comment

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

😱 😛

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/);
Copy link
Member

Choose a reason for hiding this comment

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

Different what? 😃

Copy link
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? :)

Copy link
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
Copy link
Contributor

Narretz commented Jul 31, 2018

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

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

@gkalpak gkalpak gkalpak approved these changes

@petebacondarwin petebacondarwin petebacondarwin approved these changes

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