test: adding tests for fs/promises.js fileHandle methods #19605

willhayslett · 2018-03-26T03:39:22Z

Working to increase test code coverage for fs/promises.js.
Added tests for fileHandle.appendFile and fileHandle.chmod.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott · 2018-03-26T20:56:00Z

@nodejs/testing @nodejs/fs

willhayslett · 2018-03-28T23:16:41Z

Hi @Trott. Are the format of these tests okay? Just want to make sure before I try creating more.

Trott · 2018-03-29T00:19:16Z

Looks OK to me. Surprised these haven't gotten any reviews. I'm leaving work RIGHT NOW but let's try to ping some folks: @nodejs/collaborators

willhayslett · 2018-03-29T01:11:31Z

Thanks, @Trott!

benjamingr

Just a question - why do these methods validate fs methods by calling the sync versions and comparing the output? Wouldn't it make more sense to validate the actual output is what we expect?

For example in the writeFile example - wouldn't it be better to compare it to the buffer?

willhayslett · 2018-03-29T20:29:17Z

@benjamingr thanks for the feedback. I may have been overthinking it through trying to validate that the new methods should just behave as the old. I don't necessarily think it's a bad test to have, but it doesn't test the specification directly. I'll update the tests where appropriate tonight.

TimothyGu · 2018-03-30T20:26:16Z

@willclarktech Could you rebase to get rid of the merge commit in your branch?

willhayslett · 2018-03-30T20:34:51Z

Hi @TimothyGu. Was researching how to do that last night. Will it be okay to just squash all the commits on my branch into one?

Trott · 2018-03-30T21:21:33Z

Will it be okay to just squash all the commits on my branch into one?

That should be fine, although you can also just squash out the one merge commit too. Either way.

willhayslett · 2018-03-30T21:29:54Z

Thanks, everyone. I was able to get rid of the merge commit. Not sure what I did to get into that state but the merge commit itself had a lot of the changes I wanted. I ended up rebasing to the parent of the merge and reapplied my changes on top of it in a new commit. Let me know if any other changes are needed.

willhayslett · 2018-04-03T04:59:56Z

Hi @benjamingr; are the changes that were made okay? I'm still learning and very open to any feedback/refactoring requests. Just trying to get better and hopefully provide a healthy contribution.

Trott · 2018-04-03T05:11:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14008/

Trott · 2018-04-03T05:25:20Z

@willhayslett It looks like a new lint rule landed recently and there are a few comments in this PR that need a space after //. Would you mind adding them? Sorry about the delay in getting reviews on this. Otherwise, that probably wouldn't have happened. FWIW, these changes look good to me at a quick glance, but I'd rather get someone who has worked on the fs promises implementation to give it a closer review if possible.

Trott · 2018-04-03T05:25:39Z

test/parallel/test-fs-promises-file-handle-append-file.js

