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

Jump to bottom

process: fix error message of hrtime() #13739

Closed
wants to merge 3 commits into from
Closed

process: fix error message of hrtime() #13739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb284bb
process: fix error message of hrtime()
tniessen Jun 17, 2017
2bdc7c4
Add actual argument
tniessen Jun 18, 2017
07b1833
Propose ERR_INVALID_ARRAY_LENGTH
tniessen Jun 18, 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
3 changes: 1 addition & 2 deletions lib/internal/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ function setup_hrtime() {
const needsBorrow = nsec < 0;
return [needsBorrow ? sec - 1 : sec, needsBorrow ? nsec + 1e9 : nsec];
}
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'process.hrtime()', 'Array');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'time', 'Array');
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the actual argument now that we are here?

Copy link
Member Author

@tniessen tniessen Jun 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This comes with a problem. After adding the actual argument, the error message looks like 'The "time" argument must be of type Array. Received type object' for arrays with an invalid size. The correct message would be something like 'The "time" array must have a length of 2. Received 3'. Any ideas for a solution? Maybe a new error code ERR_INVALID_ARRAY_LENGTH?

Copy link
Member

Choose a reason for hiding this comment

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

This could be improved by rewriting the statements to something like:

if (!Array.isArray(time)) {
  throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'time', 'Array', time);
}
if (time.length !== 2) {
  throw new errors.TypeError('ERR_TYPE_X', 'time', 2, time);
}
const sec = (hrValues[0] * 0x100000000 + hrValues[1]) - time[0];
const nsec = hrValues[2] - time[1];
const needsBorrow = nsec < 0;
return [needsBorrow ? sec - 1 : sec, needsBorrow ? nsec + 1e9 : nsec];

Copy link
Member Author

Choose a reason for hiding this comment

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

@BridgeAR I know how it can be rewritten, that is not the point (I am actually already running tests against a fix). This is more of a design decision whether we want a code ERR_INVALID_ARRAY_LENGTH or such.

}

return [
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-process-hrtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ validateTuple(process.hrtime(tuple));
const invalidHrtimeArgument = common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "process.hrtime()" argument must be of type Array'
message: 'The "time" argument must be of type Array'
});

// test that only an Array may be passed to process.hrtime()
Expand Down