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: add test coverage for fs.truncate #23620

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

christian-bromann
Copy link
Contributor

@christian-bromann christian-bromann commented Oct 12, 2018

Add test to check:

  • for null as len parameter
  • if error is propagated into callback if file doesn't exist
  • if an error is actually thrown if len is not a number

I think it is safe to say that we could remove:

  } else if (len === undefined) {
    len = 0;
  }

because there is an validateInteger(len, 'len'); right after. Another option would be to just set len to 0 for all cases it is not a Number. What do you think?

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@refack
Copy link
Contributor

refack commented Oct 12, 2018

Hello @christian-bromann and thank you for the contribution 🥇

@refack refack added the fs Issues and PRs related to the fs subsystem / file system. label Oct 12, 2018
@refack
Copy link
Contributor

refack commented Oct 12, 2018

the following:

node/lib/fs.js

Lines 617 to 622 in bcbb937

if (typeof len === 'function') {
callback = len;
len = 0;
} else if (len === undefined) {
len = 0;
}

Is a sort of JS idiom to handle optional function arguments. IMHO it could stay as as.

@BridgeAR BridgeAR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@trivikr
Copy link
Member

trivikr commented Oct 13, 2018

CI (pending): https://ci.nodejs.org/job/node-test-pull-request/1​77​96/

{
const file8 = path.resolve(tmp, 'truncate-file-8.txt');
fs.truncate(file8, 0, common.mustCall(function(err) {
assert.ok(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are expecting an error, its bettor to explicitly assert the error. But, I think you meant to write assert.ifError here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this passes CI, I'm assuming it fails with code ENOENT.
Assertion should have been near

// ftruncate
{
const validateError = (err) => {
assert.strictEqual(err.syscall, 'ftruncate');
// Could be EBADF or EINVAL, depending on the platform
if (err.code === 'EBADF') {
assert.strictEqual(err.message, 'EBADF: bad file descriptor, ftruncate');
assert.strictEqual(err.errno, UV_EBADF);
} else {
assert.strictEqual(err.message, 'EINVAL: invalid argument, ftruncate');
assert.strictEqual(err.errno, UV_EINVAL);
assert.strictEqual(err.code, 'EINVAL');
}
return true;
};
common.runWithInvalidFD((fd) => {
fs.ftruncate(fd, 4, common.mustCall(validateError));
assert.throws(
() => fs.ftruncateSync(fd, 4),
validateError
);
});
}

But since it's not, I agree it should be here.

@christian-bromann
Copy link
Contributor Author

@thefourtheye added explicit check for error

@refack
Copy link
Contributor

refack commented Oct 15, 2018

}

{
const file8 = path.resolve(tmp, 'truncate-file-8.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: maybe use some more descriptive filename that will indicate that this file is indeed non-existent and make sure that nobody creates it accidentally (via other test), perhaps: 'non-existent-truncate-file.txt'?

code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError [ERR_INVALID_ARG_TYPE]',
message: 'The "len" argument must be of type number. ' +
`Received type ${typeof input}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this line should be aligned to the '

@christian-bromann
Copy link
Contributor Author

It says Missing subsystem. which is odd because I started the commit with test:...

@addaleax
Copy link
Member

/cc @Trott

@christian-bromann It’s not the first time that that happened, but I think we thought we had fixed it. I don’t think you need to do anything here, though.

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2018
@christian-bromann
Copy link
Contributor Author

Maybe to add context to this situation: I used git commit --amend for all changes that happen after I made the initial PR

Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: nodejs#23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@refack refack merged commit 74f854e into nodejs:master Oct 15, 2018
@refack
Copy link
Contributor

refack commented Oct 15, 2018

Landed in 74f854e 🎉
Congratulations @christian-bromann on GitHub promoting you form
image
to
image
Hope we see you contributing more in the future 😉

@christian-bromann christian-bromann deleted the truncate-test-coverage branch October 15, 2018 23:05
@christian-bromann
Copy link
Contributor Author

Hope we see you contributing more in the future 😉

Will try my best. Thanks for the support. I really enjoyed the code and learn session at Node+JS Interactive.

jasnell pushed a commit that referenced this pull request Oct 17, 2018
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: #23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: #23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: #23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: #23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Add test to check:
- for `null` as len parameter
- if error is propagated into callback if file doesn't exist
- if an error is actually thrown if len is not a number

PR-URL: #23620
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants