Add some user flexibility for branch coverage stats #15

Closed
gotwarlost opened this Issue Oct 26, 2012 · 33 comments
@gotwarlost
Owner

@davglass @reid @mfncooper @sdesai

Starting this thread to see how we can improve branch coverage in Istanbul.

Assumption: Default processing is just about ok in the sense of generality and hitting the cases we care about. If not, let me know.

Based on conversations with some of you here are some desires:

  1. Ability to automatically exclude generally-impossible-to-test-without-a-lot-of-shennanigans branches. The big one here is if (object.hasOwnProperty('foo')) {} style branches where the else path should be considered non-existent and istanbul should probably not even consider this if statement as a branch.

  2. Ability for user to tag certain branches as unimportant/ untestable. Potential implementation is via comments just before the line of code (similar to jslint). Feel free to send thoughts on what this interface should look like.

  3. Ability to specify a reason along with the comment that is somehow shown in the HTML report

  4. Ability (for managers) to ensure that people aren't commenting their way out of their responsibilities (perhaps by having "raw" numbers shown against the "processed" numbers when different?)

Anyway, ideas welcome. Just keep this issue updated and we can circle back 3-4 weeks from now and decide what needs to be done. I'll also need to do some research on how comments are made available in the parse tree by esprima before we decide the final solution.

@davglass
Collaborator

Number 1 is great, and I love 2.

http://davglass.github.com/grover/lib/index.js.html

Lines 26 & 27 are a perfect candidate for this!

For 3 & 4, I think an * and a title for the "real numbers" is ok. I only think it needs to be called out if that number stands out against the total processed. Take my link above as an example, 2 out of 48 branches ignored shouldn't warrant a "This used a comment to bypass coverage" flag. It's well inside a margin of error, so should probably be ignored and taken as is.

@sdesai

For 1:

I would recommend we not special case hasOwnProperty - as in hardcode that into istanbul behavior if that's what you meant by "automatically".

It's a pretty wide-spread use case for lower branch numbers, which we probably don't want to test explicitly as we discussed - I'm not debating that.

I just think hardcoding it in would be making assumptions about the code, which we can't reliably [100%] do.

End users can just address that use case with solution 2 as a first step. I think the value there is that it's a conscious call being made by the owner of the code.

From there we can see if we want to expose a flag [ ala jslint ] which says "ignore hasOwnProperty" - since it is a common use case - but still make it an explicit end-user choice [ opt in or opt out - either way ].

For 3/4:

I think this is what Dav is saying too, but just a separate metric is sufficient as a first step.

I think the main thing we're trying to protect against is the 'ignore branch' feature being used too casually/excessively/egregiously (or some other fancier word) - which just the additional metric/column seems to do.

line % | statements % | branches % (after consciously ignored) | real branches %

@davglass
Collaborator

I think the comment configs are a great idea. We could potentially go the route of jshint and have enforcing options as well as relaxing options. We could potentially set the hasOwnProperty check by default and have the relaxing option to turn it off. It could even have a config file (also like JSHint) that istanbul reads from. So we could potentially add a .istanbul.json file with these configs predefined.

@gotwarlost
Owner

To me having a comment as to why the developer chose to ignore a specific branch for coverage is somewhat important since the initial reasons may not make sense when the code is refactored, for example.

Maybe it is not as important for these comments to show up in the report. We could simply allow for additional comments/ reason in the same comment block without requiring them so the maintainer of the code can have some context as to the reasons of the original developer.

@sdesai

I agree - capturing why the developer chose to ignore the branch is valuable/important for maintenance. I was just talking about the importance/priority of what to report really.

@abierbaum

Has anyone made progress on this? We had been using another tool in the past that supports marking blocks as covered and I am really missing this capability.

@gotwarlost
Owner

no, not yet.

@jussi-kalliokoski

To me another thing to add to this list is that no-op functions are currently required to be covered, e.g.

var defaults = {
  callback: function () {}
};

Currently requires you to cover the dummy function. Of course I suppose it might serve some purpose to cover a thing like this if you're out of meaningful tasks... :)

@gotwarlost
Owner

The question is: why would you write a function that is never called? I mean: you cannot assume, in the general case that this sort of thing is incorrect behavior on istanbul's part. It is always possible to provide a strawman example for a specific case and say that the coverage tool has a problem.

@gotwarlost
Owner

Finally found some time to work on this issue. Note that the code should be treated as alpha quality.

Would like to get your opinions on the interface and behavior @davglass @sdesai @jussi-kalliokoski @abierbaum @mfncooper @reid @ariya @mathiasbynens

Code is on the ignore-coverage branch.

