Skip to content

test: add a common.debuglog function #22206

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

Closed
wants to merge 4 commits into from
Closed

test: add a common.debuglog function #22206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e69a09
test: add a common.debuglog function
SirR4T Aug 9, 2018
1c5cbb5
test: add tests for common.debuglog
SirR4T Aug 9, 2018
a9fb74a
test: incorporating suggested changes for common.debuglog
SirR4T Aug 10, 2018
93aa055
test: add docs for common.debuglog behaviour
SirR4T Aug 10, 2018
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
18 changes: 18 additions & 0 deletions test/common/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,23 @@ On non-Windows platforms, this always returns `true`.

Platform normalizes the `dd` command

### debuglog([fmt, ]msg[, msgs])
* `fmt` [<string>] Optional format string
* `msg` [<string>] The string to be printed
* `msgs` [<string>] Additional arguments to the function

Prints messages (optionally, formatted using `fmt`). Since [`util.debuglog()`]
is used internally (invoked as `util.debuglog('test')`), these messages will be
visible only when `NODE_DEBUG` environment variable is set with `test`.

```sh
// when NODE_DEBUG is not set, or doesn\'t contain `test`, prints nothing.
$ NODE_DEBUG=fs,net ./node test/fixtures/common-debuglog.js hello world
// when NODE_DEBUG contains `test`, messages are printed to `process.stderr`
$ NODE_DEBUG=test,fs,net ./node test/fixtures/common-debuglog.js hello world
TEST 89440: hello world
```

### disableCrashOnUnhandledRejection()

Removes the `process.on('unhandledRejection')` handler that crashes the process
Expand Down Expand Up @@ -802,3 +819,4 @@ implementation with tests from
[`hijackstdio.hijackStdErr()`]: #hijackstderrlistener
[`hijackstdio.hijackStdOut()`]: #hijackstdoutlistener
[internationalization]: https://github.com/nodejs/node/wiki/Intl
[`util.debuglog()`]: https://nodejs.org/api/util.html#util_util_debuglog_section
3 changes: 3 additions & 0 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,3 +804,6 @@ exports.runWithInvalidFD = function(func) {

exports.printSkipMessage('Could not generate an invalid fd');
};

const utilDebugLog = util.debuglog('test');
Copy link
Member

@joyeecheung joyeecheung Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If the intention is to get all the tests to slowly refactor to use common.debuglog, then I am -1 to this since I think something like out/Release/node test/paralell/test-*.js should still give useful output without any tweak to the environment.

In that sense at least this PR needs to also modify test.py to add NODE_DEBUG=test to

node/tools/test.py

Line 197 in f4e4ef5

print "Command: %s" % EscapeCommand(failed.command)

or even a hint in CONTRIBUTING.md (even though I believe many people don't read it through before contributing) if that's where it's going, otherwise when the test fail, the hint won't be too useful to a new contributor.

Copy link
Member

Choose a reason for hiding this comment

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

If the intention is to get all the tests to slowly refactor to use common.debuglog, then I am -1 to this since I think something like out/Release/node test/paralell/test-*.js should still give useful output without any tweak to the environment.

@joyeecheung do you mean that it should still give output for tests that fail, or that it should always give output for all tests?

Copy link
Member

@joyeecheung joyeecheung Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@gibfahn I am not sure if it's reasonable to only give output for tests that fail unless we implement this in the test runner - we test our process.exit() hooks in JS, among many things. If the hooks are broken, common.debuglog() can be broken as well - unless we setup some signal handlers in the native layer to do this, but I am not sure if that is a good idea.

exports.debuglog = utilDebugLog;
6 changes: 4 additions & 2 deletions test/common/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ const {
getBufferSources,
disableCrashOnUnhandledRejection,
getTTYfd,
runWithInvalidFD
runWithInvalidFD,
debuglog,
} = common;

export {
Expand Down Expand Up @@ -103,5 +104,6 @@ export {
getBufferSources,
disableCrashOnUnhandledRejection,
getTTYfd,
runWithInvalidFD
runWithInvalidFD,
debuglog,
};
6 changes: 6 additions & 0 deletions test/fixtures/common-debuglog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

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

// argv[0] is the execPath, argv[1] is the test script filename
common.debuglog(...process.argv.slice(2));
39 changes: 39 additions & 0 deletions test/parallel/test-common-debuglog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

require('../common');
const assert = require('assert');
const fixtures = require('../common/fixtures');
const { spawnSync } = require('child_process');

const message = 'message';

{
process.env.NODE_DEBUG = 'test';
const { stderr } = spawnSync(process.execPath, [
fixtures.path('common-debuglog.js'),
message
], { encoding: 'utf8' });

assert.ok(stderr.toString().startsWith('TEST'));
assert.ok(stderr.toString().trim().endsWith(message));
}

{
delete process.env.NODE_DEBUG;
const { stderr } = spawnSync(process.execPath, [
fixtures.path('common-debuglog.js'),
message
], { encoding: 'utf8' });

assert.strictEqual(stderr.toString().trim(), '');
}

{
process.env.NODE_DEBUG = 'fs';
const { stderr } = spawnSync(process.execPath, [
fixtures.path('common-debuglog.js'),
message
], { encoding: 'utf8' });

assert.strictEqual(stderr.toString().trim(), '');
}