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

Possible error with coverage report #12136

Closed
lucamaraschi opened this issue Mar 30, 2017 · 5 comments
Closed

Possible error with coverage report #12136

lucamaraschi opened this issue Mar 30, 2017 · 5 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers. test Issues and PRs related to the tests.

Comments

@lucamaraschi
Copy link
Contributor

  • Version: master
  • Platform: all
  • Subsystem: test, fs

In the test coverage report line 131 is marked as not covered but a test for this exception/line already exists (https://github.com/nodejs/node/blob/master/test/parallel/test-fs-make-callback.js).

cc: @addaleax && @nodejs/testing

@addaleax
Copy link
Member

@lucamaraschi That test uses fs.stat which uses makeStatsCallback, not makeCallback for special callback handling… I think the line is really not covered?

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers. test Issues and PRs related to the tests. labels Mar 30, 2017
@lucamaraschi
Copy link
Contributor Author

@addaleax you are indeed right!Gonna open a PR on the test to extend the case also to makeCallback.

@gibfahn
Copy link
Member

gibfahn commented Mar 30, 2017

@lucamaraschi in that case I'll reopen this, and you can close it in your PR!

In case you didn't know, you can add:

Fixes: https://github.com/nodejs/node/issues/12136

to your commit description to auto-close this when your PR lands.

@gibfahn gibfahn reopened this Mar 30, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Mar 30, 2017

@gibfahn I'm not sure that adding coverage for that line of code is related to this issue. This issue was opened for a possible bug in the coverage reporting, but that turned out not to be the case.

@gibfahn
Copy link
Member

gibfahn commented Mar 30, 2017

@cjihrig the way I see it, the issue was raised to ask why that line wasn't being reported as covered. The answer is because the test doesn't cover it, so a PR fixing the test would resolve the issue.

lucamaraschi added a commit to lucamaraschi/my.node that referenced this issue Apr 1, 2017
makeCallback and makeStatsCallback are both tested intedependently.

Fixes: nodejs#12136
gibfahn pushed a commit that referenced this issue Jun 19, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this issue Jun 20, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: nodejs/node#12140
Fixes: nodejs/node#12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants