Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise test coverage #49

Merged
merged 4 commits into from
Oct 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
86 changes: 49 additions & 37 deletions app/scripts/angular-apimock.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ angular.module('apiMock', [])
var $location;
var $log;
var $q;
var $filter;
var config = {
defaultMock: false,
mockDataPath: '/mock_data',
Expand All @@ -68,7 +69,28 @@ angular.module('apiMock', [])
return keys;
}

// Taken from Angular 1.4.x: https://github.com/angular/angular.js/blob/f13852c179ffd9ec18b7a94df27dec39eb5f19fc/src/Angular.js#L296
// TODO: IE8: remove when we drop IE8/Angular 1.2 support.

Choose a reason for hiding this comment

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

Why IE8 support? For coverage?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The hint is right there in the comment: we support Angular 1.2. So then we need to support whatever browsers it supports, which includes IE8.

Choose a reason for hiding this comment

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

Fair enough!

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

// Date.prototype.toISOString isn't supported in IE8. Which we need to support as long as we support Angular 1.2.
// Modified from MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString
function toISOString(date) {
function pad(number) {
if (number < 10) {
return '0' + number;
}
return number;
}

return date.getUTCFullYear() +
'-' + pad(date.getUTCMonth() + 1) +
'-' + pad(date.getUTCDate()) +
'T' + pad(date.getUTCHours()) +
':' + pad(date.getUTCMinutes()) +
':' + pad(date.getUTCSeconds()) +
'.' + (date.getUTCMilliseconds() / 1000).toFixed(3).slice(2, 5) +
'Z';
}

