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,console: known_issues test for issue 831 #5639

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 10, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

console, test

Description of change

This adds a test for the issue described in
#831.

I've added some metadata fields that I would personally find useful in
these tests.

Refs: #831

@Trott Trott added test Issues and PRs related to the tests. console Issues and PRs related to the console subsystem. lts-watch-v4.x labels Mar 10, 2016
@Trott Trott mentioned this pull request Mar 10, 2016
@Trott
Copy link
Member Author

Trott commented Mar 10, 2016

This is probably fragile, dependent on net.js always using the presence of the _handle property to determine if the socket has been closed or not. It does that today but I imagine there's no guarantee it will continue to work that way tomorrow. But I'm not sure there's a better way to have the problem fire reliably on CI. Maybe there's some child_process sorcery I'm unaware of?

@Trott
Copy link
Member Author

Trott commented Mar 10, 2016

The _handle checking in net.js that I refer to above is at

node/lib/net.js

Lines 645 to 648 in a498492

if (!this._handle) {
this._destroy(new Error('This socket is closed'), cb);
return false;
}

@Fishrock123
Copy link
Contributor

@Trott I don't think that issue link is correct?

@Trott
Copy link
Member Author

Trott commented Mar 10, 2016

@Fishrock123 Argh. You are correct. The correct issue link is #831. Will fix above too...

@Trott Trott changed the title test,console: known_issues test for issue 548 test,console: known_issues test for issue 831 Mar 10, 2016
This adds a test for the issue described in
nodejs#831.

I've added some metadata fields that I would personally find useful in
these tests.

Refs: nodejs#831
@cjihrig
Copy link
Contributor

cjihrig commented Mar 10, 2016

I believe #947 is the same issue, or closely related. I have this failing test, which I haven't opened a PR for:

'use strict';
// Refs: https://github.com/nodejs/node/issues/947
const common = require('./test/common');
const assert = require('assert');
const cp = require('child_process');

if (process.argv[2] === 'child') {
  process.on('message', common.mustCall((msg) => {
    assert.strictEqual(msg, 'go');
    console.log('logging should not cause a crash');
    process.disconnect();
  }));
} else {
  const child = cp.fork(__filename, ['child'], {silent: true});

  child.on('close', common.mustCall((exitCode, signal) => {
    assert.strictEqual(exitCode, 0);
    assert.strictEqual(signal, null);
  }));

  child.stdout.destroy();
  child.send('go');
}

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

Test LGTM, will be good to get this fixed.

@Trott
Copy link
Member Author

Trott commented Mar 11, 2016

@cjihrig That sure seems like the same thing and a more robust test. And I see you've included it in #5653. So I'm going to close this.

@Trott Trott closed this Mar 11, 2016
@Trott Trott deleted the test-548 branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants