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

fix(build): Fix failing tests with Angular 1.6. #8404

Closed
wants to merge 1 commit into from
Closed

fix(build): Fix failing tests with Angular 1.6. #8404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/dialog/dialog.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>';
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are testing raw content, this should be a <div> right ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

  1. Could it be an HTML string that contains <md-dialog> ?
  2. Could it be an HTML string containing a directive whose template contains <md-dialog> ?
  3. 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).

var parent = angular.element('<div>');

$mdDialog.show({
Expand All @@ -830,7 +830,7 @@ describe('$mdDialog', function() {
$rootScope.$apply();

var container = parent[0].querySelectorAll('md-dialog');
expect(container.length).toBe(0);
expect(container.length).toBe(1); // Should not have two dialogs; but one is required
}));
});

Expand Down
7 changes: 6 additions & 1 deletion src/components/icon/icon.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,13 +529,18 @@ describe('mdIcon service', function() {
});
});

/*
* Previous to Angular 1.6, requesting an icon that is not found would throw no errors. After
* 1.6, since we do not have a .catch() handler, it now throws a Possibly Unhandled Rejection
* error.
*/
describe('icon is not found', function() {
it('should not throw Error', function() {
expect(function(){
$mdIcon('notfound');

$httpBackend.flush();
}).not.toThrow();
}).not.toThrow('Cannot GET notfoundicon.svg');
Copy link
Member

Choose a reason for hiding this comment

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

This test is quite brittle (e.g. it coould be throwing a different error or there might be a typo in the error message).
It might be better to either chain a .catch() to avoid the unhandled exception error.

TBH, I'm not sure what this test verifies and what the expected behavior is or why would it throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We went back and forth on this single test for almost an hour. The issue is that if we .catch() the error, then it definitely won't throw anything, which makes the not.toThrow() useless. In ng1.3-1.5, it doesn't throw anything, but in 1.6 it will throw a Possibly Unhandled Rejection because we're not handling it anywhere (which is what was designed; we wanted to ignore this particular type of error).

I agree that it is a bit brittle if the error description changes, but I think I would want this test to fail if the error changes so that we know. I'm slightly worried that if something else throws an error (besides the PUR in 1.6) that it will get swallowed, but we have a lot of other tests that should catch that too.

We honestly talked about just removing the test, but I think it's better to have it than to not.

I think all this test is really trying to say is "if we can't find the icon, we shouldn't throw an icon not found error", and that's all we really want to guarantee.

I'm always open to thoughts on how to fix it if you have a moment to dive in and try some code 😄

Copy link
Member

Choose a reason for hiding this comment

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

I mean if you are verifying that .not.toThrow('Cannot GET notfoundicon.svg'); the test would pass even if it throws 'Can't GET notfoundicon.svg' (just because the error messahe is slightly different.

I am confused because I am not sure if you want to verify that it doesn't throw an error or that the promise isn't rejected. The way I'm thinking it, I would rather chain a .catch() callback and verify that it is called with the specified "thing" (object, thing, error) or not called (if that is what is supposed to happen).

});
});
});
Expand Down
12 changes: 11 additions & 1 deletion src/core/services/gesture/gesture.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ describe('$mdGesture', function() {
maxDistance: 10
});

// Setup our spies and trigger our first action (touchstart)
el.on('$md.hold', holdSpy);
spyOn($timeout, 'cancel').and.callThrough();

Expand All @@ -303,18 +304,27 @@ describe('$mdGesture', function() {
target: el[0],
touches: [{pageX: 100, pageY: 100}]
});

// The $md.hold spy should NOT have been called since the user has not lifted their finger
expect(holdSpy).not.toHaveBeenCalled();

// Reset calls to $timeout.cancel so that we can ensure (below) that it is called and
// trigger our second action (touchmove)
$timeout.cancel.calls.reset();

$document.triggerHandler({
type: 'touchmove',
target: el[0],
touches: [{pageX: 90, pageY: 90}]
});

// Because the user moves their finger instead of lifting, expect cancel to have been called
// and the $md.hold spy NOT to have been called
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.
Copy link
Member

Choose a reason for hiding this comment

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

BTW, this is an issue with Angular and will be fixed soon-ish (see angular/angular.js#14336).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak Good to know! I'll follow that one.

}));

it('should not reset timeout if moving < options.maxDistance', inject(function($mdGesture, $document, $timeout) {
Expand Down
4 changes: 2 additions & 2 deletions src/core/services/interimElement/interimElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ function InterimElementProvider() {
element = linkElement( compiledData, options );

showAction = showElement(element, options, compiledData.controller)
.then(resolve, rejectAll );
.then(resolve, rejectAll);

}, rejectAll);

Expand Down Expand Up @@ -673,7 +673,7 @@ function InterimElementProvider() {
}

} catch(e) {
reject(e.message);
reject(e);
}
});
}
Expand Down
17 changes: 12 additions & 5 deletions src/core/services/interimElement/interimElement.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ describe('$$interimElement service', function() {
});

describe('a service', function() {
var Service;
var Service, ieShow;

beforeEach(function() {
setup();
Expand All @@ -263,6 +263,8 @@ describe('$$interimElement service', function() {

Service = $$interimElement();

ieShow = Service.show;
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think it's not a good idea to mock services methods in tests only for convenience, as it might hide actual issues (or make breakages hard to debug). I would create helper functions instead (e.g. showAndFlush()) and call those when necessary. This makes it much easier to understand what is happening in each test (e.g. "here we are calling .show() and the flush()").

When tinkering with this locally, I used the following helpers:

function callMethodAndFlush(method, args) {
  var result = Service[method].apply(Service, args);
  $material.flushInterimElement();

  return result;
}

function cancelAndFlush() {
  return callMethodAndFlush('cancel', arguments);
}

function hideAndFlush() {
  return callMethodAndFlush('hide', arguments);
}

function showAndFlush() {
  return callMethodAndFlush('show', arguments);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, we may want to refactor it and use something like this; and I'll definitely do this in future tests.


Service.show = tailHook(Service.show, flush);
Service.hide = tailHook(Service.hide, flush);
Service.cancel = tailHook(Service.cancel, flush);
Expand All @@ -288,9 +290,12 @@ describe('$$interimElement service', function() {
};

// `templateUrl` is invalid; element will not be created
Service.show({

// We use the original $$interimElement.show so that we ignore the tailhook and manually
// run it
ieShow({
templateUrl: 'testing.html',
onShow : function() { return $q.reject("failed"); }
onShow : function() { return $q.reject("failed"); }
})
.catch( onShowFail );
$timeout.flush();
Expand All @@ -303,7 +308,9 @@ describe('$$interimElement service', function() {
showFailed = reason;
};

Service.show({
// We use the original $$interimElement.show so that we ignore the tailhook and manually
// run it
ieShow({
templateUrl: 'testing.html',
onShow : function() { throw new Error("exception"); }
})
Expand Down Expand Up @@ -714,11 +721,11 @@ describe('$$interimElement service', function() {
return function() {
var args = Array.prototype.slice.call(arguments);
var results = sourceFn.apply(null, args);

hookFn();

return results;
}
}

});

4 changes: 2 additions & 2 deletions test/angular-material-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ angular.module('ngMaterial-mock', [
$delegate.flush = function() {
var args = Array.prototype.slice.call(arguments);
try { ngFlush.apply($delegate, args); }
catch(e) { ; }
catch(e) { }
};

return $delegate;
});

}])
}]);

/**
* Stylesheet Mocks used by `animateCss.spec.js`
Expand Down