// Taken as-is from Angular 1.4.x: https://github.com/angular/angular.js/blob/f13852c179ffd9ec18b7a94df27dec39eb5f19fc/src/Angular.js#L296
function forEachSorted(obj, iterator, context) {
var keys = objectKeys(obj).sort();
for (var i = 0; i < keys.length; i++) {
Expand All @@ -77,50 +99,49 @@ angular.module('apiMock', [])
return keys;
}

// Taken from Angular 1.4.x: https://github.com/angular/angular.js/blob/929ec6ba5a60e926654583033a90aebe716123c0/src/ng/http.js#L18
// Modified from Angular 1.4.x: https://github.com/angular/angular.js/blob/929ec6ba5a60e926654583033a90aebe716123c0/src/ng/http.js#L18
function serializeValue(v) {
if (angular.isObject(v)) {
return angular.isDate(v) ? v.toISOString() : angular.toJson(v);
if (angular.isDate(v)) {

Choose a reason for hiding this comment

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

Intent is much clearer now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

return toISOString(v);
}

Choose a reason for hiding this comment

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

Shouldn't the last return statement be
return angular.isObject(v) ? angular.toJson(v) : v;
to cover all cases, instead of
return v;
?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The cases don't get more covered with a ternary operator, compared to a regular if/guard clause? It would be a one-liner which might be nicer, but it won't cover anything different?

Choose a reason for hiding this comment

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

Sorry, I was rather unclear:

The function body is now

     function serializeValue(v) {
        if (angular.isDate(v)) {
            return toISOString(v);
        }

        return v;
    }

So right now, v is not serialized if it's an object, unless I'm missing something. Below is a full example. Ternary or if statement doesn't matter, but if there would be more corner cases later, it will be cleaner to default to a ternary in my opinion :)

    function serializeValue(v) {
        if (angular.isDate(v)) {
            return toISOString(v);
        }

        return angular.isObject(v) ? angular.toJson(v) : v;
    }

Copy link
Owner Author

Choose a reason for hiding this comment

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

Did you see the diff? You commented on the lines above it. Your example is kinda how it was before :D
But that was useless. My test coverage report from back then said it wasn't being tested: https://coveralls.io/builds/3755120/source?filename=app%2Fscripts%2Fangular-apimock.js#L83 (their page scrolls so that line is the first line in the browser and easily missed, so check the line nr in the link ^^)

So obviously I thought that I forgot some cases, which I kinda did and lead to the date tests being added (that you seemed to like). But another reason was that isObject/toJson was already avoided because of this:

} else if (angular.isObject(toSerialize) && !angular.isDate(toSerialize)) {
forEachSorted(toSerialize, function (value, key) {
serialize(value, prefix +
(topLevel ? '' : '[') +
key +
(topLevel ? '' : ']'));
});

Choose a reason for hiding this comment

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

Aaah that makes sense. Yeah, that makes it even more clear that it should be removed, it fooled me totally.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

return v;
}

// Taken from Angular 1.4.x: https://github.com/angular/angular.js/blob/720012eab6fef5e075a1d6876dd2e508c8e95b73/src/ngResource/resource.js#L405
function encodeUriQuery(val, pctEncodeSpaces) {
// Modified from Angular 1.4.x: https://github.com/angular/angular.js/blob/720012eab6fef5e075a1d6876dd2e508c8e95b73/src/ngResource/resource.js#L405
function encodeUriQuery(val) {
return encodeURIComponent(val).
replace(/%40/gi, '@').
replace(/%3A/gi, ':').
replace(/%24/g, '$').
replace(/%2C/gi, ',').
replace(/%20/g, (pctEncodeSpaces ? '%20' : '+'));
replace(/%20/g, '+');

Choose a reason for hiding this comment

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

Much cleaner 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍
Thx ^^ the parameter wasn't used anyway. Just cargo cult code that came along when copying the stuff from the Angular source code.

}

// TODO: replace with a $httpParamSerializerJQLikeProvider() call when we require Angular 1.4 (i.e. when we drop 1.2 and 1.3).
// Taken from Angular 1.4.x: https://github.com/angular/angular.js/blob/929ec6ba5a60e926654583033a90aebe716123c0/src/ng/http.js#L108
// Modified from Angular 1.4.x: https://github.com/angular/angular.js/blob/929ec6ba5a60e926654583033a90aebe716123c0/src/ng/http.js#L108
function jQueryLikeParamSerializer(params) {
if (!params) {
return '';
}

var parts = [];

function serialize(toSerialize, prefix, topLevel) {
if (toSerialize === null || angular.isUndefined(toSerialize)) {
return;
}

if (angular.isArray(toSerialize)) {
// Serialize arrays.
angular.forEach(toSerialize, function (value, index) {
serialize(value, prefix + '[' + (angular.isObject(value) ? index : '') + ']');
});
} else if (angular.isObject(toSerialize) && !angular.isDate(toSerialize)) {
// Serialize objects (not dates, because that's covered by the default case).
forEachSorted(toSerialize, function (value, key) {
serialize(value, prefix +
(topLevel ? '' : '[') +
key +
(topLevel ? '' : ']'));
});
} else if (toSerialize === undefined || toSerialize === '') {
// Keep empty parameters as it still affects the mock file path.
parts.push(encodeUriQuery(prefix));

Choose a reason for hiding this comment

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

Why still push the prefix if toSerialize is null or empty?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because the result is still useful. The whole point of supporting query parameters is that it changes the .json file you get in the end. If I'd ignore empty query values then we'd still return the base .json. See https://github.com/seriema/angular-apimock/blob/chore-44-raise-test-coverage/test/spec/services/angular-apimock.js#L731-L746

Or see the bugfix it was related to. (see if you can find the reference from the commit, see it as a challenge ;) )

Choose a reason for hiding this comment

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

Yeah, I figured after i commented. But a small inline comment, linking to your comment above, would be nice. This is definitely something someone could remove by accident.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done! 👍

} else {
// Serialize everything else (including dates).
parts.push(encodeUriQuery(prefix) + '=' + encodeUriQuery(serializeValue(toSerialize)));
}
}
Expand Down Expand Up @@ -247,15 +268,13 @@ angular.module('apiMock', [])
}

function removeFallback(res) {
var found = false;
angular.forEach(fallbacks, function (fallback, index) {
if (fallback.method === res.method && fallback.url === res.url) {
found = true;
fallbacks.splice(index, 1);
}
});
var startLength = fallbacks.length;
fallbacks = $filter('filter')(fallbacks, {
method: '!' + res.method,
url: '!' + res.url
}, true);

return found;
return startLength > fallbacks.length;

Choose a reason for hiding this comment

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

superclean 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! I feel it's super obscure to the people that don't know how it should be called. Maybe I should link to https://docs.angularjs.org/api/ng/filter/filter ?

But it feels like it's a lot better tested anyway and I was loosing code coverage because I wasn't testing it enough, which feels unnecessary since the point of apiMock isn't to reinvent "remove elements from array".

Choose a reason for hiding this comment

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

No links needed. If anyone is unsure what filter is, they'll google it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

}

function reroute(req) {
Expand Down Expand Up @@ -309,10 +328,11 @@ angular.module('apiMock', [])
// Expose public interface for provider instance
//

function ApiMock(_$location, _$log, _$q) {
function ApiMock(_$location, _$log, _$q, _$filter) {

Choose a reason for hiding this comment

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

is this a breaking change?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nope. Angular looks at that and injects whatever I want. $filter is supported in 1.2 and up.

$location = _$location;
$log = _$log;
$q = _$q;
$filter = _$filter;
}

var p = ApiMock.prototype;
Expand Down Expand Up @@ -342,18 +362,13 @@ angular.module('apiMock', [])
case 'respond':
return httpStatusResponse(command.value);
case 'ignore':
return req;

/* falls through */

Choose a reason for hiding this comment

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

clean 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

The /* falls through */ might be a bit obscure though. It's not a regular comment. It's a JSHint thing: see "Switch statements" on http://jshint.com/docs/

default:
return req;
}
};

p.onResponse = function (res) {
if (config.disable) {
return res;
}

removeFallback(res);
return res;
};
Expand Down Expand Up @@ -382,8 +397,8 @@ angular.module('apiMock', [])
angular.extend(config, options);
};

this.$get = function ($location, $log, $q) {
return new ApiMock($location, $log, $q);
this.$get = function ($location, $log, $q, $filter) {
return new ApiMock($location, $log, $q, $filter);
};
})

