Add 'ignoreGuardStatements' option

[oguimbal]

Motivation

One writes dummy statements like this on a daily basis:

if (!myArgument) {
   throw new Error('my argument should not be null');
}

They are quite harmfull, often "useless" (just here to check something that is supposed to never happen), and are, most of the time, useless to check for coverage.

This pull requests allows to pass an option to istanbul instrumenter that systematically ignores such 'guard' statements, without having to clutter our code with thousands of /* istanbul ignore if */s

Solution

What would you think of the ignoreGuardStatements that this pull request introduces ?

It works as follows:

// When passing { ignoreGuardStatements: ['returns'] } 
if (condition) {
   return whatever; // will ignore any return statement
}

// When passing { ignoreGuardStatements: ['literalReturns'] }
if (condition) {
   return false; // will only ignore literal values (numbers, strings, null, undefined)
}

// When passing { ignoreGuardStatements: ['identifierReturns'] }
if (condition) {
   return myVariable; // will only ignore identifier return values
}

// When passing { ignoreGuardStatements: ['voidReturns'] }
if (condition) {
   return; // will only ignore void returns
}

// When passing { ignoreGuardStatements: ['throws'] }
if (condition) {
   throw whatever: // will ignore any throw statement
}

// When passing  { ignoreGuardStatements: ['continues'] }
while (condition1) {
  if (condition2) {
     continue; // will be ignored
  }
}

// When passing  { ignoreGuardStatements: ['breaks'] }
while (condition1) {
  if (condition2) {
     break; // will be ignored
  }
}

