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 coverage report for single-line functions and single-expression arrow functions #520

Merged
merged 7 commits into from Feb 18, 2016

Conversation

@rluba
Copy link
Contributor

rluba commented Feb 17, 2016

It seems like lab does not correctly report coverage on various forms of short function declarations:

  1. Any kind of function expression that’s written on a single line (see #274)
  2. Single-expression arrow functions (eg. (a, b) => a+b), even if they span multiple lines.

This pull request adds a failing coverage test that demonstrates the problem. Unfortunately, I don’t know enough about lab’s AST transformations to be able to fix the problem.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 17, 2016

Is this a dup of #274

EDIT: Ignore me. I thought this was an issue report, not a PR.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 17, 2016

Thx, I’ve added #274 to the description (where it applies).

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 17, 2016

I was able to fix single-expression arrow functions in dba4897.

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 17, 2016

@rluba there is one failing test... otherwise it looks good

@geek geek added the bug label Feb 17, 2016
@geek geek added this to the 8.3.1 milestone Feb 17, 2016
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 17, 2016

@geek Yeah, the failing test is the one I added in this pull request!

  • dba4897 fixes only 2. (single-expression arrow functions)
  • The other part (single-line function expressions) is still open
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 17, 2016

0bf75e8 fixes single-line function expressions.

This fixes the remaining test cases that this PR originally added.

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Feb 17, 2016

Can it handle OR statements too? For example in case of (a, b) => a || b, does it report a missing coverage when only the left side (a) is tested?

… as suggested by @gergoerdosi. Works as expected.
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 18, 2016

@gergoerdosi Valid question. I’ve added a corresponding test case and it works as expected. See c2c3595.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 18, 2016

As far as I’m concerned, this PR is done and ready to be merged.

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

gergoerdosi commented Feb 18, 2016

Nice, thanks!

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 18, 2016

Btw: Don’t be surprised if your reported coverage drops after these changes — might break some -t 100 builds. (It broke some of mine…)

@rluba rluba changed the title Add failing test for reporting coverage on single-line functions Fix coverage report for single-line functions and single-expression arrow functions Feb 18, 2016
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 18, 2016

Seems like Travis stumbled and timed out, the tests still run fine.

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 18, 2016

Fixed.

@geek geek added feature and removed bug labels Feb 18, 2016
@geek geek self-assigned this Feb 18, 2016
geek added a commit that referenced this pull request Feb 18, 2016
Fix coverage report for single-line functions and single-expression arrow functions
@geek geek merged commit 7a316ef into hapijs:master Feb 18, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.