test: added coverage tests related with method fs.read and fs.readSync #17875

bacriom · 2017-12-27T09:17:18Z

Tests added to extend the code coverage testing of the project on the method read and readSync of the file fs.js

Checklist

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

Affected core subsystem(s)

test added to extend the code coverage testing of the project

mmarchini · 2017-12-27T10:48:00Z

test/parallel/test-fs-read-constructor-errors.js

@@ -0,0 +1,58 @@
+// Copyright Joyent, Inc. and other Node contributors.


Correct me if I'm wrong, but this shouldn't say Copyright Joyent, Inc. unless you work there

The license header is only necessary for pre-io.js files.

sorry I'll fix it

mmarchini · 2017-12-27T10:48:10Z

test/parallel/test-fs-read-constructor-errors.js

+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+const filepath = fixtures.path('x.txt');
+const fd = fs.openSync(filepath, 'r');


Shouldn't this be fs.open(filepath, 'r'), as this is the async test?

@mmarchini Sync I/O is okay the initialization stage.

Didn't know that. Thanks!

mmarchini · 2017-12-27T11:07:10Z

test/parallel/test-fs-read-constructor-errors.js

+
+const expected = Buffer.from('xyz\n');
+
+common.expectsError(


I'd suggest changing the .bind with an arrow function, here and elsewhere. It's more obvious what the code does and also is how other expectsError tests are written:

common.expectsError(() => { fs.read(fd, Buffer.allocUnsafe(expected.length), 0, expected.length, 0, common.mustNotCall()); }, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError });

ok thanks for all, I 'll make some changes.
@mmarchini I'll close this pull and I'll create a new

PR-URL: nodejs#17852 Refs: nodejs#17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mmarchini · 2017-12-27T18:38:01Z

You can just update this PR, no need to create a new one :)

To update it you can follow the steps described here. And don't hesitate to ask if you have any questions.

PR-URL: nodejs#17656 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Error occurs while dealing with Tar archives PR-URL: nodejs#17663 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

The `rawPacket` is the current buffer that just parsed. Adding this buffer to the error object of `clientError` event is to make it possible that developers can log the broken packet. PR-URL: nodejs#17672 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor and simplify the perf_hooks native internals. PR-URL: nodejs#17822 Reviewed-By: Anna Henningsen <anna@addaleax.net>

`options.server` only needs to be set when its contents are actually being inspected. PR-URL: nodejs#17835 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Assert the server name directly in the `SNICallback`, since `common.mustCall()` already guarantees that the callback is called exactly once, making `process.on('exit')` unnecessary. PR-URL: nodejs#17836 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

