Missing "else" changes branch percentage

[mbielski]

When a script contains an "if" without an "else" the file is flagged for not having covered the "else" that isn't there. This changes the total coverage of the branches incorrectly when the contents of the "if" are fully covered. The lone "if" should be detected correctly.

[gotwarlost]

This is a feature. It tells you that you don't have a test case where the "empty else block that notionally exists" is not taken.

[mbielski]

I disagree. This is not a "feature."

In order to "test" the "empty else block that notionally exists" I actually have to add something to the script that does nothing. That doesn't lint well, and it is just plain poor programming practice. Can you at least provide a configuration option to turn this off?

[gotwarlost]

What do you mean? Consider this code snippet:

if (foo) { doSomething(); }
doSomethingElse();

When you have a single test where foo is true, both functions are being executed. So you hitting every line of code but you are not testing the case where foo is false and doSomething() is never called. And this is what istanbul points out in its branch analysis.

To make branch coverage 100% you need to add a test where foo is false. You don't have to change the code in any way.

Why is this wrong?

BTW, there is not going to be a config option to turn off what I consider the bread-and-butter use-case of branch coverage. Without this feature, branch coverage doesn't give you any extra information that statement coverage does not already give you.

[mbielski]

Your summation about covering every line of code is not correct. To cover every line of the code you provided, I need only check to see that doSomething() is called when foo is true, and then a check to see if doSomethingElse() is called regardless of the value of foo. You didn't provide an else statement, so doSomethingElse() is called regardless of the value of foo. What you've described is:

if (foo) { doSomething(); } else { doSomethingElse(); }

or it can be written as:

if (foo) {
doSomething();
} else {
doSomethingElse();
}

Whereas the code you provided as a sample can also be written as:

if (foo) {
doSomething();
}
doSomethingElse();

In this second case, the tool will tell you that you didn't cover the else statement when obviously there isn't one.

Your code isn't wrong, but your personal coding style isn't what everyone else uses. Suppose that the code inside of the if is longer, say 17 lines. Would you run that all into one line? I wouldn't, nor would I litter my code with additional functions just to make a single-line if statement.

I'm actually looking for the tool to parse the code correctly, which it isn't doing at this point. The else statement isn't required when using an if (see the ECMA 262 specification here: http://www.ecma-international.org/ecma-262/5.1/#sec-12.5) and I don't believe that the tool following the specification correctly. It should not presume that there is an else statement. It should check for it and report correctly based upon the presence. If it exists and I didn't cover the code within, then tell me that. If it doesn't exist, it shouldn't nag me about writing a test to cover code that doesn't exist and falsely report that that branch of code isn't covered when it doesn't even exist.

I should also say that I am using this tool as a result of it being included in Testacular, which is the recommended test platform for AngularJS. I like the tool and the reporting it provides, but this one thing strikes me as the tool not functioning correctly. Please re-consider making this change.

[gotwarlost]

istanbul doesn't care about any coding style - it's job is to tell you what is covered and what is not. It will produce the same results for the same code and tests irrespective of coding style - so let's get that out of the way.

Maybe I'm doing a horrible job of explaining how this works so I'll point you to a blog post by @ariya on what the problem is (and branch coverage in istanbul is a solution to this problem).

IIRC, this post was written before istanbul was open-sourced.

http://ariya.ofilabs.com/2012/09/the-hidden-trap-of-code-coverage.html

And here is another post that explains why the way branch coverage works in istanbul is a good feature:

http://ariya.ofilabs.com/2012/12/javascript-code-coverage-with-istanbul.html

[mbielski]

Ok, now let me give you an example of why istanbul is not working correctly. Let's say that we want to display temperatures in celsius if and only if our visitor is from a non-US country, and we have a function that does the checking for us, like so:

var foo, ab;

ab = "32 Degrees";

foo = visitorIsNotUSBased();

if (foo) {
ab = convertFarenheightToCelsius(ab);
}

$('#waterfreezes').val(ab);

In the example above there is no else because we don't want to do anything to ab if foo is not true. Running this code through istanbul with the appropriate tests, it will nag at me for an else that not only does not exist but does not have to or need to exist. Why is that correct?

