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.

Sign up for GitHub

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: added coverage tests related with method fs.read and fs.readSync #17875

Closed
wants to merge 24 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
37c97b1
test: add test-fs-read-constructor-errors test
bacriom Dec 27, 2017
d7326b2
test: add test-fs-readSync-constructor-errors test
bacriom Dec 27, 2017
bd3aa3e
test: add test-fs-read-constructor-errors test
bacriom Dec 27, 2017
5e59b4f
test: add test-fs-readSync-constructor-errors test
bacriom Dec 27, 2017
9ec700b
fs: validate path in fs.readFile
joyeecheung Dec 24, 2017
71396a2
fs: validate path in fs.exists{Sync}
joyeecheung Dec 24, 2017
c0e9cde
inspector: make Coverity happy
eugeneo Dec 13, 2017
b0dd43c
tools: fix AttributeError: __exit__ on Python 2.6
dkasyanov Dec 13, 2017
6c0da34
http: add rawPacket in err of `clientError` event
XadillaX Dec 14, 2017
9e5ccf0
perf_hooks: refactor internals
jasnell Dec 21, 2017
46510f5
tls: fix SNICallback without .server option
addaleax Dec 23, 2017
b343671
test: refactor test-tls-securepair-fiftharg
addaleax Dec 23, 2017
cb3de88
crypto: add ocsp_request ClientHelloParser::Reset
danbev Dec 11, 2017
df30fd5
buffer: optimize readDouble and readFloat methods
bnoordhuis Dec 20, 2017
83e5215
async_hooks: use typed array stack as fast path
addaleax Dec 19, 2017
24c71fb
test: make test-tls-invoke-queued use public API
addaleax Dec 25, 2017
c569727
src: expose uv.errmap to binding
joyeecheung Dec 25, 2017
6ca10de
fs: simplify the error context collection in C++
joyeecheung Nov 27, 2017
9f122e3
fs: throw fs.close errors in JS
joyeecheung Nov 29, 2017
d3ac18a
lib: migrate _http_outgoing.js's remaining errors
b0yfriend Dec 23, 2017
c560ae4
test: add test-fs-read-constructor-errors test
bacriom Dec 27, 2017
bfae5ac
test: add test-fs-read-sync-constructor-errors test
bacriom Dec 27, 2017
25475ad
Merge remote-tracking branch 'remotes/origin/bacriom-branch' into bac…
bacriom Dec 27, 2017
047b2d7
Update test-fs-read-constructor-errors.js
bacriom Dec 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions test/parallel/test-fs-read-constructor-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I'll fix it

//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
const common = require('../common');
const fixtures = require('../common/fixtures');
const fs = require('fs');
const filepath = fixtures.path('x.txt');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add empty line after all the require statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty lines added

const fd = fs.openSync(filepath, 'r');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that. Thanks!


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

common.expectsError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fs.read.bind(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
0,
common.mustNotCall()),
{ code: 'ERR_INVALID_ARG_TYPE', type: TypeError });
Copy link
Member

@apapirovski apapirovski Dec 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


common.expectsError(
fs.read.bind(undefined,
fd,
Buffer.allocUnsafe(expected.length),
-1,
expected.length,
0,
common.mustNotCall()),
{ code: 'ERR_OUT_OF_RANGE', type: RangeError });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


common.expectsError(
fs.read.bind(undefined,
fd,
Buffer.allocUnsafe(expected.length),
0,
-1,
0,
common.mustNotCall()),
{ code: 'ERR_OUT_OF_RANGE', type: RangeError });
55 changes: 55 additions & 0 deletions test/parallel/test-fs-readSyc-constructor-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

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

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

common.expectsError(
fs.readSync.bind(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
0),
{ code: 'ERR_INVALID_ARG_TYPE', type: TypeError });

common.expectsError(
fs.readSync.bind(undefined,
fd,
Buffer.allocUnsafe(expected.length),
-1,
expected.length,
0),
{ code: 'ERR_OUT_OF_RANGE', type: RangeError });

common.expectsError(
fs.readSync.bind(undefined,
1,
Buffer.allocUnsafe(expected.length),
0,
-1,
0),
{ code: 'ERR_OUT_OF_RANGE', type: RangeError });