Expand All @@ -393,10 +408,7 @@ angular.module('apiMock', [])
* `apiMock` to determine if a mock should be done, then do the actual mocking.
*/
this.request = function (req) {
req = apiMock.onRequest(req);

// Return the request or promise.
return req || $q.when(req);
return apiMock.onRequest(req);
};

this.response = function (res) {
Expand Down
2 changes: 2 additions & 0 deletions karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ module.exports = function (config) {
// You need to set `SAUCE_USERNAME` and `SAUCE_ACCESS_KEY` as environment variables.
sauceLabs: {
testName: 'Angular ApiMock',
recordVideo: false,
recordScreenshots: false,
startConnect: true
},

Expand Down
46 changes: 46 additions & 0 deletions test/spec/services/angular-apimock.js
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,52 @@ describe('Service: apiMock', function () {

expectHttpSuccess();
});

Choose a reason for hiding this comment

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

Over the range clear, consise, meaningful and readable tests.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

it('should serialize objects nested inside arrays', function () {
defaultRequest.url = '/api/pokemon';
defaultRequest.params = {
'moves': [ {
'Thunderbolt': {
power: 95,
type: 'Electric'
}}, {
'Double Edge': {
power: 120,
type: 'Normal'
}
} ]
};
defaultExpectPath = '/mock_data/pokemon/moves%5b0%5d%5bthunderbolt%5d%5bpower%5d=95&moves%5b0%5d%5bthunderbolt%5d%5btype%5d=electric&moves%5b1%5d%5bdouble+edge%5d%5bpower%5d=120&moves%5b1%5d%5bdouble+edge%5d%5btype%5d=normal.get.json';

expectHttpSuccess();
});

it('should handle empty value', function () {
defaultRequest.url = '/api/pokemon?releaseDate';
defaultExpectPath = '/mock_data/pokemon/releasedate.get.json';

expectHttpSuccess();
});

it('should handle undefined value', function () {
defaultRequest.url = '/api/pokemon';
defaultRequest.params = {
'releaseDate': undefined
};
defaultExpectPath = '/mock_data/pokemon/releasedate.get.json';

expectHttpSuccess();
});

it('should serialize date type', function () {
defaultRequest.url = '/api/pokemon';
defaultRequest.params = {
'releaseDate': new Date(Date.UTC(96, 1, 27, 0, 0, 0))
};
defaultExpectPath = '/mock_data/pokemon/releasedate=1996-02-27t00:00:00.000z.get.json';

expectHttpSuccess();
});
});

describe('delay option', function () {
Expand Down