+}
+
+async function validateAppendString() {
+  //don't refresh the directory


Linter now requires a space after //.

Trott · 2018-04-03T05:25:48Z

test/parallel/test-fs-promises-file-handle-chmod.js

+
+  const filePath = path.resolve(tmpDir, 'tmp-chmod.txt');
+  const fileHandle = await open(filePath, 'w+', 0o644);
+  //file created with r/w 644


Trott · 2018-04-03T05:25:53Z

test/parallel/test-fs-promises-file-handle-chmod.js

+  //file created with r/w 644
+  const statsBeforeMod = fs.statSync(filePath);
+  assert.deepStrictEqual(statsBeforeMod.mode & 0o644, 0o644);
+  //change the permissions to 765


Trott · 2018-04-03T05:26:18Z

test/parallel/test-fs-promises-file-handle-write.js

+
+  const filePathForHandle = path.resolve(tmpDir, 'tmp-write-file.txt');
+  const fileHandle = await open(filePathForHandle, 'w+');
+  const buffer = Buffer.from(''); //empty buffer


And here too, probably.

willhayslett · 2018-04-03T05:56:34Z

Thanks for the feedback, @Trott. I've updated the tests to conform to the new rules.

Trott · 2018-04-03T13:49:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14016/

Trott · 2018-04-03T17:33:17Z

@willhayslett CI failures! Might require some digging to figure these out. Could be fun! Or frustrating! Let's see...

These tests are failing consistently on two platforms: Windows and Raspberry Pi

So, here's the test failing on a Raspberry Pi:

https://ci.nodejs.org/job/node-test-binary-arm/114/RUN_SUBSET=1,label=pi1-docker/console

10:44:41 not ok 81 parallel/test-fs-promises-file-handle-read
10:44:41   ---
10:44:41   duration_ms: 3.72
10:44:41   severity: fail
10:44:41   stack: |-
10:44:41     (node:28170) ExperimentalWarning: The fs/promises API is experimental
10:44:41     /home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:785
10:44:41                  (err) => process.nextTick(() => { throw err; }));
10:44:41                                                    ^
10:44:41     
10:44:41     Error: ENOTEMPTY: directory not empty, rmdir '/home/iojs/tmp/.tmp.0'
10:44:42         at Object.fs.rmdirSync (fs.js:788:3)
10:44:42         at rmdirSync (/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:48:10)
10:44:42         at rimrafSync (/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:18:7)
10:44:42         at Object.exports.refresh (/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:65:3)
10:44:42         at validateEmptyRead (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-promises-file-handle-read.js:32:10)
10:44:42   ...

The oddity with Raspberry Pi is that (I think) the tmp directory is NFS mounted. In general, we don't call tmpdir.refresh() twice in one test (AFAIK). We should be able to, though.

Windows failure looks like this:

https://ci.nodejs.org/job/node-test-binary-windows/16187/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=0/console

not ok 141 parallel/test-fs-promises-file-handle-chmod
  ---
  duration_ms: 0.323
  severity: fail
  stack: |-
    (node:2892) ExperimentalWarning: The fs/promises API is experimental
    c:\workspace\node-test-binary-windows\test\common\index.js:785
                 (err) => process.nextTick(() => { throw err; }));
                                                   ^
    
    AssertionError [ERR_ASSERTION]: 436 deepStrictEqual 501
        at validateFilePermission (c:\workspace\node-test-binary-windows\test\parallel\test-fs-promises-file-handle-chmod.js:27:10)
  ...

I haven't looked closely. This may be an OS limitation, or it may be an actual bug in the fs/promises.js implementation, or something else...

Trott

I'm leaving a "Request changes" just to make sure that no one lands this before the CI issues are fixed.

Trott · 2018-04-03T17:34:49Z

@willhayslett One thing you can do, if you want, is submit each new test in a separate pull request. Then, any that pass can land. And any that fail can be worked on individually. Or if you'd rather keep them as one bundle, that's fine too. Either way. Whatever works for you.

Trott · 2018-04-03T17:36:56Z

Windows fix is probably to use common.isWindows to check the platform and assign the expected value appropriately in (or right before) this line.

Trott · 2018-04-03T17:38:20Z

Windows fix is probably to use common.isWindows to check the platform and assign the expected value appropriately in (or right before) this line.

Although I'm not sure that it makes sense that the mode wouldn't come back the same as it does on POSIX. @nodejs/platform-windows @nodejs/fs @nodejs/testing

willhayslett · 2018-04-03T22:18:43Z

On it @Trott!

willhayslett · 2018-04-04T06:27:36Z

Hi @Trott, when you can, please run the CI tests again.

Also, is there a way I can execute them on my end, or otherwise test against each of the supported platforms?

Thanks!

Trott · 2018-04-04T11:22:36Z

test/parallel/test-fs-promises-file-handle-append-file.js

+}
+
+async function validateAppendString() {
+  // don't refresh the directory


As is, this gives me mild pause. Can you:

Remove this comment

Move the tmpdir.refresh() in validateAppendBuffer() outside of that function (as it now is initialization for the whole test file, not just that one test case)

Maybe change the name of the file on line 32 to file-append-file-string to match this function's name and also so that the two tests do not affect each other.

Oh, while at it, it seems that common.crashOnUnhandledRejection() should only be called once for the whole test file so move that outside of the individual test functions too?

Also, you'll need to make similar changes to test/parallel/test-fs-promises-file-handle-read.js as it was failing the same way in CI as test/parallel/test-fs-promises-file-handle-append-file.js was. I provided test/parallel/test-fs-promises-file-handle-append-file.js as an example.

Got it. I didn't realize those common methods were to only be used once per file. I placed their calls outside of the functions. Also updated test/parallel/test-fs-promises-file-handle-append-file.js so the validateAppendString test is cleaner. Let me know if any additional changes are needed.

Working to increase test code coverage for fs/promises.js. Added tests for fileHandle.appendFile and fileHandle.chmod.

Updated and added tests for fs/promises fileHandle. Changes include updates to chmod and appendFile. New tests were created for the read, readFile, write, and writeFile methods.

Updated fs/promises fileHandle tests. Removed indirect assertions that compared data between the old fs methods and those being tested in favor of more direct comparisons between the tested methods' output and the file input data.

Tests were updated to conform to the new linting rules that were landed.

Updated fs/promises fileHandle tests to resolve CI build test errors being returned from Windows and Rasberry Pi.

Updated tests to ensure common.crashOnUnhandledRejection() and the common/tmpdir refresh method and are only being called once per file.

Trott · 2018-04-04T12:10:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14038/

willhayslett · 2018-04-04T17:02:48Z

CI test failures again. Looks like it's from another piece of code though I'm not entirely sure.

/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test.tap:
not ok 717 parallel/test-http-client-timeout-agent```

Trott · 2018-04-04T17:53:26Z

@willhayslett Yes, that looks unrelated. I've opened an issue (#19804) and will re-run for this PR.

Re-run: https://ci.nodejs.org/job/node-test-commit-aix/13902/

Trott · 2018-04-05T02:06:47Z

Landed in 126b03e. Thanks, @willhayslett!

Working to increase test code coverage for fs/promises.js. Added tests for fileHandle.appendFile and fileHandle.chmod. PR-URL: nodejs#19605 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

willhayslett · 2018-04-05T02:13:20Z

Great! Thank you, @Trott!

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 26, 2018

willhayslett force-pushed the test-fs-promises-file-handle branch from 3ae097a to a24069b Compare March 28, 2018 07:02

benjamingr reviewed Mar 29, 2018

View reviewed changes

willhayslett force-pushed the test-fs-promises-file-handle branch 2 times, most recently from d2b9bce to 5f5977a Compare March 30, 2018 03:48

willhayslett force-pushed the test-fs-promises-file-handle branch from 5f5977a to 6a62939 Compare March 30, 2018 21:25

Trott reviewed Apr 3, 2018

View reviewed changes

willhayslett force-pushed the test-fs-promises-file-handle branch from 6a62939 to f8daeb9 Compare April 3, 2018 05:55

jasnell approved these changes Apr 3, 2018

View reviewed changes

Trott requested changes Apr 3, 2018

View reviewed changes

willhayslett force-pushed the test-fs-promises-file-handle branch from f8daeb9 to b461f0a Compare April 4, 2018 06:24

Trott reviewed Apr 4, 2018

View reviewed changes

targos mentioned this pull request Apr 4, 2018

Background color conflict with refined github in review comments StylishThemes/GitHub-Dark#603

Closed

willhayslett added 6 commits April 4, 2018 07:00

test: adding tests for fs/promises.js fileHandle methods

f98fc81

Working to increase test code coverage for fs/promises.js. Added tests for fileHandle.appendFile and fileHandle.chmod.

test: additional tests for fs/promises fileHandle

5183962

Updated and added tests for fs/promises fileHandle. Changes include updates to chmod and appendFile. New tests were created for the read, readFile, write, and writeFile methods.

test: updated tests for fs/promises fileHandle

74b6ba1

Updated fs/promises fileHandle tests. Removed indirect assertions that compared data between the old fs methods and those being tested in favor of more direct comparisons between the tested methods' output and the file input data.

test: updated fs/promises tests to conform to new linting rules

8377667

Tests were updated to conform to the new linting rules that were landed.

test: resolving cross platform support issues

eb0955b

Updated fs/promises fileHandle tests to resolve CI build test errors being returned from Windows and Rasberry Pi.

test: refactored use of common methods in fs/promises tests

5310f92

Updated tests to ensure common.crashOnUnhandledRejection() and the common/tmpdir refresh method and are only being called once per file.

willhayslett force-pushed the test-fs-promises-file-handle branch from b461f0a to 5310f92 Compare April 4, 2018 12:01

Trott approved these changes Apr 5, 2018

View reviewed changes

Trott mentioned this pull request Apr 5, 2018

Increase test coverage for fs/promises.js #19811

Closed

Trott closed this Apr 5, 2018

targos added the dont-land-on-v9.x label Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: adding tests for fs/promises.js fileHandle methods #19605

test: adding tests for fs/promises.js fileHandle methods #19605

willhayslett commented Mar 26, 2018 •

edited

Loading

Trott commented Mar 26, 2018

willhayslett commented Mar 28, 2018

Trott commented Mar 29, 2018

willhayslett commented Mar 29, 2018

benjamingr left a comment

willhayslett commented Mar 29, 2018

TimothyGu commented Mar 30, 2018

willhayslett commented Mar 30, 2018

Trott commented Mar 30, 2018

willhayslett commented Mar 30, 2018

willhayslett commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018 •

edited

Loading

Trott Apr 3, 2018

Trott Apr 3, 2018

Trott Apr 3, 2018

Trott Apr 3, 2018

willhayslett commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott left a comment

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

willhayslett commented Apr 3, 2018

willhayslett commented Apr 4, 2018

Trott Apr 4, 2018

Trott Apr 4, 2018

willhayslett Apr 4, 2018

Trott commented Apr 4, 2018

willhayslett commented Apr 4, 2018

Trott commented Apr 4, 2018

Trott commented Apr 5, 2018

willhayslett commented Apr 5, 2018

test: adding tests for fs/promises.js fileHandle methods #19605

test: adding tests for fs/promises.js fileHandle methods #19605

Conversation

willhayslett commented Mar 26, 2018 • edited Loading

Checklist

Trott commented Mar 26, 2018

willhayslett commented Mar 28, 2018

Trott commented Mar 29, 2018

willhayslett commented Mar 29, 2018

benjamingr left a comment

Choose a reason for hiding this comment

willhayslett commented Mar 29, 2018

TimothyGu commented Mar 30, 2018

willhayslett commented Mar 30, 2018

Trott commented Mar 30, 2018

willhayslett commented Mar 30, 2018

willhayslett commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018 • edited Loading

Trott Apr 3, 2018

Choose a reason for hiding this comment

Trott Apr 3, 2018

Choose a reason for hiding this comment

Trott Apr 3, 2018

Choose a reason for hiding this comment

Trott Apr 3, 2018

Choose a reason for hiding this comment

willhayslett commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott left a comment

Choose a reason for hiding this comment

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

Trott commented Apr 3, 2018

willhayslett commented Apr 3, 2018

willhayslett commented Apr 4, 2018

Trott Apr 4, 2018

Choose a reason for hiding this comment

Trott Apr 4, 2018

Choose a reason for hiding this comment

willhayslett Apr 4, 2018

Choose a reason for hiding this comment

Trott commented Apr 4, 2018

willhayslett commented Apr 4, 2018

Trott commented Apr 4, 2018

Trott commented Apr 5, 2018

willhayslett commented Apr 5, 2018

willhayslett commented Mar 26, 2018 •

edited

Loading

Trott commented Apr 3, 2018 •

edited

Loading