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 debugging output to test-net-listen-after-destroy-stdin #31698

Closed
wants to merge 0 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 8, 2020

The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 8, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 8, 2020

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2020
const server = net.createServer(common.mustCall((socket) => {
console.log('accepted...');
socket.end(common.mustCall(() => { console.log('finished...'); }));
server.close(common.mustCall(() => { console.log('closed'); }));
Copy link
Member

Choose a reason for hiding this comment

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

For the QUIC tests, I've started using the pattern of creating a debuglog for the tests attached to the test category, e.g.

$ NODE_DEBUG=test ./node test/parallel/test-whatever

Then in the tests using

const { debuglog } = require('util');
const debug = debuglog('test');
// ...
debug('some message');

This works very well to get the appropriate debug output, and very clearly separates purely debug statements from stuff that's part of the test itself. I'd much prefer that pattern over using console.log()

It's not just for style either, I have run into cases before where the use of console.log ends up altering the timing of the test just enough to impact the result. It's definitely not common but it can certainly happen.

Copy link
Member Author

@Trott Trott Feb 10, 2020

Choose a reason for hiding this comment

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

For the QUIC tests, I've started using the pattern of creating a debuglog for the tests

This won't be as effective in cases like this where the test seems to fail only in CI and very infrequently. But yeah, it's preferable. This test already has some console.log() statements in it so we'd have to move it to this new pattern.

It's not just for style either, I have run into cases before where the use of console.log ends up altering the timing of the test just enough to impact the result. It's definitely not common but it can certainly happen.

Yeah, I've definitely come across that too and that's the most compelling (to me) argument for avoiding console in tests.

Also, once the cause of test failures is found, it's often possible to find another way to have surfaced it that wouldn't have involved console.

Lots to consider here and a lot of tests to refactor if we move to a new pattern.

Trott added a commit to Trott/io.js that referenced this pull request Feb 10, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: nodejs#31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott Trott closed this Feb 10, 2020
@Trott
Copy link
Member Author

Trott commented Feb 10, 2020

Landed in 81af195

@Trott Trott deleted the debug-info branch February 10, 2020 19:04
codebytere pushed a commit that referenced this pull request Feb 17, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: #31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
BethGriggs pushed a commit that referenced this pull request Feb 25, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: #31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Mar 12, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: #31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: #31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
The test failed in CI once with a timeout but there is insufficient
information to further debug. Add additional debugging information.

Refactored callbacks to be arrow functions, since that seems to be the
direction we're moving.

PR-URL: #31698
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants