Skip to content

test: replace function with arrow function #17345

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
Leko wants to merge 5 commits into from
Closed

test: replace function with arrow function #17345

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
79d1fbc
test: replace function with arrow function
Leko Nov 27, 2017
c0e7ee8
test: replace arrow function with shorter property syntax
Leko Nov 27, 2017
6dccb02
test: revert changes in test-whatwg-url-searchparams-getall
Leko Nov 27, 2017
0739723
test: replace this with null
Leko Nov 27, 2017
fa4eb72
test: revert to normal function
Leko Nov 27, 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
18 changes: 9 additions & 9 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const a = assert;

function makeBlock(f) {
const args = Array.prototype.slice.call(arguments, 1);
return function() {
return () => {
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Nov 27, 2017

Choose a reason for hiding this comment

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

This makes this lexical. Is it OK here?

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 27, 2017

Choose a reason for hiding this comment

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

Thank you for your review.

I think that it is OK because test passed locally.
It is not called with neither .call nor .apply in this file.
This code equivalent to:

  return () => {
    return f.apply(null, args);
  };

Should I replace this with null ?

Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Nov 27, 2017

Choose a reason for hiding this comment

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

Maybe, for a clarity, but let's see what others think)

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 27, 2017

Choose a reason for hiding this comment

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

I got it.
Please let us know what others think.

return f.apply(this, args);
};
}
Expand Down Expand Up @@ -183,7 +183,7 @@ assert.doesNotThrow(makeBlock(a.deepEqual, a1, a2));

// having an identical prototype property
const nbRoot = {
toString: function() { return `${this.first} ${this.last}`; }
toString: () => { return `${this.first} ${this.last}`; }
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Nov 27, 2017

Choose a reason for hiding this comment

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

The same.

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 27, 2017

Choose a reason for hiding this comment

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

Thank you for your review.
I was mistaken.

I'll fix this problem with like:

  toString() { return `${this.first} ${this.last}`; }

};

function nameBuilder(first, last) {
Expand Down Expand Up @@ -458,10 +458,10 @@ assert.throws(makeBlock(thrower, TypeError));
'a.doesNotThrow is not catching type matching errors');
}

assert.throws(function() { assert.ifError(new Error('test error')); },
assert.throws(() => { assert.ifError(new Error('test error')); },
/^Error: test error$/);
assert.doesNotThrow(function() { assert.ifError(null); });
assert.doesNotThrow(function() { assert.ifError(); });
assert.doesNotThrow(() => { assert.ifError(null); });
assert.doesNotThrow(() => { assert.ifError(); });

assert.throws(() => {
assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
Expand Down Expand Up @@ -501,7 +501,7 @@ assert.throws(() => {
let threw = false;
try {
assert.throws(
function() {
() => {
throw ({}); // eslint-disable-line no-throw-literal
},
Array
Expand All @@ -516,7 +516,7 @@ assert.throws(() => {
a.throws(makeBlock(thrower, TypeError), /\[object Object\]/);

// use a fn to validate error object
a.throws(makeBlock(thrower, TypeError), function(err) {
a.throws(makeBlock(thrower, TypeError), (err) => {
if ((err instanceof TypeError) && /\[object Object\]/.test(err)) {
return true;
}
Expand Down Expand Up @@ -607,7 +607,7 @@ testAssertionMessage([1, 2, 3], '[ 1, 2, 3 ]');
testAssertionMessage(/a/, '/a/');
testAssertionMessage(/abc/gim, '/abc/gim');
testAssertionMessage(function f() {}, '[Function: f]');
testAssertionMessage(function() {}, '[Function]');
testAssertionMessage(() => {}, '[Function]');
Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 27, 2017

Choose a reason for hiding this comment

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

I think that it should be reverted because function() {} and () => {} are should be tested both.
This PR changes only syntax, should not change test case.
So it better of separated by another PR.

What do you think about it ?

Copy link
Copy Markdown
Member

@yosuke-furukawa yosuke-furukawa Nov 27, 2017

Choose a reason for hiding this comment

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

In my humble opinion, this check would be better to not replace arrow function. Because assert message also should have '[Function]' when function() {} is input.
We would be better to check both function style arrow function and normal function.

testAssertionMessage({}, '{}');
testAssertionMessage(circular, '{ y: 1, x: [Circular] }');
testAssertionMessage({ a: undefined, b: null }, '{ a: undefined, b: null }');
Expand All @@ -619,7 +619,7 @@ testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity },
let threw = false;
try {
// eslint-disable-next-line no-restricted-syntax
assert.throws(function() {
assert.throws(() => {
assert.ifError(null);
});
} catch (e) {
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-domain-top-level-error-handler-clears-stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ const domain = require('domain');
*/
const d = domain.create();

d.on('error', common.mustCall(function() {
process.nextTick(function() {
d.on('error', common.mustCall(() => {
process.nextTick(() => {
// Scheduling a callback with process.nextTick will enter a _new_ domain,
// and the callback will be called after the domain that handled the error
// was exited. So there should be only one domain on the domains stack if
Expand All @@ -29,6 +29,6 @@ d.on('error', common.mustCall(function() {
});
}));

d.run(function() {
d.run(() => {
throw new Error('Error from domain');
});
24 changes: 12 additions & 12 deletions test/parallel/test-querystring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ const qsWeirdObjects = [
[{ regexp: /./g }, 'regexp=', { 'regexp': '' }],
// eslint-disable-next-line no-unescaped-regexp-dot
[{ regexp: new RegExp('.', 'g') }, 'regexp=', { 'regexp': '' }],
[{ fn: function() {} }, 'fn=', { 'fn': '' }],
[{ fn: () => {} }, 'fn=', { 'fn': '' }],
[{ fn: new Function('') }, 'fn=', { 'fn': '' }],
[{ math: Math }, 'math=', { 'math': '' }],
[{ e: extendedFunction }, 'e=', { 'e': '' }],
Expand Down Expand Up @@ -192,7 +192,7 @@ function check(actual, expected, input) {
`Expected keys: ${inspect(expectedKeys)}`;
}
assert.deepStrictEqual(actualKeys, expectedKeys, msg);
expectedKeys.forEach(function(key) {
expectedKeys.forEach((key) => {
if (typeof input === 'string') {
msg = `Input: ${inspect(input)}\n` +
`Key: ${inspect(key)}\n` +
Expand All @@ -206,21 +206,21 @@ function check(actual, expected, input) {
}

// test that the canonical qs is parsed properly.
qsTestCases.forEach(function(testCase) {
qsTestCases.forEach((testCase) => {
check(qs.parse(testCase[0]), testCase[2], testCase[0]);
});

// test that the colon test cases can do the same
qsColonTestCases.forEach(function(testCase) {
qsColonTestCases.forEach((testCase) => {
check(qs.parse(testCase[0], ';', ':'), testCase[2], testCase[0]);
});

// test the weird objects, that they get parsed properly
qsWeirdObjects.forEach(function(testCase) {
qsWeirdObjects.forEach((testCase) => {
check(qs.parse(testCase[1]), testCase[2], testCase[1]);
});

qsNoMungeTestCases.forEach(function(testCase) {
qsNoMungeTestCases.forEach((testCase) => {
assert.deepStrictEqual(testCase[0], qs.stringify(testCase[1], '&', '='));
});

Expand Down Expand Up @@ -258,15 +258,15 @@ qsNoMungeTestCases.forEach(function(testCase) {
// now test stringifying

// basic
qsTestCases.forEach(function(testCase) {
qsTestCases.forEach((testCase) => {
assert.strictEqual(testCase[1], qs.stringify(testCase[2]));
});

qsColonTestCases.forEach(function(testCase) {
qsColonTestCases.forEach((testCase) => {
assert.strictEqual(testCase[1], qs.stringify(testCase[2], ';', ':'));
});

qsWeirdObjects.forEach(function(testCase) {
qsWeirdObjects.forEach((testCase) => {
assert.strictEqual(testCase[1], qs.stringify(testCase[0]));
});

Expand Down Expand Up @@ -300,7 +300,7 @@ assert.strictEqual('foo=', qs.stringify({ foo: Infinity }));
assert.strictEqual(f, 'a=b&q=x%3Dy%26y%3Dz');
}

assert.doesNotThrow(function() {
assert.doesNotThrow(() => {
qs.parse(undefined);
});

Expand Down Expand Up @@ -432,15 +432,15 @@ check(qs.parse('%\u0100=%\u0101'), { '%Ā': '%ā' });
}

// Test QueryString.unescapeBuffer
qsUnescapeTestCases.forEach(function(testCase) {
qsUnescapeTestCases.forEach((testCase) => {
assert.strictEqual(qs.unescape(testCase[0]), testCase[1]);
assert.strictEqual(qs.unescapeBuffer(testCase[0]).toString(), testCase[1]);
});

// test overriding .unescape
{
const prevUnescape = qs.unescape;
qs.unescape = function(str) {
qs.unescape = (str) => {
return str.replace(/o/g, '_');
};
check(
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-whatwg-url-searchparams-getall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const { test, assert_equals, assert_true, assert_array_equals } =
License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html
*/
/* eslint-disable */
test(function() {
test(() => {
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Nov 27, 2017

Choose a reason for hiding this comment

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

As per the comment above, it seems we should not change this fragment.

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 27, 2017

Choose a reason for hiding this comment

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

Modifications to them should be upstreamed first.

I'm sorry to have missed it.
I'll revert changes in this file.

var params = new URLSearchParams('a=b&c=d');
assert_array_equals(params.getAll('a'), ['b']);
assert_array_equals(params.getAll('c'), ['d']);
Expand All @@ -25,7 +25,7 @@ test(function() {
assert_array_equals(params.getAll('a'), ['', 'e']);
}, 'getAll() basics');

test(function() {
test(() => {
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ditto.

var params = new URLSearchParams('a=1&a=2&a=3&a');
assert_true(params.has('a'), 'Search params object has name "a"');
var matches = params.getAll('a');
Expand Down
24 changes: 12 additions & 12 deletions test/parallel/test-writeint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ function test8(clazz) {
assert.strictEqual(0xfb, buffer[1]);

/* Make sure we handle truncation correctly */
assert.throws(function() {
assert.throws(() => {
buffer.writeInt8(0xabc, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt8(0xabc, 0);
}, errorOutOfBounds);

Expand All @@ -54,10 +54,10 @@ function test8(clazz) {

assert.strictEqual(0x7f, buffer[0]);
assert.strictEqual(0x80, buffer[1]);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt8(0x7f + 1, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt8(-0x80 - 1, 0);
}, errorOutOfBounds);
}
Expand Down Expand Up @@ -94,10 +94,10 @@ function test16(clazz) {
assert.strictEqual(0xff, buffer[1]);
assert.strictEqual(0x80, buffer[2]);
assert.strictEqual(0x00, buffer[3]);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt16BE(0x7fff + 1, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt16BE(-0x8000 - 1, 0);
}, errorOutOfBounds);

Expand All @@ -107,10 +107,10 @@ function test16(clazz) {
assert.strictEqual(0x7f, buffer[1]);
assert.strictEqual(0x00, buffer[2]);
assert.strictEqual(0x80, buffer[3]);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt16LE(0x7fff + 1, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt16LE(-0x8000 - 1, 0);
}, errorOutOfBounds);
}
Expand Down Expand Up @@ -163,10 +163,10 @@ function test32(clazz) {
assert.strictEqual(0x00, buffer[5]);
assert.strictEqual(0x00, buffer[6]);
assert.strictEqual(0x00, buffer[7]);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt32BE(0x7fffffff + 1, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt32BE(-0x80000000 - 1, 0);
}, errorOutOfBounds);

Expand All @@ -180,10 +180,10 @@ function test32(clazz) {
assert.strictEqual(0x00, buffer[5]);
assert.strictEqual(0x00, buffer[6]);
assert.strictEqual(0x80, buffer[7]);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt32LE(0x7fffffff + 1, 0);
}, errorOutOfBounds);
assert.throws(function() {
assert.throws(() => {
buffer.writeInt32LE(-0x80000000 - 1, 0);
}, errorOutOfBounds);
}
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-zerolengthbufferbug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@
const common = require('../common');
const http = require('http');

const server = http.createServer(function(req, res) {
const server = http.createServer((req, res) => {
const buffer = Buffer.alloc(0);
// FIXME: WTF gjslint want this?
res.writeHead(200, { 'Content-Type': 'text/html',
'Content-Length': buffer.length });
res.end(buffer);
});

server.listen(0, common.mustCall(function() {
http.get({ port: this.address().port }, common.mustCall(function(res) {
server.listen(0, common.mustCall(() => {
http.get({ port: server.address().port }, common.mustCall((res) => {

res.on('data', common.mustNotCall());

res.on('end', function(d) {
res.on('end', (d) => {
server.close();
});
}));
Expand Down