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

test: increase fs.exists coverage #15963

Closed

Conversation

@NigelKibodeaux
Copy link
Contributor

commented Oct 6, 2017

Tests the case of calling fs.exists without a callback.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@mscdex mscdex added the fs label Oct 6, 2017

@Trott Trott added the code-and-learn label Oct 6, 2017

@@ -30,6 +30,8 @@ fs.exists(f, common.mustCall(function(y) {
assert.strictEqual(y, true);
}));

fs.exists(f);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Oct 7, 2017

Contributor

This should probably assert something.

This comment has been minimized.

Copy link
@jasnell

jasnell Oct 10, 2017

Member

For instance,

assert.doesNotThrow(() => fs.exists(f));

This comment has been minimized.

Copy link
@NigelKibodeaux

NigelKibodeaux Oct 10, 2017

Author Contributor

Thanks, I just fixed it.

@@ -30,6 +30,8 @@ fs.exists(f, common.mustCall(function(y) {
assert.strictEqual(y, true);
}));

assert.doesNotThrow(fs.exists(f));

This comment has been minimized.

Copy link
@cjihrig

cjihrig Oct 10, 2017

Contributor

This isn't quite right yet. In this case, fs.exists(f) will be evaluated, and the return value will be passed to assert.doesNotThrow(). If you wrap fs.exists(f) in a function first, then it will be good to go. It would look like:

assert.doesNotThrow(() => { fs.exists(f); });

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

@gireeshpunathil

This comment has been minimized.

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

CI failed with merge conflict. My guess it because of the different commeints. @NigelKibodeaux could you squash the commits down to 1 ?

@apapirovski

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

Hi @NigelKibodeaux, would you still like to follow up on this so that it can land in Node? Let me know if I can offer any pointers. Thanks for helping improve Node!

@NigelKibodeaux NigelKibodeaux force-pushed the NigelKibodeaux:fs-exists-coverage branch from 70c18e1 to e714c42 Oct 25, 2017

@NigelKibodeaux

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2017

Hi @apapirovski, I'm attempting to squash my commits. I hope this works...

@apapirovski

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

@NigelKibodeaux seems to have worked 😄 I'll get a CI started and post a link shortly.

Edit: oops, can you remove the merge commit. Let me know if you need pointers.

@tniessen tniessen force-pushed the NigelKibodeaux:fs-exists-coverage branch from 3c5d976 to 7bcf844 Oct 28, 2017

@tniessen

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

I rebased and pushed for you 😃 CI: https://ci.nodejs.org/job/node-test-commit/13558/

(For further reference, NigelKibodeaux@6d9abdd is the original commit.)

tniessen added a commit that referenced this pull request Oct 28, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@tniessen

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

Landed in 4ed152e, thank you! 🎉

@tniessen tniessen closed this Oct 28, 2017

@NigelKibodeaux

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2017

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 30, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn added a commit that referenced this pull request Oct 31, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@gibfahn gibfahn referenced this pull request Oct 31, 2017

Merged

v8.9.0 proposal #16630

Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

test: increase fs.exists coverage
PR-URL: nodejs/node#15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

test: increase fs.exists coverage
PR-URL: nodejs/node#15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 16, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

MylesBorins added a commit that referenced this pull request Nov 21, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

test: increase fs.exists coverage
PR-URL: #15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

test: increase fs.exists coverage
PR-URL: nodejs/node#15963
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.