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

Coverage gives a false positive with a || branch #841

Open
ljharb opened this issue May 12, 2018 · 6 comments
Open

Coverage gives a false positive with a || branch #841

ljharb opened this issue May 12, 2018 · 6 comments

Comments

@ljharb
Copy link
Contributor

ljharb commented May 12, 2018

Expected Behavior

The below code will not have 100% coverage, because keys.length is never > 1, so the "if" branch of that condition was never exercised.

Observed Behavior

The below code is reported as having 100% coverage.

Bonus Points! Code (or Repository) that Reproduces Issue

function test(options = {}) {
  const keys = Object.keys(options);
  if (keys.length > 1 || (keys.length === 1 && keys[0] !== 'something valid')) {
    return false;
  }
  return true;
}
test();
test({});
test({ 'something valid': true });

Forensic Information

Operating System: Mac OS
Environment Information: node 10.1, npm 5.6.

@ljharb
Copy link
Contributor Author

ljharb commented May 12, 2018

Specifically, if I remove this line then I should not have 100% coverage anymore.

@coreyfarrell
Copy link
Member

@ljharb I've done some testing with nyc@12.0.2. See https://gist.github.com/coreyfarrell/1a98d9a4d2cbf46854f2c9936dab0c77 for a slightly modified version of your test function and how that is instrumented by NYC. As far as I can tell the coverage data for the if branch is correct, it reports if you fail to hit both true and false conditions. The issue is for the binary-expr branches, it looks like we're counting when each sub-expression is hit regardless of result instead of counting how many results were truthy and how many weren't.

I have to think about this more to determine a good solution which would avoid breaking stuff like txt = txt || 'default';.

@ljharb
Copy link
Contributor Author

ljharb commented Aug 13, 2018

Thanks, that sounds like progress to me :-D

@stale stale bot added the wontfix label Jan 5, 2019
@ljharb
Copy link
Contributor Author

ljharb commented Jan 6, 2019

bump.

@stale stale bot removed the wontfix label Jan 6, 2019
@stale stale bot added the stale label Mar 7, 2019
@ljharb
Copy link
Contributor Author

ljharb commented Mar 7, 2019

bump

@stale stale bot removed the stale label Mar 7, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Mar 7, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Mar 7, 2019
@coreyfarrell
Copy link
Member

@ljharb I still don't have a solution for this bug but the stale bot will not be touching this issue again.

@JaKXz JaKXz added the bug label Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants