Skip to content

Conversation

@skeggse
Copy link
Contributor

@skeggse skeggse commented Sep 14, 2015

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

@skeggse skeggse added the documentation Non-code related changes label Sep 14, 2015
@geek geek added this to the 5.16.2 milestone Sep 14, 2015
@geek geek self-assigned this Sep 14, 2015
README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. Will change in about an hour.

@skeggse
Copy link
Contributor Author

skeggse commented Sep 14, 2015

@geek there ya go

@gergoerdosi
Copy link
Contributor

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
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
Copy link
Contributor

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
Copy link
Contributor Author

skeggse commented Sep 14, 2015

Hmm good point.

@skeggse
Copy link
Contributor Author

skeggse commented Sep 14, 2015

That look better?

@gergoerdosi
Copy link
Contributor

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.

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
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation Non-code related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants