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

Fix issue-426: Add the test for fsPromises.readdir when the path exsts, and is a directory #452

Merged
merged 1 commit into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@ywpark1
Copy link
Contributor

commented Sep 22, 2018

I added two tests for fsPromises.readdir function to ensure that fsPromises.readdir is a function, and it returns the contents of the directory if the path given exists and is a directory.

I was not sure if I should create new group for fsPromises.readdir test cases(i.e. describe(...) ), so I simply added under fs.readdir test cases by adding '(promise)' in the front.

I also checked PR-issue-400. It seems like there is a conflict between my tests and @jespirit 's test in terms of how fsPromises tests are written.

Any advice for this?

@jespirit

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2018

I think you should separate the tests in two different sections because the functions are different in terms of their arguments and what they return, so fs.readdir is quite different than fs.promises.readdir.

I agree there seems to be a syntax conflict for formatting the tests and we should try agree on a consensus so that the code looks like it was written by one person. You can refer to my comment I made here (at the bottom) regarding the syntax about promises in another pull request #402 (comment)

For example, I could rewrite your tests to look like mine, or vice-versa.

.then(() => fsPromises.readdir('/'))
// same as
.then(() => return fsPromises.readdir('/'))
// same as
.then(() => {
  return fsPromises.readdir('/');
})
// same as
.then(function() {
  return fsPromises.readdir('/');
})
@@ -54,4 +54,33 @@ describe('fs.readdir', function() {
});
});
});

it('(promise) should be a function', function() {

This comment has been minimized.

Copy link
@YuechengWu

YuechengWu Sep 27, 2018

Contributor

Just to nitpick here, you can use () => {...} to declare a function instead of function() {...}. It's a shorter syntax so less typing.

@humphd humphd merged commit 3e0da99 into filerjs:master Oct 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.