I noticed that ocsp_request is not being reset in ClientHelloParser::Reset. I've not been able to figure out the the reason for this and wanted to bring this up just in case this was overlooked and should be reset. PR-URL: nodejs#17753 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Compute the floating point number in JavaScript to avoid having to call out to the C++ runtime. The improvements are not insubstantial: improvement confidence p.value value="big" endian="BE" type="Double" noAssert="false" 292.86 % *** 1.688367e-08 value="big" endian="BE" type="Double" noAssert="true" 353.19 % *** 6.079414e-10 value="big" endian="BE" type="Float" noAssert="false" 406.21 % *** 1.730122e-07 value="big" endian="BE" type="Float" noAssert="true" 450.81 % *** 6.909242e-07 value="big" endian="LE" type="Double" noAssert="false" 268.39 % *** 8.625486e-09 value="big" endian="LE" type="Double" noAssert="true" 310.66 % *** 2.798332e-15 value="big" endian="LE" type="Float" noAssert="false" 382.99 % *** 3.412057e-07 value="big" endian="LE" type="Float" noAssert="true" 394.60 % *** 1.406742e-07 value="inf" endian="BE" type="Double" noAssert="false" 312.91 % *** 7.407943e-12 value="inf" endian="BE" type="Double" noAssert="true" 392.47 % *** 3.821179e-08 value="inf" endian="BE" type="Float" noAssert="false" 466.01 % *** 8.953363e-08 value="inf" endian="BE" type="Float" noAssert="true" 460.76 % *** 5.381256e-09 value="inf" endian="LE" type="Double" noAssert="false" 279.50 % *** 2.390682e-09 value="inf" endian="LE" type="Double" noAssert="true" 335.30 % *** 3.587173e-09 value="inf" endian="LE" type="Float" noAssert="false" 439.77 % *** 1.057133e-07 value="inf" endian="LE" type="Float" noAssert="true" 426.72 % *** 4.353408e-09 value="nan" endian="BE" type="Double" noAssert="false" 271.18 % *** 2.281526e-05 value="nan" endian="BE" type="Double" noAssert="true" 312.63 % *** 1.974975e-07 value="nan" endian="BE" type="Float" noAssert="false" 429.17 % *** 2.416228e-07 value="nan" endian="BE" type="Float" noAssert="true" 461.39 % *** 1.956714e-08 value="nan" endian="LE" type="Double" noAssert="false" 267.03 % *** 9.938479e-12 value="nan" endian="LE" type="Double" noAssert="true" 276.93 % *** 7.842481e-08 value="nan" endian="LE" type="Float" noAssert="false" 415.97 % *** 8.082710e-07 value="nan" endian="LE" type="Float" noAssert="true" 433.68 % *** 1.030200e-07 value="small" endian="BE" type="Double" noAssert="false" 273.20 % *** 9.071652e-11 value="small" endian="BE" type="Double" noAssert="true" 326.25 % *** 3.120167e-08 value="small" endian="BE" type="Float" noAssert="false" 845.61 % *** 8.044170e-08 value="small" endian="BE" type="Float" noAssert="true" 868.61 % *** 2.944539e-08 value="small" endian="LE" type="Double" noAssert="false" 251.29 % *** 5.613930e-09 value="small" endian="LE" type="Double" noAssert="true" 286.82 % *** 8.149603e-10 value="small" endian="LE" type="Float" noAssert="false" 824.87 % *** 1.199729e-08 value="small" endian="LE" type="Float" noAssert="true" 834.35 % *** 4.799620e-08 value="zero" endian="BE" type="Double" noAssert="false" 216.70 % *** 3.872293e-12 value="zero" endian="BE" type="Double" noAssert="true" 239.31 % *** 6.439601e-09 value="zero" endian="BE" type="Float" noAssert="false" 353.75 % *** 3.639974e-07 value="zero" endian="BE" type="Float" noAssert="true" 388.86 % *** 7.074318e-10 value="zero" endian="LE" type="Double" noAssert="false" 179.34 % *** 5.230763e-06 value="zero" endian="LE" type="Double" noAssert="true" 199.66 % *** 2.177589e-11 value="zero" endian="LE" type="Float" noAssert="false" 299.55 % *** 9.961978e-08 value="zero" endian="LE" type="Float" noAssert="true" 333.30 % *** 2.470764e-08 PR-URL: nodejs#17775 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 PR-URL: nodejs#17780 Reviewed-By: James M Snell <jasnell@gmail.com>

`parallel/test-tls-invoke-queued` previously used the internal `_write()` API to hook into the internals more directly, but this invalidates the general assumption made by streams APIs that only a single write is active at a time, and which is enforced through the public API. PR-URL: nodejs#17864 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. PR-URL: nodejs#17338 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

- Simplify the SyncCall template function, only collect error number and syscall in the C++ layer and collect the rest of context in JS for flexibility. - Remove the stringFromPath JS helper now that the unprefixed path is directly put into the context before the binding is invoked with the prefixed path. - Validate more properties in fs.access tests. PR-URL: nodejs#17338 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

* Collect the error context in both JS and C++, then throw the error in JS * Test that the errors thrown from fs.close and fs.closeSync includes the correct error code, error number and syscall properties PR-URL: nodejs#17338 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

A couple of lib/_http_outgoing.js's errors were still in the "old style": `throw new Error(<some message here>)`. This commit migrates those 2 old style errors to the "new style": internal/errors.js's error-system. In the future, changes to these errors' messages won't break semver-major status. With the old style, changes to these errors' messages broke semver-major status. It was inconvenient. Refs: nodejs#17709 PR-URL: nodejs#17837 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test added to extend the code coverage testing of the project

apapirovski

SGTM with a few nits.

apapirovski · 2017-12-27T20:42:25Z

test/parallel/test-fs-read-constructor-errors.js

+               expected.length,
+               0,
+               common.mustNotCall()),
+  { code: 'ERR_INVALID_ARG_TYPE', type: TypeError });


For ERR_INVALID_ARG_TYPE, a general best practice is to use forEach and iterate over all the possible types that the function will throw for. Something like [true, 0, null, undefined, () => {}, {}, Symbol()].forEach((arg) => test). (Of course, adjust as necessary depending on what the function actually accepts.)

Also, a general rule is that we also test the "message" of the error object.

@apapirovski I have a doubt when the parameter fd gets a 0 the error ERR_INVALID_ARG_TYPE has to be thrown?

apapirovski · 2017-12-27T20:43:26Z

test/parallel/test-fs-read-constructor-errors.js

+const common = require('../common');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+const filepath = fixtures.path('x.txt');


Nit: add empty line after all the require statements.

empty lines added

apapirovski · 2017-12-27T20:44:44Z

test/parallel/test-fs-read-constructor-errors.js

+               expected.length,
+               0,
+               common.mustNotCall()),
+  { code: 'ERR_OUT_OF_RANGE', type: RangeError });


