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.

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

test: cleanup test-stdout-close-catch.js #10006

Closed
wants to merge 2 commits into from
Closed

test: cleanup test-stdout-close-catch.js #10006

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c755afa
test: cleanup test-stdout-close-catch.js
furnox Dec 1, 2016
ab0a3b2
test: fixed mustCall default, coding style fixes
Dec 14, 2016
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
29 changes: 15 additions & 14 deletions test/parallel/test-stdout-close-catch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,28 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var path = require('path');
var child_process = require('child_process');
const common = require('../common');
const assert = require('assert');
const path = require('path');
const child_process = require('child_process');

var testScript = path.join(common.fixturesDir, 'catch-stdout-error.js');
const testScript = path.join(common.fixturesDir, 'catch-stdout-error.js');

var cmd = JSON.stringify(process.execPath) + ' ' +
JSON.stringify(testScript) + ' | ' +
JSON.stringify(process.execPath) + ' ' +
'-pe "process.stdin.on(\'data\' , () => process.exit(1))"';
const cmd = JSON.stringify(process.execPath) + ' ' +
JSON.stringify(testScript) + ' | ' +
JSON.stringify(process.execPath) + ' ' +
'-pe "process.stdin.on(\'data\' , () => process.exit(1))"';

var child = child_process.exec(cmd);
var output = '';
var outputExpect = { 'code': 'EPIPE',
const child = child_process.exec(cmd);
let output = '';
const outputExpect = { 'code': 'EPIPE',
'errno': 'EPIPE',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these lines should probably be indented as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig - thanks for looking at this.
I'm perfectly happy to align to any coding style, however, what is the style to be? make lint didn't make a peep over the alignment of these lines. My own style would be to indent these lines like so:

const outputExpect = {
  'code': 'EPIPE',
  'errno': 'EPIPE',
  'syscall': 'write'
};

but I don't know if that's the indentation you'd like to see.

Copy link
Member

Choose a reason for hiding this comment

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

@furnox What you have as your preference there is fine. (That's my preference too but it's not universal on the project. Basically, as long as the property names are lined up, that's the main thing.)

When we get over a couple of issues that are preventing us from updating ESLint, we will have more strict linting for indentation. Thanks for bearing with us until then.

Also, while you're in here editing: it seems like the quotation marks around the property names are unnecessary. If I'm not mistaken, it could be written as:

const outputExpect = {
  code: 'EPIPE',
  errno: 'EPIPE',
  syscall: 'write'
};

If there's a good reason for the quotation marks, then I guess keep them, but if not, perhaps take this opportunity to remove them.

'syscall': 'write' };

child.stderr.on('data', function(c) {
output += c;
});

child.on('close', function(code) {

child.on('close', common.mustCall(function(code) {
try {
output = JSON.parse(output);
} catch (er) {
Expand All @@ -31,4 +32,4 @@ child.on('close', function(code) {

assert.deepStrictEqual(output, outputExpect);
console.log('ok');
});
}), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed. 1 is the default for common.mustCall().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll fix that.