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

Fix false-positive coverage of if statements (REDUCES REPORTED COVERAGE!) #878

Merged
merged 3 commits into from Nov 7, 2018

Conversation

@rluba
Copy link
Contributor

rluba commented Nov 2, 2018

Lab currently reports the last part of a conditional within an if statement as covered even though one case might not have been covered!

For example, the following if statement is reported as fully covered, even if value2 is always false:

if (value1 || value2) {
…
}

This PR causes the coverage of Lab itself to drop because it exposes several uncovered conditionals within Lab that have previously been incorrectly reported as covered.

@rluba rluba changed the title Fix false-positive coverage of if statements Fix false-positive coverage of if statements (REDUCES REPORTED COVERAGE!) Nov 2, 2018
@geek geek self-assigned this Nov 2, 2018
@geek geek added this to the 18.0.0 milestone Nov 2, 2018
@geek

This comment has been minimized.

Copy link
Member

geek commented Nov 6, 2018

@rluba I'm working through the missing tests and will PR into your PR so that the merge doesn't break master.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 7, 2018

When I did this, I ran out of time to deal with the difference between if (x || y) {} and const z = x || y. In the assignment case, you don't want to test if y is always true or false because it doesn't matter.

@geek go ahead and break master... it's free.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Nov 7, 2018

In the assignment case, you don't want to test if y is always true or false because it doesn't matter.

@hueniverse Exactly. That’s why I left it as it is for the assignment case. But ignoring the last boolean in an if clause caused us to miss some serious coverage issues.

@geek geek merged commit db630d2 into hapijs:master Nov 7, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Node Security No known vulnerabilities found
Details
@Samueljoli Samueljoli mentioned this pull request May 16, 2019
6 of 9 tasks complete
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.