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: replace function with arrow function #17345

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

Choose a reason for hiding this comment

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

The same.

Copy link
Contributor Author

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
Contributor Author

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
Member

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
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
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
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
Contributor

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
Contributor Author

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
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.

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
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
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