[ariya]

In your tests, you need to provide two scenarios (through mocks, if necessary), US and non-US. Istanbul then will mark both the branches (including the implicit, empty else) as covered.

[mbielski]

So, you are both advocating that I write a test for code that isn't there and then that will make things right? Seriously?

[gotwarlost]

I don't think I'm going to be able to convince you so here's my last attempt.

Forget how istanbul instruments the code, empty else blocks and the like. In the example above, you will get a branch coverage of 50%. The high-level information you can get out of it is that you have a test case for non-US based visitors but you don't have a test case for US visitors. That is, you are testing that temperature conversions are happening for non-US visitors but you are not testing that temperatures are left unconverted for US visitors

You can look at the code and say: Of course, the US visitor case will work and that's cool. The point is you haven't tested that it does, in fact, work.

The job of the coverage tool is to identify gaps and report accurate numbers. What you choose to do with that information is entirely up to you. In short, there will not be a flag to turn off this behavior globally.

Also, see the discussion in: #15 for cases where it is helpful to disable branch coverage. But this use case is not one of them.

[mbielski]

Actually, I think you have. Your third paragraph is actually the tipping point here, and you're right, it hasn't been tested. The reason it hasn't been tested is an assumption of functionality, right? That would probably be a bad thing.

I do see your point, and while I may not like it, it is correct and I stand corrected. Thank you for your efforts to educate me. I'm always willing to learn, it's just hard to get a new concept into my head when it changes what I have been doing and thinking it was right all the while.

[gotwarlost]

Thanks for listening. I now understand why my colleague @davglass likes to say "If you think JS Lint hurts your feelings, wait until you use istanbul".

I swear, I never set out to make a tool to hurt people's feelings :)

[ariya]

It's also important to keep in mind that, due to the dynamic nature of the language, static analyzer like Istanbul can't full predict the impact of various if/else branch. A generalized but similar construct like

if (X) doA();
doB();

does not tell any side-effect of the invocation of doA. Imagine you need jQuery within doB function and you set it inside doA function. Without testing the else branch (which Istanbul happily points out as missing), that problem won't be exposed.

Obviously this is a contrived example. However, I've seen pretty similar cases in real-world code, in particular because functions tend to be more than just one-liner and suddenly our brain can't keep up with the tracking of what is being done where. Even if we can do that at this moment, that does not mean that in the future nobody can get away with it again. Eventually, an intern or a junior engineer gets the task of improving the code, adds some stuff here and there, and then suddenly the interplay between those branches become more important than ever.

[mbielski]

chuckle no hurt feelings, just re-aligned thinking.

[tomasdev]

how do you test the 'else branch' for the following?

for (var property in defaultValues) {
 if (defaultValues.hasOwnProperty(property)) {
  $window.s[property] = defaultValues[property];
 }
}

[tomasdev]

Solution:

var parent = { key: 'value' };
var child = Object.create(parent);
child.key2 = 'value2';
// ...
expect($window.s.key).toBeUndefined();
expect($window.s.key2).toBe('value2');

[gotwarlost]

@tomasdev - these are the hard-to-test cases where you can use /* istanbul ignore else */ to signal that the else path is hard to test or untestable.

[jt3k]

why istanbul show warning only on first IF (on line 34) but ignore second IF (on line 41)

[robsonbittencourt]

@jt3k Probably you have not tested the 4 possibilities of your if.

[jt3k]

@robsonbittencourt why did you write this text? can't you see my question?

[robsonbittencourt]

Yes it can. I replied because I think the warning refers to you don't have covered the four possibilities of first if. Already the second if must have had his two possibilities covered.

[iturgeon]

If I can contribute to this at all, I think the error could use a more explicit description.