Of course, you can combine several of these options to ignore multiple guard patterns (that's why the option is an array).

The git is pretty simple: It ignores automatically any if or else statement that has a single statement body which matches one of the patterns provided as option (return, break, throw...)

[furudean]

If it's just one or two inputs you're checking in a test, is leaving a comment telling nyc to ignore the coverage really so spammy?

And if you are validating more data, I think you should rely on a validation framework, like joi. Then it would just be one if statement that you'd have to check.

And besides - should you not be testing that your guard statements work as you expect? You are not really covering all your bases if not.

[oguimbal]

@c-bandy Yes, I personnaly dislike having half my comments being coverage tests ignores. It makes it hard to see actual comments.
When there is a real reason to ignore a statement, thats OK (and I would usually argue in the comment why this is ignored).
But when it's a dummy check, I find it quite annoying, and it forces me to write ~twice as much code when writing those checks.

As it happens, I already extensively use Joi. But:

• The same kind problem arises with Joi... if(Joi.validate(xxx).error) throw new Error('blah blah'); // THIS WILL NOT BE COVERED
• Joi is a bit overkill to check for things like arguments being null, isnt it ?
• Its not only about data validation, you could also have something like: if(this.cachedValue) return this.cachedValue that you dont want to bother to check for cover since it might be trivial.

As for your question ("Should you not be testing...") => Some checks are just impossible (or at least very hard) to test because they're just safety checks what will never happen.
Having them lower the coverage scores just puts an incentive on not writing any of those healthy guards, since they might be hard to test.

[furudean]

Then I have to question why you have these safety checks anyway if you don't think they are important enough to test for

[oguimbal]

Why would anyone write coverage tests ?
It surely isnt to tell that any single line of code is covered...
I personally write coverage to tell me that the logic I wrote is OK, not tell me that I checked every edge cases that wont happen in practice.

Picture this method:

const getMember = obj => obj.member;

As a developer writing it, I could just decide: ok, lets be nice with others, and add a guard:

const getMember = obj => {
   if (!obj) {
        throw new Error('Dude, you the "obj " argument you've given me is not okay');`
   }
   return obj.member;
}

This guard is not a piece of logic, I dont care to test it, this method will fail either case... it's just here to have nicer logs.

On the other hand, if I'm writing those kind of guards, my coverage percentage is dropping... which just encourages me not to write them, and to fill my logs with unexpressive cannot read member of undefined logs.

Oh, sure I could write an /% istanbul ignore if %/ comment, but it just doubles the cost of me being nice, and cluttered my code with a comment that brings nothing to the reader.

So why would I write those ? Just to be nice, to add a bit of robusteness, readability and disambiguation.

[oguimbal]

But again, I wrote this to fill a need that I had. I can only offer my point of view :)

If you tell me "sorry, that's not our philosophy", I wont have anything to add to it, but I really feel that istanbul is enforcing a level of strictness and constraint in those kind of use cases that seems a bit overwhelming.

As a user, I would really like it to help me opt-out of this kind of things easily.

[furudean]

FYI, I'm not really a contributor, or responsible for what goes into the software. I am merely a user.

What exactly is it about having multiple comments that is messy? I think you are just being explicit about which behaviour you would like to have. If you don't like the comments, you can remove them. You will have less code coverage, but that code coverage was only really a lie to begin with. It was uncovered code after all.

With your solution, you solve your problem in particular but open up a lot of ambiguity in other cases. "Is this always desired behaviour" is a question you have to ask yourself with a global option like this. Because any other user in the same context may not expect, or want this.

It sounds a bit too magic for me personally. Just my 2¢.

[oguimbal]

In our case, there is not much ambiguity given that most of the developers are not even aware that we have code coverage.

Its just a metric that help me to manage people, in order to know who tests well, or who must be encouraged to test more.

The other problem when developpers dont pay attention or are not aware of coverage testing:
They might be tempted to modify this:

/% istanbul ignore if %/
if (!obj) {
   throw new Error('Error !');
}

copy/paste it or edit and have something like that:

/% istanbul ignore if %/
if (!obj) {
   sendEmailToUser('There is an error');
   throw new Error('Error !');
}

... well now, a comment not removed hides the fact that a method that had no logic and now has is untested...

I really like the idea that if somebody enriches a condition that was too simple to require coverage, it automatically pops in my radar.

[ljharb]

Coverage tests are exactly to ensure every single line is covered - any ignored line is something that's not tested.

[oguimbal]

@ljharb I beg to differ... checking that every single line is covered is not a means to an end. There is no point in thinking about lines.

In my opinion, when you write code, you dont write "lines", you write your intention. You write business logic. Lines of code are just the mean to achieve your goal.

So again in my opinion, it's pointless to think about covering lines. You want to ensure that your business logic has been covered by tests. Not thinking like that leads (you guessed it, "in my opinon") to thinking your tests around your code, not around your logic. Well, I think that it just incentives you to lock down implementation details in unit tests, and to write less expressive tests.

That's why I'd like to get everything that's not business logic covering out of the way without having to think about it too much.

But I can see how it is a matter of personal preference. That's why I understand this point of view cannot be a default (my pull request is an opt-in feature).

However, I'm pretty sure I'm not the only one thinking like that. The others are just kind of happy having 80% coverage. I'm not. I'd like to have a 100% coverage that ignored dummy code without having to write thousands of comments for that.
I'd like my coverage to tell me "hey, there is some code that seems useful that you've not covered". Not "hey, there is some code that you've not covered".

In other words, I dont see any point in checking that code you dont really care about is tested.

[ljharb]

You’re totally right - but error conditions are part of the intention you absolutely want to cover.

If you have ignores anywhere, then you don’t actually have 100% coverage - 100% means every line/branch/function/statement.

[oguimbal]

You're semantically right. 100% is 100%.

You might even be right when you speak to a library developer. You really want to test everything if you want to have a library that exposes a stable api for every single edge case.

But why has the istanbul ignore comment been introduced, then ?

I dont feel so much compelled to do so on a business application, on which I dont have time, resource, nor real interest to invest on testing the edge cases so much. It's easy to forget that coverage is NOT unit testing. If an edge case is important, just test it. You dont need your coverage to force you to do it.

In those cases, I really find it useful to know which part of my application have significant parts uncovered.
And if I want to really know the actual coverage including dummy guard statements, I can always rerun coverage without the option to evict such statements (which you cant if you put an ignore comment, by the way).