https://github.com/gotwarlost/istanbul/tree/ignore-coverage

Interface:

  1. Just before an if statement you can say /* istanbul ignore if */ to consider the if path as covered and /* istanbul ignore else */ to consider the else path as covered.

  2. You can also say: /* istanbul ignore conditional */ just before the expressions after ? and : in a ternary operator expression or inside a logical expression (this last behavior is still flaky but should work for simple use-cases).

  3. You can add a non-word character and a comment just after these directives to document what you are doing as in: /* istanbul ignore else: windows specific code */

  4. You can see an example of each of these comments in the instrumenter file on the branch itself.
    Example: https://github.com/gotwarlost/istanbul/blob/ignore-coverage/lib/instrumenter.js#L21

Computations are done as before even in the presence of these comments but the coverage numbers are adjusted at reporting time. The HTML report shows the ignored sections as grayed out.

To quickly see what it looks like for the existing example:

$ git clone git@github.com:gotwarlost/istanbul.git
$ git checkout ignore-coverage
$ npm install
$ npm test --cover
$ open build/coverage/lcov-report/index.html # and drill down to lib/instrumenter

Feedback appreciated!

@dannybloe

Well, this is interesting but not very usefull to us. We would like to exclude entire functions because they are completely stubbed during the tests but spies and mocks.
Right now these functions are counted as uncovered. Earlier we used jscoverage and that allowed you to do something like this:

//#JSCOVERAGE_IF false
var _setQuery = function(root, query) {
xpcDataSource.setQuery(root, query);
},
.......
_fetch = function (callback) {
xpcDataSource.fetch(callback);
},
};

//#JSCOVERAGE_ENDIF

So, is there any way to do this with istanbul?

@gotwarlost
Owner

No, not yet. I'm on vacation for the next 3 weeks and will take a look when I get back. Thanks for trying it out.

@Raynos

I have a use case for applying this at a global level. Some mechanisms that are used in JS are used purely for ES3/5 code whilst waiting for ES5/6

Examples are

for (var k in o) {
  if (object.hasOwnProperty(k)) {
    var v = o[k]
    ...
  }
}

Which gets replaced with

for (var v of o) {
  ...
}

Another example is

function update(file, object, callback) {
  fs.readFile(file, function (err, content) {
    if (err) {
      return callback(err)
    }

    var newValue = extend(JSON.parse(content), object)
    fs.writeFile(file, JSON.stringify(newValue), function (err) {
      if (err) {
        return callback(err)
      }

      callback(null, newValue)
    })
  })
}

vs

function* update(file, object) {
  var content = yield fs.readFile.bind(null, file)
  var newValue = extend(JSON.parse(content), object)
  yield fs.writeFile.bind(null, file, JSON.stringify(newValue))
  return newValue
}

In both cases I want to globally say ignore ELSE case on the pattern