Does this have an upper boundary or just lower? If it does, that would ideally also be tested.

…riom-branch

mmarchini · 2017-12-28T18:09:29Z

Looks like there's a merge commit here. To avoid it we use git rebase instead of git merge (see CONTRIBUTING.md#step-5-rebase).

It can be fixed with:

$ git checkout bacriom-branch
$ git remote add upstream https://github.com/nodejs/node.git  # If there's no remote pointing to nodejs/node yet
$ git fetch upstream
$ git rebase upstream/master  # Fix any conflicts if they happen
$ git diff origin/master  # Check if everything is alright
$ git push --force-with-lease  # Forces

bacriom · 2017-12-28T18:15:09Z

@mmarchini ok , I'll fix the merge problem, also I'll add the suggestions of the above comments

bacriom added 4 commits December 26, 2017 22:29

test: add test-fs-read-constructor-errors test

37c97b1

test added to extend the code coverage testing of the project

test: add test-fs-readSync-constructor-errors test

d7326b2

test added to extend the code coverage testing of the project

test: add test-fs-read-constructor-errors test

bd3aa3e

test added to extend the code coverage testing of the project

test: add test-fs-readSync-constructor-errors test

5e59b4f

test added to extend the code coverage testing of the project

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 27, 2017

bacriom changed the title ~~test: add coverage tests relates with method fs.read~~ test: add coverage tests relates with method fs.read and fs.readSync Dec 27, 2017

bacriom changed the title ~~test: add coverage tests relates with method fs.read and fs.readSync~~ test: added coverage tests related with method fs.read and fs.readSync Dec 27, 2017

mmarchini reviewed Dec 27, 2017

View reviewed changes

joyeecheung added 2 commits December 27, 2017 12:47

fs: validate path in fs.readFile

9ec700b

PR-URL: nodejs#17852 Refs: nodejs#17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

fs: validate path in fs.exists{Sync}

71396a2

PR-URL: nodejs#17852 Refs: nodejs#17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

bacriom closed this Dec 27, 2017

bacriom deleted the bacriom-branch branch December 27, 2017 18:11

eugeneo and others added 14 commits December 27, 2017 19:41

perf_hooks: refactor internals

9e5ccf0

Refactor and simplify the perf_hooks native internals. PR-URL: nodejs#17822 Reviewed-By: Anna Henningsen <anna@addaleax.net>

src: expose uv.errmap to binding

c569727

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. PR-URL: nodejs#17338 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

bacriom restored the bacriom-branch branch December 27, 2017 20:15

bacriom reopened this Dec 27, 2017

test: add test-fs-read-constructor-errors test

c560ae4

test added to extend the code coverage testing of the project

test: add test-fs-read-sync-constructor-errors test

bfae5ac

test added to extend the code coverage testing of the project

apapirovski reviewed Dec 27, 2017

View reviewed changes

bacriom and others added 2 commits December 27, 2017 14:49

Merge remote-tracking branch 'remotes/origin/bacriom-branch' into bac…

25475ad

…riom-branch

Update test-fs-read-constructor-errors.js

047b2d7

bacriom closed this Dec 28, 2017

bacriom deleted the bacriom-branch branch December 28, 2017 22:53

bacriom mentioned this pull request Dec 29, 2017

test:parameters of fs read and readsync #17910

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: added coverage tests related with method fs.read and fs.readSync #17875

test: added coverage tests related with method fs.read and fs.readSync #17875

bacriom commented Dec 27, 2017 •

edited

Loading

mmarchini Dec 27, 2017

TimothyGu Dec 27, 2017

bacriom Dec 27, 2017

mmarchini Dec 27, 2017

TimothyGu Dec 27, 2017

mmarchini Dec 27, 2017

mmarchini Dec 27, 2017

bacriom Dec 27, 2017

mmarchini commented Dec 27, 2017

apapirovski left a comment

apapirovski Dec 27, 2017 •

edited

Loading

bacriom Dec 28, 2017

apapirovski Dec 27, 2017

bacriom Dec 27, 2017

apapirovski Dec 27, 2017

mmarchini commented Dec 28, 2017

bacriom commented Dec 28, 2017

		@@ -0,0 +1,58 @@
		// Copyright Joyent, Inc. and other Node contributors.


		const expected = Buffer.from('xyz\n');

		common.expectsError(

test: added coverage tests related with method fs.read and fs.readSync #17875

test: added coverage tests related with method fs.read and fs.readSync #17875

Conversation

bacriom commented Dec 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Dec 27, 2017

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Dec 28, 2017

bacriom commented Dec 28, 2017

bacriom commented Dec 27, 2017 •

edited

Loading

apapirovski Dec 27, 2017 •

edited

Loading