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

Lab report showing incorrect information #931

Closed
ferrao opened this issue Aug 2, 2019 · 9 comments
Closed

Lab report showing incorrect information #931

ferrao opened this issue Aug 2, 2019 · 9 comments
Assignees
Labels
non issue Issue is not a problem or requires changes

Comments

@ferrao
Copy link

ferrao commented Aug 2, 2019

Lab report tells me that a condition in my code will always evaluate to true, but I believe this is not correct, bellow some sample code to validate.

lib/index.js

module.exports = function(response, options = {}) {
    const { key } = options;

    if (!Array.isArray(response) && !key) {
        throw Error('missing key')
    }

    if (key && !response[key]) {
        throw Error('wrong key')
    }
    
    // if (key) {
    if (key && !Array.isArray(response)) {
        return 1;
    }

    return 0;
}

test/test.js

const test = require("../lib");

describe("Testing lab coverage", () => {
  it("1 - valid object and key", () => {
    expect(test({ someKey: "xpto" }, { key: "someKey" })).to.equal(1);
  });

  it("2 - valid object and no key", () => {
    expect(() => test({ someKey: "xpto" })).to.throw();
  });

  it("3 - array and no key", () => {
    expect(test([])).to.equal(0);
  });

  it("4 - array and key", () => {
    expect(() => test([], { key: "someKey" })).to.throw();
  });

  it('5 - array and numeric key', () => {
       expect(test([1,2], { key: 1 })).to.equal(0);
  });
});

If I run the following test code through lab with ./node_modules/.bin/lab test -v -t 100 I am getting a missing coverage on line if (key && !Array.isArray(response)) { and if I generate a report with ./node_modules/.bin/lab test -v -t 100 -r html I get on the same line an indication regarding !Array.isArray(response) - Condition always true.

As is, I can in fact remove the right side of the boolean expression and all tests will still pass.

If I uncomment the last test in the suite I now get 100% coverage and no indication regarding the condition.

But with this last test, I can not remove the right side of the boolean expression, as this last test will now fail, making my point that such condition is NOT always true.

@ferrao
Copy link
Author

ferrao commented Aug 2, 2019

This old case #401 is probably related?

@hapijs hapijs deleted a comment from ferrao Aug 2, 2019
@Marsup
Copy link
Contributor

Marsup commented Aug 2, 2019

I'm not sure I follow you, without your last test of course it's missing coverage. Either your function or your tests logic is flawed, but everything seems normal to me.

@ferrao
Copy link
Author

ferrao commented Aug 2, 2019

What I am claiming is that without the last test, lab reports Condition always true which is wrong. If condition was always true as lab claims, I could remove the redundant condition from my code.

But I can not, because if I do it will make that last test fail.

@Marsup
Copy link
Contributor

Marsup commented Aug 2, 2019

But it is always true. There is never a case where you reach that if with a key and an array. Your test 3 has an array but no key.

@ferrao
Copy link
Author

ferrao commented Aug 2, 2019

Forget about it... I was trying to come up with a more simple example that the actual code to explain what was happening, but I failed miserably at my example and I believe you are right that lab is doing the right thing (with my actual code and not the example I mean)....

@Marsup
Copy link
Contributor

Marsup commented Aug 2, 2019

To explain further, lab checks logical coverage.
So with if (a && b) the possible scenarios are:

a b
true true
true false
false not important

You have case 1 and 3 but you're clearly missing the 2nd.

@ferrao
Copy link
Author

ferrao commented Aug 2, 2019

Yes, I understand and it is how I expected coverage to work, but let me make another try at explaining my point, I have renamed the tests for better clarity.

What seems wrong to me is not the coverage at all, but the Condition always true message I am getting on the coverage report, but perhaps I am misunderstanding the meaning of it.

Lets say I have code a with condition if (key && !Array.isArray(response)) and code b with condition if (key).

Running the exact same 5 tests above on code a and code b produces different results. It passes all tests with b and fails test 5 with a . So these two pieces of code can not be interchangeable.

That said, if I simply comment test 5, the coverage report now informs me that code a contains a condition which is always true, which leads me to think I can replace code a with code b, which I know that I can not as I concluded they are not the same on the previous step.

Anyway, thank you for your time on this @Marsup, it is by no means a big issue to me, more just a curiosity, so if you believe that my point is not valid let's just close the issue.

@Marsup
Copy link
Contributor

Marsup commented Aug 2, 2019

Well, when it is reached, it's always true, when it's not the value has no point since it virtually doesn't exist. In terms of coverage, I think it's harder to differentiate between a value that's reached and false, and not reached, we could have an execution counter for each elements of conditionals but I'm not sure it would make it clearer.

@hueniverse hueniverse added the non issue Issue is not a problem or requires changes label Aug 7, 2019
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
non issue Issue is not a problem or requires changes
Projects
None yet
Development

No branches or pull requests

3 participants