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

fix(coverage): get close to 100% test coverage #67

Merged
merged 1 commit into from
Feb 17, 2018
Merged

fix(coverage): get close to 100% test coverage #67

merged 1 commit into from
Feb 17, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 11, 2018

It would be great if we could get to 100% coverage, especially as we're so close!

Missing a couple of branches in utils.js, but I've also added a couple of ignores I would like to drop. Help appreciated!

Missing coverage:
image

@@ -1,19 +1,16 @@
'use strict';

function getName(node) {
function joinNames(a, b) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@borilla mind sharing why you added the check? None of the tests hit this path, so I just inlined it

Copy link
Contributor

Choose a reason for hiding this comment

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

Just defensive coding, but I can come up with a theoretical example using an anonymous object:

({ f: function () {} }).f()

}

/* istanbul ignore next */
return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this just to be future proof?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is for any unmatched node-types (ie those we can't find names for), eg

(a || b).f(); // callee.object here is LogicalExpression

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks! Added those 2 examples, and coverage hit 100% 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -27,6 +27,7 @@ module.exports = {
create: context => ({
CallExpression(node) {
const callee = node.callee;
/* istanbul ignore if */
if (!callee) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jkimbo this is an old one, can you provide details on the cases where a CallExpression has no callee?

@@ -17,6 +17,7 @@ const isExpectCallPresentInFunction = body => {
return body.body.find(line => {
if (line.type === 'ExpressionStatement')
return isExpectCall(line.expression);
/* istanbul ignore else */
if (line.type === 'ReturnStatement') return isExpectCall(line.argument);
Copy link
Member Author

Choose a reason for hiding this comment

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

@tushardhole can you provide some sample code where this condition is false?

@SimenB
Copy link
Member Author

SimenB commented Feb 11, 2018

@xfumihiro Do you think we could add some tests covering the uncovered parts of utils.js?

@SimenB SimenB changed the title fix(coverage): get too 100% test coverage fix(coverage): get to 100% test coverage Feb 11, 2018
@SimenB SimenB force-pushed the coverage branch 7 times, most recently from 90a7104 to d56a0ed Compare February 17, 2018 18:41
@SimenB SimenB changed the title fix(coverage): get to 100% test coverage fix(coverage): get close to 100% test coverage Feb 17, 2018
@SimenB
Copy link
Member Author

SimenB commented Feb 17, 2018

I'm gonna merge as is, and open up a separate issue for 100%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants