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

Implement coverage bypass stack #926

Merged
merged 2 commits into from Jun 14, 2019
Merged

Implement coverage bypass stack #926

merged 2 commits into from Jun 14, 2019

Conversation

@skeggse
Copy link
Contributor

skeggse commented Jun 9, 2019

Fixes #904.

let newSkipState;
if (command === 'pop') {
if (!skipStack.length) {
throw new Error('unable to pop coverage bypass stack');

This comment has been minimized.

Copy link
@skeggse

skeggse Jun 9, 2019

Author Contributor

There are a number of other reasonable behaviors here, such as just resetting to the default (on), or doing nothing. I lean towards this behavior because it seems more helpful if you mis-implement machine generated stack manipulation comments, though in that case it may be prudent to also warn if the stack isn't empty by the end of the file. Thoughts?

segmentSkip = skip;
if (skip) {
skipStart = comment.range[1];
const command = directive[1];

This comment has been minimized.

Copy link
@skeggse

skeggse Jun 9, 2019

Author Contributor

This whole block ends up being a bit of a gross deeply nested block. I could split bits out into functions or reorganize using switch or something; let me know if that's important - this seemed sufficient.

skipStart = comment.range[1];
}
else {
for (let i = skipStart; i < comment.range[0]; ++i) {

This comment has been minimized.

Copy link
@skeggse

skeggse Jun 9, 2019

Author Contributor

it's just occurring to me that a $lab:coverage:off$ comment without a subsequent on comment might result in no bypass - is this intended behavior, or just a property of how this was originally structured?

@@ -0,0 +1,37 @@
'use strict';

This comment has been minimized.

Copy link
@skeggse

skeggse Jun 9, 2019

Author Contributor

I was going to have bypass-stack.js also have this functionality and let getIsSet be parameterized to the two utilModule copies, but I didn't immediately see how to reset the coverage state of the module.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Jun 9, 2019

Travis failures seem windows-related and unrelated to proposed changes.

@geek

This comment has been minimized.

Copy link
Member

geek commented Jun 10, 2019

@skeggse nice work! Do you think it is worth explaining how this works in the readme?

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Jun 12, 2019

@skeggse nice work! Do you think it is worth explaining how this works in the readme?

Ah yeah that seems reasonable. I'll write some docs soon.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Jun 14, 2019

Added some documentation. Does that align with your expectations?

@geek geek added this to the 19.1.0 milestone Jun 14, 2019
@geek
geek approved these changes Jun 14, 2019
@geek

This comment has been minimized.

Copy link
Member

geek commented Jun 14, 2019

@skeggse looks good, going to be published at @hapi/lab as 19.1.0

@geek geek merged commit 42636bd into hapijs:master Jun 14, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@geek geek assigned skeggse and unassigned hueniverse Jun 14, 2019
@skeggse

This comment has been minimized.

Copy link
Contributor Author

skeggse commented Jun 22, 2019

Heh I'm just noticing that I wrote the original documentation for the coverage directives in #441

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.