We just started using istanbul and didn't understand the output. Initially the developer assumed the test wasn't getting into the if block (because it indicated it wasn't covered). That led to some crazy misunderstandings of how istanbul was evaluating code.

After reading the error completely, the developer realized it was complaining about an else not being run, but when we looked at the source, there was no else statement at all. The developer was pretty confused, a lot of random things seemed to be occurring.

At this point, the first assumption was that for some reason istanbul required if statements to have else statements, which seemed crazy. Eventually we uncovered this github issue and figured out what is really happening.

After seeing this happen, it seems reasonable that it would happen to others. I believe making the user aware that this coverage test requires if statements to eval as true and false will help save some headaches.

[lifeich1]

var isEmptys = function() {
    var obj = this;
    for (key in obj) {
        E if (typeof(obj[key]) !== 'function' && obj[key]) {
            return false;
        }
    }
    return true;
}

The disturbing "else path not taken" occurs when I test the code in all cases:

            (isEmptys.call({})).should.be.true();
            (isEmptys.call({9:1})).should.be.false()

THAT MAKES ME MAD!!!

[lifeich1]

I found I made a mistake. There is an another testcase:

            (isEmptys.call({1:function() {}})).should.be.true();

I am sorry for ignoring that case.

[andreyluiz]

Just contributing with a practical example of how to get rid of this problem. I have the following:

async updateTravelInfo(newLocation) {
  if (!isEqual(newLocation, this.props.location)) {
    // update location accordingly
  }
  // Nothing to do
}

In other words, the update location stuff only needs to be called if the location changes. To test this, I have the following:

test("fetches travel info when location changes", () => {
  const travelTime = jest.spyOn(baseProps.locationsService, "getTravelInfo");

  travelTime.mockResolvedValue({ time: "100 anos", from: "Madrid" });

  const wrapper = shallowWithContext(
    <Home {...baseProps} />,
  );

  wrapper.setProps({
    location: {
      location: {
        latitude: 12,
        longitude: 21,
      },
    },
  });

  // FUNCTION called the first time (if taken)
  expect(travelTime)
    .toHaveBeenCalledWith({
      lat: 12,
      lng: 21,
    });

  // Set the same props as before
  wrapper.setProps({
    location: {
      location: {
        latitude: 12,
        longitude: 21,
      },
    },
  });

  // The function should have been called just once!
  // The error disappeared from coverage report.
  expect(travelTime)
    .toHaveBeenCalledTimes(1);
});

With this I have no uncovered branches.

I am not testing coding that does nothing. I am testing if the code will do something in the hidden else path. In this case, nothing should be done.

[shaycraft]

Just my 2 cents: requiring every if statement to have a corresponding else statement has nothing to to do with code coverage, it's a linting rule (and a stupid one at that).

[justinpincar]

So I understand your philosophy now that I've read through this thread, but you have to admit this is hella confusing if you're just looking at the reports. 100% of statements, 100% of functions, and 100% of lines covered, with line 12 uncovered. Mmmk.

File                 |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
---------------------|----------|----------|----------|----------|-------------------|
  logger.js          |      100 |       75 |      100 |      100 |                12 |

Instanbul is a great tool; it would be nice if it supported more use cases.

[vkiperman]

I disagree. This is not a "feature."

In order to "test" the "empty else block that notionally exists" I actually have to add something to the script that does nothing. That doesn't lint well, and it is just plain poor programming practice. Can you at least provide a configuration option to turn this off?

It simply means you have to provide a spec that tests the else case. There's no need to add useless code.

[vkiperman]

Ok, now let me give you an example of why istanbul is not working correctly. Let's say that we want to display temperatures in celsius if and only if our visitor is from a non-US country, and we have a function that does the checking for us, like so:

var foo, ab;

ab = "32 Degrees";

foo = visitorIsNotUSBased();

if (foo) {
ab = convertFarenheightToCelsius(ab);
}

$('#waterfreezes').val(ab);

In the example above there is no else because we don't want to do anything to ab if foo is not true. Running this code through istanbul with the appropriate tests, it will nag at me for an else that not only does not exist but does not have to or need to exist. Why is that correct?

in this example, ab can have one of two possible values. But, you are only testing it for one of the values. Technically, your coverage is incomplete.
What happens when you call expect($.val).toHaveBeenCalledWith(?), you should cover all the ways the val can be called.

[vkiperman]

My question is, if there is nothing at all after the if statement, where the if statement is the only thing inside a function.

public set initialData(value: string) {
      if (value) {
        this.form.get('selection').setValue(value);
      }
   }

What's the advantage of mocking the else case and having that coverage?

[kammanageswararao]

HI Can you please assist the below test cases in angular?

Here i have written one test case

it('productSelection', async(() => {
spyOn(component, 'productSelection');
const btn = fixture.debugElement.query(By.css('.BPNormal'));
btn.triggerEventHandler('click', 'productSelection');
fixture.whenStable().then(() => {
expect(component.productSelection).toHaveBeenCalled();
});
}));

still in code coverage showing conditional 50 % (1/2).
Could you please please provide me the expecting test cases example.

Thanks in advance.

[joelgetaction]

I just encountered this. I still don't understand why this is considered a "feature". Take for example the following:

if (typeof x !== "number") {
  ...
} else if (x > 2) {
  ...
} else if (x <= 2) {
 ...
}

Why mark that as else path not taken - it is logically impossible for the else path to be taken, provably so. Why should I get less than 100% coverage here when under no system of logic could the else path ever be taken let alone need to be coded let alone need to be tested?

I don't understand ... :-(

[vkiperman]

The last else if in the above code can be else. That should solve the problem.

Your code could also be rewritten in this way:

if (typeof x !== "number") {
    ...
    return
} else if (x > 2) {
    ...
    return
}
...

This way all the cases can be covered. It's my opinion that the else path coverage rules are designed to force us to write if-else statements this way. I'm still trying to understand the coverage rules.

[mgartner]

@joelgetaction I think there is actually an implicit else that is possible, even though Istanbul isn't "smart" enough to know whether or not that is the case (I think).

$ node
> x = NaN
NaN
> typeof x
'number'
> x > 2
false
> x <= 2
false

[hasezoey]

I dont really understand why this is a feature and has no config options, because of the following example
(context: this is inside an function that assembles an mongoose Schema)

// interface BaseVirtual {
//   options?: {
//     [key: string]: any;
//     overwrite: boolean;
//   };
//   get?(): any;
//   set?(toSet: any): any;
// }
// getterSetters is of type Map<string, BaseVirtual>
// sch is of type mongoose.Schema<any>
for (const [key, virtual] of getterSetters) {
  if (virtual.options && virtual.options.overwrite) {
    sch.virtual(key, virtual.options);
  } else {
    if (virtual.get) { // else path not taken
      sch.virtual(key, virtual.options).get(virtual.get);
    }

    if (virtual.set) { // else path not taken
      sch.virtual(key, virtual.options).set(virtual.set);
    }
  }
}

why is this marked as "else path not taken" when there is simply nothing todo in an else path (because sch.virtual(key, virtual.options).set(undefined); or "null" would not be accepted by the function)

[vctormb]

My workaround is doing like this:

function handleChange() {
  if (!props.onChange) return;

  props.onChange('value');
}

And then I test it without the onChange prop.

[Jimmydalecleveland]

I was pretty annoyed with this error and the debate in the thread until I read this comment #35 (comment) from @andreyluiz .

Since most linters I've used do not want you to have unnecessary return statements, doing something like:

if (isTrue) {
  doSomething()
}
return

is not feasible. I thought that was what was being advised until the previous comment, when I realized I'm just supposed to actually test the event in which the if statement does not pass (e.g. isTrue === false). This makes a lot more sense to me, and I find the rule very helpful so thank you to everyone who debated about this.

[rajeshrasch]

Change something with only if statement to
onKeyDown={event => { if (this.state.shouldClose && event.keyCode === ECS_KEY) { this.setState({ toShow: false, }); } }}
To this with shortand
onKeyDown={event => { (this.state.shouldClose && event.keyCode === ECS_KEY) && this.setState({ toShow: false, }) }}

This works with ES6 transpile

[MrBrN197]

I've read through this entire discussion trying to understand why this makes sense and why my branch coverage wasn't 100% regardless of how many tests I wrote. like @Jimmydalecleveland said about the former comment; it really is just about explicitly testing the "branches" in your code. even if it does nothing, explicitly testing that... is necessary.

[Serzhs]

Consider this as a jest bug and add /* istanbul ignore else */ on top of your if