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

Document coverage ignore syntax, fixes #438 #441

Merged
merged 1 commit into from Sep 15, 2015

Conversation

@skeggse
Copy link
Contributor

skeggse commented Sep 14, 2015

It's a pretty long example, but I wanted it to be believable. Comments?

@geek geek added this to the 5.16.2 milestone Sep 14, 2015
@geek geek self-assigned this Sep 14, 2015
README.md Outdated
@@ -226,6 +226,46 @@ module.exports = [
];
```

It is difficult to reach 100% code coverage. Sometimes you just want to turn it off, like when you have an assertion for logical errors:

This comment has been minimized.

Copy link
@geek

geek Sep 14, 2015

Member

Mind removing the part about being difficult, maybe rephrase to something like:

"Sometimes you want to disable code coverage for specific lines, below is an example demonstrating this:"

This comment has been minimized.

Copy link
@skeggse

skeggse Sep 14, 2015

Author Contributor

Can do. Will change in about an hour.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Sep 14, 2015

@geek there ya go

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Sep 14, 2015

I'm not sure this is a good example. If a code may throw an error, then it should definitely have a test case.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Sep 14, 2015

The thrown error is a logical error - it's basically an assertion. It's like having the following:

var fn = function () {

    var value = 7;
    assert(value !== 6);
};

The assert could theoretically throw an error. I mean, it is a function, and it has a statement that throws an error. However, in no conceivable case would the error actually be thrown, and there aren't any tests you can write that would cause the assertion to fail.

My example is a little more involved, but it boils down to the same thing: you cannot write a test to make the variable anything but 0 and 1, because the only two changes to the variable are state = 0 and state = 1. There's no state = option or anything.

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Sep 14, 2015

I get the code part. What I'm saying is that users reading the documentation are only interested in the syntax - how to disable coverage for a certain part of the code. Most of them won't spend time on understanding the whole code in your example (it's 33 lines long), they will just see this part:

// $lab:coverage:off$
default:
    throw new Error('State machine violation');
// $lab:coverage:on$

So what they will think is that coverage can be ignored for thrown errors in the code. Which I think is a bad message, because exceptions affect the flow of the code, so they should have a test case.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Sep 14, 2015

Hmm good point.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Sep 14, 2015

That look better?

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Sep 14, 2015

Yes, I think a simpler example is better. I actually like the one which is already in the code of lab (https://github.com/hapijs/lab/blob/master/lib/reporters/console.js#L99-L103):

/* $lab:coverage:off$ */ // There is no way to cover that in node 0.10
if (typeof value === 'symbol') {
    return '[' + value.toString() + ']';
}
/* $lab:coverage:on$ */

It is clear why coverage is ignored. But I'm fine with your example too. It was only the previous one that I didn't like.

Signed-off-by: Eli Skeggs <skeggse@gmail.com>
geek added a commit that referenced this pull request Sep 15, 2015
Document coverage ignore syntax, fixes #438
@geek geek merged commit 1bb1a2e into hapijs:master Sep 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.