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

util: adding a unit test to cover invalid code check in util.deprecate method #16305

Closed
wants to merge 1 commit into from
Closed

Conversation

akaila
Copy link
Contributor

@akaila akaila commented Oct 18, 2017

Added a unit test that verifies assertion if util.deprecate is called with a non-string code parameter.

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)

util.deprecate

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 18, 2017
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit

common.expectsError(
() => {
util.deprecate(() => {}, "message", 123);
},
Copy link
Member

Choose a reason for hiding this comment

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

nit: There's only one statement inside the function.
This can be converted to:

() => util.deprecate(() => {}, "message", 123)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and updated

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @akaila! This looks well on the way but there are a few nits that could be updated.

Also, I would recommend running the JS linter (make lint-js) and fixing any problematic things that it flags. See our contributing guide for more info: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#the-process-of-making-changes

Please let me know if any of the info in there is unclear or doesn't work for you.

@@ -0,0 +1,17 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Please use single quotes for strings throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah my local workspace had prettier enabled which I have disabled now. Updated PR.
Thanks !

[1, true, false, null, {}].forEach(notString => {
common.expectsError(
() => {
util.deprecate(() => {}, "message", 123);
Copy link
Member

Choose a reason for hiding this comment

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

I think 123 was meant to be notString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Fixed.

{
code: "ERR_INVALID_ARG_TYPE",
type: global.TypeError,
message: `The "code" argument must be of type string`
Copy link
Member

Choose a reason for hiding this comment

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

This can be a plain single-quoted string instead of a template string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

},
{
code: "ERR_INVALID_ARG_TYPE",
type: global.TypeError,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please remove the global. part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@apapirovski
Copy link
Member

@akaila To make it easier to land this, you could also update your commit message to be 50 chars or less. If you need to provide more info, you can put in two new lines and then write the rest of it. See the specs for valid commit messages here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines

Or whoever lands this can also do it later. :)

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Oct 19, 2017
@apapirovski
Copy link
Member

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Looks like the linter caught one more issue. If you could fix that and potentially adjust the commit message as per above, that would be amazing! Thanks!

const common = require('../common');
const util = require('util');

[1, true, false, null, {}].forEach(notString => {
Copy link
Member

Choose a reason for hiding this comment

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

Caught by linter: this needs parens around notString to be (notString).

Copy link
Member

Choose a reason for hiding this comment

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

@akaila You can run linter just for Javascript files using make lint-js

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 trying now.

const util = require('util');

[1, true, false, null, {}].forEach(notString => {
common.expectsError(() => util.deprecate(() => { }, 'message', notString), {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you mind removing the extra space inside { }? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and I ran linter to ensure things look ok:

make lint-js
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md
benchmark doc lib test tools

@apapirovski
Copy link
Member

@trivikr
Copy link
Member

trivikr commented Oct 19, 2017

About update of diff in PR taking time, I've logged the issue with Github at isaacs/github#1098

@akaila
Copy link
Contributor Author

akaila commented Oct 19, 2017

Looking at the output, no idea why build should fail:
>> Job status: [node-test-commit-aix] the 'build only if scm changes' feature is disabled.
Starting build job node-test-commit-aix.
Finished Build : #12781 of Job : node-test-linter with status : SUCCESS
Finished Build : #9555 of Job : node-test-commit-linuxone with status : SUCCESS
Finished Build : #12380 of Job : node-test-commit-plinux with status : SUCCESS
Finished Build : #13317 of Job : node-test-commit-osx with status : SUCCESS
Finished Build : #12380 of Job : node-test-commit-smartos with status : SUCCESS
Finished Build : #11848 of Job : node-test-commit-linux-fips with status : SUCCESS
Finished Build : #11864 of Job : node-test-commit-arm-fanned with status : SUCCESS
Finished Build : #12508 of Job : node-test-commit-freebsd with status : FAILURE
Finished Build : #12678 of Job : node-test-commit-windows-fanned with status : UNSTABLE
Finished Build : #13357 of Job : node-test-commit-linux with status : FAILURE

Finished Build : #9507 of Job : node-test-commit-aix with status : SUCCESS
Notifying upstream projects of job completion
Finished: FAILURE

@apapirovski
Copy link
Member

CI was fine, we just have some failing tests at the moment which are being fixed elsewhere.

@akaila
Copy link
Contributor Author

akaila commented Oct 19, 2017

I see. Do you have to override the failure to have it merge into master? Sorry if I am asking too many questions being a newbie :)

@apapirovski
Copy link
Member

@akaila Not at all, that's what we're here for :) Merging is just at the discretion of collaborators. As long as a pull request is approved (and not a single collaborator objects) and no CI failures are related to it, it can be merged. (Oh and at least 48 hours have to pass.)

You can read more about the process here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-10-landing (the whole doc is pretty useful in general)

apapirovski pushed a commit that referenced this pull request Oct 22, 2017
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@apapirovski
Copy link
Member

Landed in fe8297c

@akaila Congrats on becoming a Contributor!

@akaila
Copy link
Contributor Author

akaila commented Oct 22, 2017

Thanks !

@MylesBorins
Copy link
Contributor

The new tests are failing on 8.x

Would someone be willing to manually backport?

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

What does backport mean here? I tested this on current master (8.7 I believe) and it passed. If you can give me an example of backport, I can definitely do it.
Thanks

@trivikr
Copy link
Member

trivikr commented Oct 24, 2017

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

Added backport PR. Please approve

@MylesBorins
Copy link
Contributor

in retrospect this PR requires changes to using error codes that we cannot backport to 8.x as they are semver major. I am marking as don't land

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Test for invalid argument types passed to code on util.deprecate.

PR-URL: nodejs/node#16305
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Backport-PR-URL: #16430
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Backport-PR-URL: #16430
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Test for invalid argument types passed to code on util.deprecate.

PR-URL: nodejs/node#16305
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants