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: removed unnecessary Buffer import #13860

Closed
wants to merge 5 commits into from
Closed

test: removed unnecessary Buffer import #13860

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b612f6
test: removed unnecessary Buffer import
swinston1000 Jun 21, 2017
e903a96
Update test-sendfd.js
swinston1000 Jun 21, 2017
daa9328
reverted code
swinston1000 Jun 21, 2017
ac96ae9
reverted style change
swinston1000 Jun 21, 2017
9e2e60d
reverted style change
swinston1000 Jun 21, 2017
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
19 changes: 9 additions & 10 deletions test/disabled/test-sendfd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// The above copnotice and this permission notice shall be included
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please revert this change?

Copy link
Member

Choose a reason for hiding this comment

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

This looks like an accident :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'twas, now reverted

Copy link
Contributor

Choose a reason for hiding this comment

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

Unintentional?
(beware of over eager IDEs. If you're using WebStorm, enable ESlint with our .eslint.yaml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes completely, do you want me to close and fix all of this?

// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
Expand Down Expand Up @@ -50,21 +50,20 @@
const common = require('../common');
const assert = require('assert');

const buffer = require('buffer');
const child_process = require('child_process');
const fs = require('fs');
const net = require('net');
var netBinding = process.binding('net');
const path = require('path');

var DATA = {
'ppid' : process.pid,
'ord' : 0
'ppid': process.pid,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unrelated. You may do it in this PR if you want, but you need to reframe the commit message and the PR title differently then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Something like:

test: removed unnecessary require and style fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the change so won't change title
Thanks for pointing it out
Lesson learned not to have auto-beautify when working on open-source projects :-)

Copy link
Member

Choose a reason for hiding this comment

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

Why the formatting changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjamingr

Auto beautifier - and the tests didn't pick it up.
I reverted all changes - should all be good now and know in future to turn that off or use your guide!

Sorry and thanks for this evening!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now reverted :-)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

'ord': 0
};

var SOCK_PATH = path.join(__dirname,
'..',
path.basename(__filename, '.js') + '.sock');
'..',
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated style change.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we either chop (put every arg in it's own line)
Or align to call ( (as it was).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :-)

path.basename(__filename, '.js') + '.sock');

var logChild = function(d) {
if (typeof d == 'object') {
Expand Down Expand Up @@ -112,7 +111,7 @@ var srv = net.createServer(function(s) {
var str = JSON.stringify(DATA) + '\n';

DATA.ord = DATA.ord + 1;
var buf = buffer.Buffer.allocUnsafe(str.length);
var buf = Buffer.allocUnsafe(str.length);
buf.write(JSON.stringify(DATA) + '\n', 'utf8');

s.write(str, 'utf8', pipeFDs[1]);
Expand All @@ -127,9 +126,9 @@ var srv = net.createServer(function(s) {
srv.listen(SOCK_PATH);

// Spawn a child running test/fixtures/recvfd.js
var cp = child_process.spawn(process.argv[0],
[path.join(common.fixturesDir, 'recvfd.js'),
SOCK_PATH]);
var cp = child_process.spawn(process.argv[0], [path.join(common.fixturesDir, 'recvfd.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated style change. Besides that, it will fail linting. Please don't ignore the friendly reminder to run make -j4 test you can see in the pull request template :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, and it failed first time
I changed, it passed
Not sure how this happened
Fixing now, sorry :-)

SOCK_PATH
]);

cp.stdout.on('data', logChild);
cp.stderr.on('data', logChild);
Expand Down
1 change: 0 additions & 1 deletion test/internet/test-dgram-broadcast-multi-process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ const assert = require('assert');
const dgram = require('dgram');
const util = require('util');
const networkInterfaces = require('os').networkInterfaces();
const Buffer = require('buffer').Buffer;
const fork = require('child_process').fork;
const LOCAL_BROADCAST_HOST = '255.255.255.255';
const TIMEOUT = common.platformTimeout(5000);
Expand Down
1 change: 0 additions & 1 deletion test/pummel/test-https-no-reader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ if (!common.hasCrypto) {
}
const https = require('https');

const Buffer = require('buffer').Buffer;
const fs = require('fs');
const path = require('path');

Expand Down