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

Code coverage: Always surround expressions in ternary operator with parenthesis #556

Merged
merged 3 commits into from Apr 5, 2016

Conversation

@HCanber
Copy link
Contributor

HCanber commented Mar 31, 2016

Fixes #555 Code coverage for conditional operator with comma operator isn't handled correctly

HCanber added 2 commits Mar 30, 2016
i.e. this is not producing expected result:
true ? ('no', 'yes') : 'nope'

Should produce 'yes' but produces 'no' when the code gets annotated

Demonstrates issue #555
when annotating for code coverage.

Fixes #555


exports.method = function (value) {
return true ? (value-1, value) : value+1; //Should return value

This comment has been minimized.

Copy link
@geek

geek Mar 31, 2016

Member

please space the operators

return true ? (value - 1, value) : value + 1;

This comment has been minimized.

Copy link
@HCanber

HCanber Apr 1, 2016

Author Contributor

New commit pushed which fixes this.

@@ -175,6 +175,14 @@ describe('Coverage', () => {
done();
});

it('retains original value of conditional result with comma operator', (done) => {

This comment has been minimized.

Copy link
@geek

geek Mar 31, 2016

Member

You will want to enable coverage on a test that uses the condition2 file to make sure that coverage works and the method executes correctly

This comment has been minimized.

Copy link
@HCanber

HCanber Apr 1, 2016

Author Contributor

Not sure what this means. I copied the test above, and just modified it. What do I need to do differently?
Note, the purpose of this PR is not to add new or missing functionality. If coverage of conditional operator and comma operator was wrong before, it will continue to be wrong after this. This PR just to fix a bug which causes errors (in my case tests starts to fail) when lab is run with coverage switched on.

@geek geek added the bug label Apr 5, 2016
@geek geek added this to the 10.3.1 milestone Apr 5, 2016
@geek geek self-assigned this Apr 5, 2016
@geek geek merged commit 99273e2 into hapijs:master Apr 5, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Apr 5, 2016

@HCanber thanks for fixing this issue, its now published in lab 10.3.1

@HCanber HCanber deleted the HCanber:fix-555 branch Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.