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 for conditional operator with comma operator isn't handled correctly #555

Closed
HCanber opened this issue Mar 30, 2016 · 2 comments
Assignees
Labels
bug
Milestone

Comments

@HCanber
Copy link
Contributor

@HCanber HCanber commented Mar 30, 2016

Version: Lab 10.3.0 with code coverage turned on, -c

Problem

This expression is not correctly annotated so when the code is run it produces invalid result

true ? ('no', 'yes') : 'nope'

Expected value: 'yes'
Actual value: 'no'

Why

The reason is it's being annotated like this:

(global.__$$labCov._statement('/Users/2921/code/_3rd/lab/test/coverage/conditional2.js',1,1,true)
  ? global.__$$labCov._statement('/Users/2921/code/_3rd/lab/test/coverage/conditional2.js',2,1,'no', 'yes')
  : global.__$$labCov._statement('/Users/2921/code/_3rd/lab/test/coverage/conditional2.js',3,1,'nope'))

This excerpt shows that consequent expression is missing it's parentheses:

global.__$$labCov._statement('file.js',2,1,'no', 'yes')

It should have been annotated as:

global.__$$labCov._statement('file.js',2,1,('no', 'yes'))

The consequent code is rendered using node.consequent.source() on

node.set('(' + node.test.source() + '? global.__$$labCov._statement(\'' + filename + '\',' + consequent + ',' + line + ',' + node.consequent.source() + ') : global.__$$labCov._statement(\'' + filename + '\',' + alternate + ',' + line + ',' + node.alternate.source() + '))');

node.set('(' + node.test.source() + '? global.__$$labCov._statement(\'' + filename + '\',' + consequent + ',' + line + ',' + node.consequent.source() + ') : global.__$$labCov._statement(\'' + filename + '\',' + alternate + ',' + line + ',' + node.alternate.source() + '))');

The problem with missing parentheses comes from acorn (used by espree) that sets start and end position for SequenceExpression to not include parentheses.

Solution

I see three possible solutions:

  1. Test if consequent node is a SequenceExpression. If it is, add parentheses around it. Do the same for alternate node.

  2. Always surround consequent and alternate nodes with parentheses, i.e:

    node.set('(' + node.test.source() + '? global.__$$labCov._statement(\'' + filename + '\',' + consequent + ',' + line + ',(' + node.consequent.source() + ')) : global.__$$labCov._statement(\'' + filename + '\',' + alternate + ',' + line + ',(' + node.alternate.source() + ')))');
  3. acorn has an option preserveParens that could be switched on

    If this option is true, parenthesized expressions
    are represented by (non-standard) ParenthesizedExpression nodes
    that have a single expression property containing the expression
    inside parentheses."

I think the easiest and safest is alternative 2. There shouldn't be any problems with adding extra parentheses around the consequent and alternate nodes. It's already being done around the entire ternary expression.

If you're ok with alternative 2 I can submit a PR with the fix and a test.

HCanber added a commit to HCanber/lab that referenced this issue 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 hapijs#555
HCanber added a commit to HCanber/lab that referenced this issue Mar 30, 2016
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Mar 31, 2016

@HCanber thanks for the write-up. Solution 2 sounds like the best approach.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Mar 31, 2016

We should instrument each element individually, such statements might not be completely covered if one of them throws.

@geek geek closed this in #556 Apr 5, 2016
@geek geek added this to the 10.3.1 milestone Apr 5, 2016
@geek geek self-assigned this Apr 5, 2016
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.