if (object.hasOwnProperty(k)) { and on the pattern

if (err) {
  return callback(err)
}
@sshrefler

Are there any plans or possible timeframe to merge the ignore-coverage branch?

@aventurella

Also curious about being able to add ignore's to if/else conditions etc. It's the OCD in me I'm sure =) I could have 100% but I have 90% because if assumes else even when an else need not exist.

@gotwarlost
Owner

This is by design. if (foo) bar(); is treated as 'if (foo) { bar(); } else {} - that way istanbul can track if you have a test where foo is not true.

@gotwarlost
Owner

@sshrefler - the code change on the branch is not completely baked. Need to work on it some more before it can be merged.

@jussi-kalliokoski

FWIW, I replaced the namespace patterns in the project with _.defaults to get 100% coverage :P :

_.defaults(window, {myNameSpace: {}});
_.defaults(window.myNameSpace, {myObject: {}});

Now that I've been thinking about it, patterns that inevitably cause branches are kinda bad and can be avoided, and for example adding some pre-processor instructions to work around them makes the result even more ugly so I personally no longer think that this is a very useful feature. :)

@domenkozar

Needed for code blocks like:

// run if standalone
if (require.main === module) {
  module.exports(process.argv);
}
@garbas

👍 this would be a nice thing to have in istanbul

@azproduction

👍 I need this for testing UMD modules.

(function (root, factory) {
    'use strict';
    if (typeof exports === 'object') {
        // CommonJS
        module.exports = factory();
    } else if (typeof define === 'function' && define.amd) {
        // AMD. Register as an anonymous module.
        define(factory);
    } else {
        // Browser globals
        root.module = factory();
    }
})
@ericf

+1 for ignoring UMD wrapper.

@gotwarlost
Owner

OK, I'm going to get serious in implementing this. It is almost done on the hints branch modulo some bells and whistles.

This following is the spec. Would love to get some feedback from all the people who have commented on this issue.

The interface

  1. Coverage can be explicitly skipped using comments. There is no automatic pattern match of expressions to determine if they should be skipped for coverage.
  2. A coverage skip hint looks like /* istanbul ignore <word>[non-word] [optional-docs] */
  3. For if conditions you can say /* istanbul ignore if */ or /* istanbul ignore else */ and that will end up ignoring whichever path was required to be ignored.
  4. For all other cases, the Swiss army knife /* istanbul ignore next */ may be used which skips the "next thing" in the source code
  5. The "next" thing may be, among other things:
    • A JS statement (including assignments, ifs, loops, switches, functions) in which case all of the the statement is ignored for all forms of coverage.
    • A switch case statement, in which case the particular case is ignored for branch coverage and its contents ignored for all forms
    • A conditional inside a ternary expression in which case the branch is ignored
    • A part of a logical expression in which case that part of the expression is ignored for branch coverage
  6. It is up to the caller to scope this as narrowly as possible. For example, if you have a source file that is wrapped in a function expression, adding /* istanbul ignore next */ at the top of the file will ignore the whole file!

How it works

When some part of the JS is considered skipped, nothing actually happens in terms of changes to the instrumentation. Everything is calculated as though nothing was skipped - all that changes is that there is a skip attribute added to the metadata of the statement, function or branch as applicable.

Coverage reporting however takes the skip attribute into account and artificially increments counts, when 0 and skipped to pretend that the thing in question was covered. The HTML report shows the coverage after taking skips into account but at the same time colors the skipped statements with a gray color for easy visual scan.

This design makes it possible to report on either of the coverage numbers ("raw" v/s "processed"), show a count of statements/ functions/ branches skipped etc. Some of this stuff still needs to be developed.

Examples

The first thing I did was of course to annotate the istanbul source code for hard-to-test cases, hasOwnProperty checks, precondition asserts etc.

See commit 3faf816 for details.

@gotwarlost gotwarlost closed this Dec 18, 2013
@gotwarlost gotwarlost reopened this Dec 18, 2013
@gotwarlost
Owner

Sample HTML report:

screen shot 2013-12-17 at 9 35 45 pm

@vetik32 vetik32 referenced this issue in karma-runner/karma-coverage Dec 25, 2013
Closed

chore(config): upgrade istanbul #48

@mrzepinski mrzepinski referenced this issue in karma-runner/karma-coverage Jan 10, 2014
Closed

Update package.json #51

@vojtajina vojtajina added a commit to karma-runner/karma-coverage that referenced this issue Jan 21, 2014
@mrzepinski mrzepinski feat: update istanbul b696c3e
@FlorianLoch

Are there any future plans to implement a (cli) switch to ignore all missing else-branches?

@gotwarlost
Owner

No. Why do you need this?

@anobika

I have code that has multiple if statements which have no corresponding else statements. I have been adding /* istanbul ignore else */ individually for all these if statements in order to ignore the corresponding missing else statements. Is there are way to ignore all the missing else statements with a single comment?

@FlorianLoch

The case @anobika mentioned would be a use case for this feature.
I am needing such a feature because I am instrumenting huge amounts of code I do not control and adding these "ignore-comments" would need me to add another "parse+walk AST+generate code" step in front of using great Istanbul for instrumentation.

@pdrozdowski

Hi, marking with comment parts of code to ignore in test coverage would be super useful. In my scenario i'm having code 100% in typescript and so there are bits of code added from compiler which will not get into cover - for instance there is an if in --extend method added each time you are using inheritance in code and there is no way to test that. regards.

@ashishdasnurkar

+1 for adding a a cli switch to ignore all "missing" else branches

@deckar01

@FlorianLoch @anobika @ashishdasnurkar Is there a reason you are not testing to make sure the code inside the if block will not be called when the condition is false?

function foo(bar) {
  if(bar) {
    baz();
  }
}

should have 2 tests:

var bar = true;
foo(bar);
expect(baz).toHaveBeenCalled();
var bar = false;
foo(bar);
expect(baz).not.toHaveBeenCalled();

to protect against future changes like

function foo(bar) {
  baz();
}
@FlorianLoch

@deckar01 Perhaps I got you wrong but as I mentioned I do noz control the code that shall be instrumented so I see no way to pick up your suggestion without the former mentioned parse-walkAST-generate step which I definitely want to avoid due to various reasons.

@deckar01

@FlorianLoch I would recommend something like https://github.com/douglasduteil/isparta if the code is generated. Completely ignoring missing else branches has a very good chance of giving you false coverage reports.

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