Uh oh!
There was an error while loading. Please reload this page.
Pr 11547 - #14367
Conversation
googlebot
commented
Apr 4, 2016
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
googlebot
commented
Apr 4, 2016
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
| this.addEventListener = function(name, listener) { | ||
| if (angular.isUndefined(this.$$events[name])) this.$$events[name] = []; | ||
| this.$$events[name].push(listener); | ||
| }; |
There was a problem hiding this comment.
I am kind of confused with this snippet:
When we create a MockXhr, we set this.$$events = {}.
Then inside the mock $httpBackend, right after we create xhr = new MockXhr(), we overwrite xhr.$$events = eventHandlers (where eventHandlers is of type {[key: string]: function}).
Then, when addEventListener is called on a MockXhr instance, we check create an array on this.$$events[name] and push the listener.
I would expect the tests to fail, but they obviously don't, so what do I miss ?
There was a problem hiding this comment.
I agree this is not optimal.
The MockXhr is used in two different scenarios:
- In the
$httpBackendtests we instantiateMockXhrobjects and pass them through to the$httpBackendservice, which does calladdEventListener. This allows us to test the "real" use of the$httpBackendservice. - In the
$httptests, we have a mock$httpBackendservice (fromngMock), which happens to hold its own instance ofMockXhr. In these tests we never calladdEventListenerand I just chose to use the same$$eventsobject for holding our mock event handlers for the test
There was a problem hiding this comment.
I see 😕
(Clarification comments welcome 😄
gkalpak
commented
Apr 7, 2016
LGTM |
benjamingr
commented
Apr 8, 2016
I'd like to see more tests for this but overall nice work :) |
petebacondarwin
commented
Apr 8, 2016
@benjamingr - what further tests would you like to see? |
benjamingr
commented
Apr 10, 2016
Well, the only test right now is that events are passed in one way - there is a single test, I'd like to se more coverage. I'm also not sure how users are supposed to mock it, that should probably be documented. |
petebacondarwin
commented
Apr 10, 2016
@benjamingr could you be more explicit. What code paths are not being covered? |
petebacondarwin
commented
Apr 10, 2016
@benjamingr Can you create a new issue for improving the documentation of mocking the events so that we can track it? |
- Add a "progress" dialog in case the upload takes a long time - Note that there is no progressbar in the dialog since listening to events was apparently only supported in angular 1.5.5 angular/angular.js#14367 - pass in parameters as past of the POST message that can be interpreted on the server
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
Closes#11547
Closes#1934
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: