Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ignoring lines #27

Open
phihag opened this issue Jul 19, 2012 · 7 comments
Open

Allow ignoring lines #27

phihag opened this issue Jul 19, 2012 · 7 comments
Assignees

Comments

@phihag
Copy link

phihag commented Jul 19, 2012

Sometimes, I'm aware that a certain line is not going to be covered (say, because it's related to another platform. For example, when I run my test under node.js, browser-specific branches are not going to be touched).

There should be an option to ignore those lines (so that I can still automatically require 100% coverage). It could look like this:

if (!require) {
    XMLSerializer = window.XMLSerializer; // pragma: ignore
}

In the HTML output, the ignored lines could be marked with a light yellow hint.

@itay
Copy link
Owner

itay commented Jul 19, 2012

That'd be great. One issue is that we're dependent on Esprima to do the parsing, but if I can somehow get the comment attached that parse node, I can probably do it. There have been some new things with Esprima recently.

@ghost ghost assigned itay Jul 20, 2012
@floriancargoet
Copy link
Contributor

I'm working on that.
I've some questions if you're still interested in that feature:

  • syntax of the comment ( // cover:false ?)
  • should it only ignore one line or the whole sub-tree?

Example:
var a = function(){ // cover:false
//...
};

I think it should ignore the whole function, not just the variable assignment.

It's a small change (~ 15 LOC in instrument.js) + an esprima.js update.

@phihag
Copy link
Author

phihag commented Dec 4, 2012

Excellent.

I don't really care about the syntax. pragma:ignore is apparently used by some exotic C# validators, and Python's nosetest uses pragma: no cover. cover: false is excellent and allows for extensions such as cover: block-false.

Ignoring the subtree would be nice. However, that'd make code like

{foo;}; bar = function() { // cover: false
  baz;
}.blorg();
if (x) { // cover:false
  blarg;
};
blurg;

ambiguous, wouldn't it? At least I wouldn't be sure whether that included foo, the assignment to bar, baz, or blorg. And what exactly does this block-bases solution apply to, any block with braces or just new scopes (i.e. functions)?

So, personally, I'd be happy with the line-based solution. However, I imagine a lot of other users would love to ignore whole blocks, so why not implement both?

@floriancargoet
Copy link
Contributor

  • Being on an ignored line, foo would be ignored.
  • Being on an ignored line, the assignment to bar would be ignored.
  • Being inside a syntax tree started on an ignored line, baz would be ignored.
  • .blorg() belongs to a statement started on an ignored line, it would be ignored.

The "block-based solution" is in fact a node-based solution. And by node, I mean a node from the syntax tree generated by esprima.

When the code encounters a "cover:false", I can say:

  • ignore this node
  • ignore this node and its descendants

@itay what do you think?

P.S.: code's coming, I'm preparing a pull request.

@phihag
Copy link
Author

phihag commented Dec 4, 2012

@floriancargoet Great, that works. Thank you very much for implementing it!

@itay
Copy link
Owner

itay commented Dec 4, 2012

@floriancargoet this is great, I would be happy to merge it in. I'd also be happy to give you commit access if there are other improvements you'd like to do. I would love to have another contributor to help get things going :)

@floriancargoet
Copy link
Contributor

Here's the pull request. Hope you like it.
About the commit access, let's first see if I come up with other fixes/features ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants