fix(build): Fix failing tests with Angular 1.6. #8404
Conversation
After some recent changes to Angular's master branch (which will become 1.6), a few tests were failing. Update our code to work with 1.3, 1.4, 1.5 and the eventual 1.6.
ThomasBurleson
added
pr: merge ready
| @@ -818,7 +818,7 @@ describe('$mdDialog', function() { | |||
|
|
|||
| it('should not wrap raw content with md-dialog', inject(function($mdDialog, $rootScope) { | |||
|
|
|||
| var template = '<div id="rawContent">Hello</div>'; | |||
| var template = '<md-dialog id="rawContent">Hello</md-dialog>'; | |||
There was a problem hiding this comment.
Since we are testing raw content, this should be a <div> right ?
There was a problem hiding this comment.
I think this test tests essentialy the same thing as the "should not wrap content with existing md-dialog" one (two tests above). If raw content should always manually include <md-dialog> then it should be more explicit imo.
There was a problem hiding this comment.
@ThomasBurleson With a <div>, the test fails because it's trying to find an <md-dialog> and it can't, so we get an error on element[0].parentNode: cannot find parentNode of undefined.
@gkalpak I think it's definitely similar, but we do assume that the raw content will have a dialog. I think the test was passing erroneously because we were swallowing an error that in 1.6 was getting through.
In fact, the test description talks about the <md-dialog>, so I think this was just an oversight.
How do you think we could make this better or more clear? Do you mean adding some documentation?
There was a problem hiding this comment.
The test was indeed passed erroneously. From a quick look at the docs, it's not clear what the user should pass as template if atuoWrap is false.
- Could it be an HTML string that contains
<md-dialog>? - Could it be an HTML string containing a directive whose template contains
<md-dialog>? - Could it be any HTML string ?
(Based on the test I assume (3) isn't an option. If (2) is an option, then I would test for that here. If only (1) is possible, then I think the test above covers it, so this might be redundant.)
But, yeah, I meant that the docs should be more explicit (unless I missed that).
| expect($timeout.cancel).toHaveBeenCalled(); | ||
| expect(holdSpy).not.toHaveBeenCalled(); | ||
|
|
||
| $timeout.verifyNoPendingTasks(); | ||
| // We originally also called `$timeout.verifyNoPendingTasks();` here, however, changes made to | ||
| // $timeout.cancel() in 1.6 adds more tasks to the deferredQueue, so this will fail. |
There was a problem hiding this comment.
BTW, this is an issue with Angular and will be fixed soon-ish (see angular/angular.js#14336).
There was a problem hiding this comment.
@gkalpak Good to know! I'll follow that one.
After some recent changes to Angular's master branch (which will become 1.6), a few tests were failing with unhandled rejections of promises. Update our code to work with 1.3, 1.4, 1.5 and the eventual 1.6. Closes angular#8404
After some recent changes to Angular's master branch (which will become 1.6), a few tests were failing.
Update our code to work with 1.3, 1.4, 1.5 and the eventual 1.6.