gotwarlost / istanbul

Yet another JS code coverage tool that computes statement, line, function and branch coverage with module loader hooks to transparently add coverage when running tests. Supports all JS coverage use cases including unit tests, server side functional tests and browser tests. Built for scale.
Other
8.7k stars 787 forks source link

'istanbul ignore else' inbetween a 'else' and 'if' is ignored #781

Open jwalton opened 7 years ago

jwalton commented 7 years ago

See also #230.

screen shot 2017-03-16 at 4 26 29 pm

You can work around it with some extra braces:

screen shot 2017-03-16 at 4 28 13 pm

nmarra commented 7 years ago

I'm having this same issue, it seems like a problem with the branch coverage though because as the coverage shows the 'else if' case is covered by a unit test. It's very confusing to get the 'else path not taken' marking when the 'else if' is clearly covered as shown by the 11x marking on the right side.

woutervanvliet commented 5 years ago

Just found this issue when running into the same problem. I solved it by adding the ignore line above the first if, like so:


/* istanbul ignore else */
if (element) {
    element.addEventListener('animationend', this.handleAnimationEnd)
} else if (this.contentElement) {
    this.contentElement.removeEventListener('animationend', this.handleAnimationEnd)
}
AlexisWilke commented 5 years ago

@woutervanvliet Yeah but then the first else is likely ignored too when we want to know whether that first else was taken! We only want to ignore the last one.

getify commented 5 years ago

Bump.

This is annoying. I have a list of if..else if.. statements that does not need an else clause, because it's impossible to get anything else other than what's accounted for. Similar problem if I use a switch statement.

I've confirmed that if I put the /* istanbul ignore else */ above the first if, it ignores all the else if clauses, not just the final if.

My annoyingly bad choices are:

  1. use the /* istanbul ignore else */ above the first if, and lose the benefit of Istanbul tracking my coverage of all my else if statements.
  2. live with less than 100% coverage
  3. forcibly pass a wasted piece of bad data into this function to get it to hit that phantom else condition
  4. refactor the if...else if... series like this:

    if (one) { .. }
    else if (two) { .. }
    else if (three) { .. }
    else {
       /* istanbul ignore else */ 
       if (four) { .. }
    }

I picked option (4), but it's extra annoying because not only do I have to contort the code in that weirder shape, but then my linter complains about the "lonely if", so I have to put another comment to get the linter to hush. :/

alasdairhurst commented 5 years ago

Something like / instanbul ignore-final-else / would be pretty useful for stuff like this. I'd actually simplify the choices we have to the following:

a4xrbj1 commented 5 years ago

So is there any solution to this? I agree @getify that the workaround isn't working for all situations.

livingston commented 5 years ago

this works

