istanbuljs / nyc

the Istanbul command line interface
https://istanbul.js.org/
ISC License
5.57k stars 358 forks source link

reported uncovered lines that actually have been covered #1396

Open lichader opened 3 years ago

lichader commented 3 years ago

Link to bug demonstration repository

https://github.com/lichader/nyc-demo

Expected Behavior

Expected 100% coverage

Observed Behavior

Reporting Branch coverage as 50%.

I made a basic repo to show what the problem is. I have a simple js file here that just prints something in the log. From console output I pasted below, you can see that the code inside the if block is hit. But nyc reported lines 8-16 are uncovered, which is quite strange. Not sure where I did wrong or it is a bug.

This is the output here:

 π nyc-poc master ✗ ❯ npm run cover:unit

> unit@1.0.0 cover:unit /Users/me802707/work/rpeng/nyc-poc
> nyc --all npm run test

> unit@1.0.0 test /Users/me802707/work/rpeng/nyc-poc
> mocha tests/*.test.js --reporter mocha-junit-reporter --reporter-options mochaFile=./test-results/test-results.xml

found bar: user1
found bar: user2
bar is found
ERROR: Coverage for branches (50%) does not meet global threshold (90%)
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |       50 |     100 |     100 |
 foo.js   |     100 |       50 |     100 |     100 | 8-16
----------|---------|----------|---------|---------|-------------------

Troubleshooting steps

run command: npm install && npm run cover:unit

Environment Information

  System:
    OS: macOS 11.2.1
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 51.58 MB / 16.00 GB
  Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm
  npmPackages:
    nyc: 15.1.0 => 15.1.0

Thank you

jmm commented 3 years ago

Hi @lichader, I think the problem you're having is that you're not exercising the cases where usersList or foundUser are falsy.

On another note, I don't know if this is a simplified example, but the block of 8-14 doesn't make too much sense. It seems like you'd either want to check the length of usersList, or call .some or .find.

IzaacJ commented 3 years ago

I have a the same issue where nyc reports a line that definitively is covered, as it shows up in my logs 9 seperate times during a test run, so it's weird. And the apply method is used in most of the tests. I use nyc in combination with ava.

EDIT: When commenting out all tests except the one that actually covers line 79, it is suddenly covered...

This is the console output:

PS C:\Users\izaac\sources\test-lib> yarn tsc -p tsconfig.json ; yarn nyc ava
yarn run v1.22.5
$ C:\Users\izaac\sources\test-lib\node_modules\.bin\tsc -p tsconfig.json
Done in 3.43s.
yarn run v1.22.5
$ C:\Users\izaac\sources\test-lib\node_modules\.bin\nyc ava

  14 tests passed
  1 known failure
----------------|---------|----------|---------|---------|-------------------
File            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------------|---------|----------|---------|---------|-------------------
All files       |     100 |    96.88 |     100 |     100 | 
 lib            |     100 |      100 |     100 |     100 | 
  Logger.ts     |     100 |      100 |     100 |     100 | 
  myexports.ts  |     100 |      100 |     100 |     100 | 
 lib/ObjectBase |     100 |    96.88 |     100 |     100 | 
  classes.ts    |     100 |    96.88 |     100 |     100 | 79
  index.ts      |     100 |      100 |     100 |     100 | 
----------------|---------|----------|---------|---------|-------------------
Done in 3.88s.

And the code:

export class ObjectBase {
        private applied?: ObjectBaseOptions;
        private pending?: ObjectBaseOptions;

        // private vars, constructor, a few getters/setters

    toString(this: ObjectBase): string {
        Logger.log('info', 'ObjectBase::toString');
        const ret: string[] = [];

        ret.push(this.oid, this.name);

        Logger.log('info', 'ObjectBase::toString ' + ret.join(' '));
        return ret.join(' ');
    }

    apply(this: ObjectBase, options?: ObjectBaseOptions<ObjectBase>) {
        Logger.log('info', 'ObjectBase::apply');
        if (undefined !== options && options !== null) {
            Object.assign(this.pending, options);
        }
        if (undefined === this.pending) {
            Logger.log('info', 'ObjectBase::apply pending undefined');
            this.pending = {};
        }
        Logger.log('error', this.applied);     // <---- Shows as undefined in the logs
        if (undefined === this.applied) {   // <---- LINE 79
            Logger.log('error', 'ObjectBase::apply applied undefined');      // <----- Also shows up in the logs
            this.applied = {};
        }

        Object.assign(this.applied, this.pending);

        this.pending = {};

        Logger.log('info', 'ObjectBase::apply Not completely implemented');
    }

    reset() {   // <---- Running this function right before apply to make sure that condition is hit.
            this.applied = undefined;
            this.pending = undefined;
    }

    // Some other skeletal methods

}
rballonline commented 3 years ago

Getting the same, 50% as well. It wasn't reporting that they weren't covered until the rest of the file was actually covered. Terminal is showing grey numbers instead of red numbers. I don't know if it's a bug either because I have like 10 minutes of experience using this but thought it was odd.

hedocode commented 3 years ago

I have the same problem on if statements, however, I wrote console.log statements into these ifs and they are properly displayed in the console

andrecadete commented 2 years ago

Sorry to revive such an old topic but this is still not consistent:

image

Lines 23-25 reported as not covered in the last file, while also stating 100% line, statement and func covered. Lines 23-25 are 3 of the 5 constructor arguments, which makes no sense - they're all mocks so either all constructor lines are uncovered or all covered, half-way seems weird.

The fact it states 100% lines covered by itself should prove inconsistency of "Uncovered line #s", no?

dhensby commented 2 years ago

@andrecadete I think those lines are the lines where the code branches aren't covered.

I'm pretty sure this comes down to semantics mostly because even though an actual line might have an execution path run through it, it doesn't mean every logic branch on that line has been covered.

For example you could have a function like this:

function myFunc(a, b) {
  return a || b;
}

And a test like:

it('returns first arg', () => {
  expect(myFunc(1, 2)).to.equal(1);
});

this will show up as 100% of lines covered, but 50% of branches covered and likely show you "uncovered lines" too...

I just tested this locally and with this example got:

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |       50 |     100 |     100 |                  
 file.js  |     100 |       50 |     100 |     100 | 2                
----------|---------|----------|---------|---------|-------------------

With another test added to check the other branch, I then see 100% across the board:

it('returns a', () => {
    expect(myFunc(1, 2)).to.equal(1);
});
it('returns b if a falsey', () => {
    expect(myFunc(0, 2)).to.equal(2);
});
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                  
 file.js  |     100 |      100 |     100 |     100 |                  
----------|---------|----------|---------|---------|-------------------
andrecadete commented 2 years ago

Hi dhensby,

Makes sense for actual branches, but these are mocked Constructor arguments, how exactly does your example relate to that use-case?

Either way, once we cover the remaining branches I'll let you know if that fixed it. But seems silly, it should instead state which lines are partially tested when less than 100% of branches were tested. There's no way it can know which constructor arguments are used in untested branches without knowing which lines those are. So indicating those lines as untested or (a new feature) as partially tested would have actual use while indicating constructor arguments says very little.