Comments (33)
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:
- Just before an
if
statement you can say/* istanbul ignore if */
to consider theif
path as covered and/* istanbul ignore else */
to consider theelse
path as covered. - 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). - 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 */
- 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 [email protected]: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!
from istanbul.
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.
from istanbul.
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 %
from istanbul.
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.
from istanbul.
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.
from istanbul.
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.
from istanbul.
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.
from istanbul.
no, not yet.
from istanbul.
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... :)
from istanbul.
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.
from istanbul.
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?
from istanbul.
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.
from istanbul.
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)
}
from istanbul.
Are there any plans or possible timeframe to merge the ignore-coverage branch?
from istanbul.
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.
from istanbul.
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.
from istanbul.
@sshrefler - the code change on the branch is not completely baked. Need to work on it some more before it can be merged.
from istanbul.
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. :)
from istanbul.
Needed for code blocks like:
// run if standalone
if (require.main === module) {
module.exports(process.argv);
}
from istanbul.
👍 this would be a nice thing to have in istanbul
from istanbul.
👍 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();
}
})
from istanbul.
+1 for ignoring UMD wrapper.
from istanbul.
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
- Coverage can be explicitly skipped using comments. There is no automatic pattern match of expressions to determine if they should be skipped for coverage.
- A coverage skip hint looks like
/* istanbul ignore <word>[non-word] [optional-docs] */
- 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. - For all other cases, the Swiss army knife
/* istanbul ignore next */
may be used which skips the "next thing" in the source code - 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
- 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.
from istanbul.
Sample HTML report:
from istanbul.
Are there any future plans to implement a (cli) switch to ignore all missing else-branches?
from istanbul.
No. Why do you need this?
from istanbul.
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?
from istanbul.
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.
from istanbul.
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.
from istanbul.
+1 for adding a a cli switch to ignore all "missing" else branches
from istanbul.
@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();
}
from istanbul.
@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.
from istanbul.
@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.
from istanbul.
Related Issues (20)
- Looking for Constantinople.js HOT 1
- is it possible to apply the effect from /* istanbul ignore next */ to all typescript private methods automagically?
- option to disable coverage on import lines
- Open Coverage File Automatically on Test Command? HOT 2
- Branches that don't exist get reported as missing
- how to use istanbul as a library
- Analyzing coverage of node server in Windows HOT 1
- exclude imported node modules from coverage reporting HOT 1
- Cannot get the backend coverage by running Cypress UI e2e test HOT 1
- Ignore implied "else"
- Coverage format documentation not explicitly stating 'end' meaning HOT 1
- Istanbul doesn't ignore nested if, when not executed
- Async package is vulnerable
- Coverage not being collected from file called "payload.ts"
- Fails when running `istanbul report` HOT 1
- Unit test branch for SignInPage showing the SignUpComponent
- Ignore docs could be clarified by adding a list of allowed keywords
- Code comments are being covered.
- Feature Request - Parcel plugin
- No function coverage for named exports in barrel file
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from istanbul.