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

complete coverage of javascript coalesce #86

Closed
kellyselden opened this issue Aug 3, 2013 · 10 comments
Closed

complete coverage of javascript coalesce #86

kellyselden opened this issue Aug 3, 2013 · 10 comments
Assignees

Comments

@kellyselden
Copy link

is it possible to get complete coverage of this line?

this.imageFactory = stuff.imageFactory || function() { return new Image() };

it's a hack of dependency injection. it's never evaluated to false, but the right-most is always true.

@tntim96
Copy link
Owner

tntim96 commented Aug 3, 2013

You won't get 100% branch coverage unless it evaluates to both true and false.

@kellyselden
Copy link
Author

Is this an issue you deal with often? or do you write your code without coalescing for better code coverage?

@tntim96
Copy link
Owner

tntim96 commented Aug 4, 2013

I'm open to suggestions for detecting and avoiding this. A couple of options are:

@ghost ghost assigned tntim96 Aug 5, 2013
@tntim96
Copy link
Owner

tntim96 commented Aug 23, 2013

I could add an switch which could make the branch coverage check if the compare types are boolean at run-time. A bit more difficult, but the parser could try to determine if the result of the evaluation is an assignment, and only generate the above logic in that scenario...is that what you're after?

@kellyselden
Copy link
Author

I think that would be perfect. I've been thinking about how you would write that, how to know whether the right side is a bool or not. This line would be a problem in my mind:

var oldFunc = null;
var newFunc = new function() { };
this.test = oldFunc || newFunc;

Knowing the types of those vars when on the third line seems difficult to me, but your parser may be smarter than I think.

@eidng8
Copy link

eidng8 commented Dec 30, 2014

This kind of shortcut is widely used in JS. I think you can safely ignore the outcome in coverage, as long as it's an assignment. BTW, the boolean checking may not be proper since boolean, albeit true or false, are sometimes desired. Afterall, it's null coalescing.

@eidng8
Copy link

eidng8 commented Dec 30, 2014

Second though, it may be much easier to implement a option or switch to completely turn off the true false check

@tntim96
Copy link
Owner

tntim96 commented Dec 31, 2014

OK, I've put in a CLI option, --detect-coalesce, to exclude coalesce from conditional coverage. At this stage it simply checks for an || operator in an assignment. The tests scenarios are in BranchHelperTest.

You can try the latest build at https://drone.io/github.com/tntim96/JSCover/files. Let me know if you find any issues, otherwise I'll release in a week or so.

@eidng8
Copy link

eidng8 commented Jan 4, 2015

It works. Thanks a lot.

btw, --detect-coalesce may be confusing. Wouldn't --allow-coalesce or --ignore-branch-assign be better? Or something else, sorry for my poor English. 😏

@tntim96
Copy link
Owner

tntim96 commented Jan 4, 2015

Great.

Yes the naming is difficult, but it was the best I could think of that wasn't too long (e.g. --ignore-branch-on-null-coalesce). It's not exactly a branch but a condition, and other branch assignments won't be ignored, so I thought I'd just keep it short a leave the description in the manual and help documentation.

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

No branches or pull requests

3 participants