if (condition1) {
} else /* istanbul ignore next */
if (condition) {

Didn't mean that we should use this, just because it works.

arleyyap commented 4 years ago

Can someone explain to me why the solution of @livingston should not be done?. it worked for me.

getify commented 4 years ago

@arleyyap this "suggested solution" doesn't fit the form of the problem I brought up (which differs from the OP)... consider:

if (condition1) {
  // ..
} /* istanbul ignore next */
else {
  // ..
}

vs

if (condition1) {
  // ..
} else /* istanbul ignore next */
if (whatShouldGoHere) {
  // ..
}

In the first snippet, we're trying to ignore an else... which doesn't work -- that's my complaint. In the second snippet, the ignoring is of an if that's part of an else... but what condition should go there for an else that isn't ever planned to fire?

This hack could "work", but IMO is as janky (or worse) than some other work-arounds discussed earlier in the thread:

if (condition1) {
  // ..
} else /* istanbul ignore next */
if (false) {
  // ..
}
pampananagasatish commented 4 years ago

I am also facing the same issue while running test case with only if statement. Even '/ istanbul ignore else /' statement doesn't ignore else case and results branch coverage less.

basickarl commented 4 years ago

The /* istanbul ignore else */ should only affect it's own level and not affect nested if else statements.

elliot-nelson commented 4 years ago

Definitely still a problem in nyc@14.1.1 and nyc@15.0.1. In fact, else if statements cause endless problems for both ignore if and ignore else... example:

/* istanbul ignore if: this can cover branch 1 */
if (false) {
    console.log("branch 1");
} /* istanbul ignore if: does not work */ else /* istanbul ignore if: not here either */ if (false) {
    console.log("branch 2");
} else if (false) {
    console.log("branch 3");
}

As soon as you need to ignore an if entry in a series of else if chains, you can never ignore another one. Your only option is to use an ignore next -- but that actually ignores the entire rest of the chain, including branch 3.

(I realize this is slightly different than the ignore else case in the OP, but I think it's part of the larger problem with else-if chains.)

EDIT: I guess my example could be solved by putting /* istanbul ignore next*/ inside each if block, which isn't bad, whereas there is no easy solution for the else case. So maybe my complaint is lower priority.

mgabeler-lee-6rs commented 3 years ago

Still busted, still annoying as heck ...

I guess my example could be solved by putting /* istanbul ignore next*/ inside each if block, which isn't bad, whereas there is no easy solution for the else case.

This doesn't quite work -- I tried this too, and while it gets rid of part of the issue, it still results in the else if reporting that the else case was not taken


2 cents: #230 should never have been closed because the advice given doesn't actually work.

fubar commented 3 years ago

Using ignore next right before else seems to work fine.

} /* istanbul ignore next */ else if (foo) {
getify commented 3 years ago

@fubar that was already suggested up-thread 2 years ago.

fubar commented 3 years ago

@getify the only mention or suggestion of an "ignore next" (not "ignore else") before (not after) an else statement that I can see in this thread is in your comment (https://github.com/gotwarlost/istanbul/issues/781#issuecomment-572738364), in which you said it does not work, whereas it does work for me, so I figured someone might find it helpful.

I have not tested this in chained else statements; however, chaining elses is not exactly good practice, and I'd suggest improving the OP's code by getting rid of the unnecessary else statements and using guards instead:

if (location) {
  return ...;
}
if (timezone) {
  return ...;
}
/* istanbul ignore next */
throw new Error(...);
getify commented 3 years ago

@fubar Here's the comment from 2 years ago I'm referring to: https://github.com/gotwarlost/istanbul/issues/781#issuecomment-528292165

fubar commented 3 years ago

Like I said, the "ignore next" in that comment is after the else, not before. I'm not sure if istanbul interprets it any differently though. And when I put it behind the else, prettier reformats it to be before it, so there may be another reason for it that I'm not aware of.

greim commented 1 year ago

It was counter-intuitive, but I finally managed to isolate the last else by inserting /* istanbul ignore else */ before the second-to-last step in the chain.

if (val === 'up') {
  // ...
} else /* istanbul ignore else */ if (val === 'down') {
  // ...
} else {
  // this should never happen and should
  // be ignored from branch coverage
  assertNever(val, `${val} is invalid, should be up or down`);
}

I think the problem is thinking of if/else if/else as a list data structure, whereas Istanbul sees it as a nested data structure. Making braces explicit makes it clear:

// how istanbul sees the above code...
if (val === 'up') {
  // ...
} else {
  /* istanbul ignore else */
  if (val === 'down') {
    // ...
  } else {
    // this should never happen and should
    // be ignored from branch coverage
    assertNever(val, `${val} is invalid, should be up or down`);
  }
}
fubar commented 1 year ago

@greim make it a switch (or guards if you can make it a separate function). Excessive else statements and nested ifs are code smells. In fact, your second example would be reformatted by prettier into an else if.

switch (val) {
  case 'up':
    // ...
    break;
  case 'down':
    // ...
    break;
  /* istanbul ignore next */
  default:
    throw new Error(`Invalid value: ${val}`);
}
greim commented 1 year ago

@fubar - I didn't mean to suggest my second example is how code should be written, but rather to illustrate what Istanbul sees when it parses my first example, in order to understand its behavior here. Hope that clears it up.

getify commented 1 year ago

@greim this is basically what I suggested up-thread here.

greim commented 1 year ago

@getify You suggested putting the comment immediately above the first if. Instead, try putting it in the second-to-last position like this:

if (thing) {
  // included
} else /* istanbul ignore else */ if (thing) {
  // included
} else {
  // excluded
}

As far as I can tell, nobody in this thread has suggested this, except for OP, who was apparently complaining it doesn't work? Maybe something got fixed in the interim, but it works fine for me.