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

lib: add 'pid' prefix in `internal/util` #3878

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
9 participants
@JungMinu
Copy link
Member

commented Nov 17, 2015

This PR improves prefix in util that we've agreed on #3833
(separate PR for javascript in lib, and the original PR will be updated to move the printing function to C++ in src directly)

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2015

LGTM

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

won't this break some tests?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2015

I would imagine for any tests checking for the old string. @JungMinu did you run the tests locally with your changes?

@mscdex mscdex added the util label Nov 17, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

LGTM if CI is green

Update: ... which, it's not ... sadly. @JungMinu , can you please take a look at the failing tests and update those here as well. Thank you!

@evanlucas

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

Yea, test-deprecation-flags.js is failing

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@JungMinu ... I'll take a look in the morning. Could you post the failure that you're seeing, however?

@martfors

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2015

Looking at the test message I'm guessing this is because you haven't updated the test to use your new prefix instead of the hardcoded one when comparing the two warnings.

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2015

@jasnell I found the solution, Thanks :)

@targos

View changes

test/sequential/test-deprecation-flags.js Outdated
@@ -3,6 +3,7 @@ var common = require('../common');
var assert = require('assert');
var execFile = require('child_process').execFile;
var depmod = require.resolve('../fixtures/deprecated.js');
const prefix = `(${process.release.name}:${process.pid + 1}) `;

This comment has been minimized.

Copy link
@targos

targos Nov 18, 2015

Member

are we sure that the child process' pid will always be current+1 ?

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 18, 2015

Author Member

@targos would you recommend alternative to fix this? :)

This comment has been minimized.

Copy link
@targos

targos Nov 18, 2015

Member

I would use a RegExp to match stderr

This comment has been minimized.

Copy link
@silverwind

silverwind Nov 18, 2015

Contributor

Agree, something like assert(/util\.debug is deprecated/.test(stderr)) could suffice, but you could also use the RegExp constructor to match the whole line.

This comment has been minimized.

Copy link
@JungMinu

JungMinu Nov 18, 2015

Author Member

@targos @silverwind Yup, I will update soon, Thanks :)

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2015

@jasnell @silverwind @cjihrig Thanks, finished with RegExp:)

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

@cjihrig

View changes

test/sequential/test-deprecation-flags.js Outdated
assert.equal(stderr, '(node) util.debug is deprecated. Use console.error ' +
'instead.\nDEBUG: This is deprecated\n');
const stderrResult = stderr.replace(/ *\([^)]*\) */g, '');
assert.equal(stderrResult, 'util.debug is deprecated. Use console.error ' +

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 19, 2015

Contributor

Sorry, just looking at this again. Couldn't you just assert(/util\.debug is deprecated.../.test(stderr));

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2015

@cjihrig Sure, updated :)

@cjihrig

View changes

test/sequential/test-deprecation-flags.js Outdated
@@ -16,8 +16,7 @@ execFile(node, normal, function(er, stdout, stderr) {
console.error('normal: show deprecation warning');
assert.equal(er, null);
assert.equal(stdout, '');
assert.equal(stderr, '(node) util.debug is deprecated. Use console.error ' +
'instead.\nDEBUG: This is deprecated\n');
assert(/util\.debug is deprecated.../.test(stderr));

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 19, 2015

Contributor

Heh. I put ... in my original comment to avoid typing out the entire message. Sorry for the mix up.

Funny, this should actually still work, since . matches a single character. I don't think it's necessary to try to match the entire deprecation message verbatim. Drop the ... and this LGTM. Sorry again.

lib: add 'pid' prefix in `internal/util`
This PR improves `prefix` in `util` that we've agreed on #3833
(separate code for javascript to move the printing function to C++ directly)

lib: add 'pid' prefix in `internal/util`
@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2015

@cjihrig No problem, finished :)

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

Seeing one possibly flaky test error in CI (see https://ci.nodejs.org/job/node-test-commit-linux/1271/nodes=fedora21/console). Assuming that's an unrelated error, LGTM

@targos

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

LGTM

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

LGTM (still)

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

will get this landed!

jasnell added a commit that referenced this pull request Nov 20, 2015

lib: add 'pid' prefix in `internal/util`
This PR improves `prefix` in `util` that we've agreed on
#3833
(separate code for javascript to move the printing function
to C++ directly)

PR-URL: #3878
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

Landed in d01eb68

@jasnell jasnell closed this Nov 20, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

@JungMinu ... I know that #3833 is still pending. Hopefully we can close on that one soon

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2015

@jasnell Yes, I will finish that PR tomorrow :)

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

thank you @JungMinu !

@rvagg

This comment has been minimized.

Copy link
Member

commented Dec 5, 2015

I believe this should be semver-major like the other changes to error messages. Tagging it as such will ensure it won't slip in to v5.x (like it just did while I was working through the list).

Please speak up if you disagree or if you think I'm reading this wrong.

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 5, 2015

Works for me
On Dec 4, 2015 8:47 PM, "Rod Vagg" notifications@github.com wrote:

I believe this should be semver-major like the other changes to error
messages. Tagging it as such will ensure it won't slip in to v5.x (like it
just did while I was working through the list).

Please speak up if you disagree or if you think I'm reading this wrong.


Reply to this email directly or view it on GitHub
#3878 (comment).

@JungMinu

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2015

@rvagg Sounds good to me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.