test: replace function with arrow function by Leko · Pull Request #17345 · nodejs/node

Leko · 2017-11-27T08:57:14Z

This is part of Nodefest's Code and Learn nodejs/code-and-learn#72

Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function

test/parallel/test-assert.js
test/parallel/test-domain-top-level-error-handler-clears-stack.js
test/parallel/test-querystring.js
test/parallel/test-whatwg-url-searchparams-getall.js
test/parallel/test-writeint.js
test/parallel/test-zerolengthbufferbug.js

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Among the list of [Code and Learn](nodejs/code-and-learn#72 (comment)), I solved the unfinished task of replacing function with arrow function

vsemozhetbyt · 2017-11-27T09:51:03Z

test/parallel/test-assert.js

 function makeBlock(f) {
  const args = Array.prototype.slice.call(arguments, 1);
-  return function() {
+  return () => {


This makes this lexical. Is it OK here?

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 ?

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

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

vsemozhetbyt · 2017-11-27T09:51:26Z

test/parallel/test-assert.js

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


Thank you for your review.
I was mistaken.

I'll fix this problem with like:

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

vsemozhetbyt · 2017-11-27T09:53:30Z

test/parallel/test-whatwg-url-searchparams-getall.js

 */
 /* eslint-disable */
-test(function() {
+test(() => {


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

Modifications to them should be upstreamed first.

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

vsemozhetbyt · 2017-11-27T09:53:48Z

test/parallel/test-whatwg-url-searchparams-getall.js

 }, 'getAll() basics');

-test(function() {
+test(() => {


Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime.

These tests are copied from WPT. I should not changed it directly.

Leko · 2017-11-27T11:31:20Z

test/parallel/test-assert.js

 testAssertionMessage(/abc/gim, '/abc/gim');
 testAssertionMessage(function f() {}, '[Function: f]');
-testAssertionMessage(function() {}, '[Function]');
+testAssertionMessage(() => {}, '[Function]');


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 ?

yosuke-furukawa · 2017-11-27T11:32:27Z

test/parallel/test-assert.js

 testAssertionMessage(/abc/gim, '/abc/gim');
 testAssertionMessage(function f() {}, '[Function: f]');
-testAssertionMessage(function() {}, '[Function]');
+testAssertionMessage(() => {}, '[Function]');


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.

makeBlock does not need `this`. update `this` with `null` to clarify the intent.

vsemozhetbyt · 2017-11-27T14:07:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11748/

`function() {}` and `() => {}` are should be tested both. See also #17345 (comment)

vsemozhetbyt · 2017-11-27T15:31:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11751/

vsemozhetbyt · 2017-11-27T15:45:54Z

Hmm. It seems CI does not post status to PR currently.

Leko · 2017-11-27T15:47:31Z

Yesterday, status integration is worked fine.
What's happen ... ? 🤔

Leko · 2017-11-29T11:36:27Z

Hi @vsemozhetbyt.
Should I take any action to merge this PR ?

vsemozhetbyt · 2017-11-29T11:44:59Z

@Leko Sorry for the delay. Let's run the CI again. If it is green, we will land it today.

https://ci.nodejs.org/job/node-test-pull-request/11798/

Leko · 2017-11-29T11:46:47Z

@vsemozhetbyt I got it. Thank you for quick reply !

vsemozhetbyt · 2017-11-29T14:02:41Z

One CI failure seems unrelated. Landing...

1. Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function: nodejs/code-and-learn#72 (comment) 2. Replace arrow function with shorter property syntax Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime. 3. Replace this with null makeBlock does not need `this`. update `this` with `null` to clarify the intent. PR-URL: #17345 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Yosuke Furukawa <[email protected]>

vsemozhetbyt · 2017-11-29T14:19:34Z

Squashed and landed in 3c62f33

Thank you, @Leko!

1. Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function: nodejs/code-and-learn#72 (comment) 2. Replace arrow function with shorter property syntax Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime. 3. Replace this with null makeBlock does not need `this`. update `this` with `null` to clarify the intent. PR-URL: #17345 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Yosuke Furukawa <[email protected]>

test: replace function with arrow function

79d1fbc

Among the list of [Code and Learn](nodejs/code-and-learn#72 (comment)), I solved the unfinished task of replacing function with arrow function

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 27, 2017

vsemozhetbyt added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 27, 2017

vsemozhetbyt reviewed Nov 27, 2017

View reviewed changes

Leko added 2 commits November 27, 2017 20:04

test: replace arrow function with shorter property syntax

c0e7ee8

Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime.

test: revert changes in test-whatwg-url-searchparams-getall

6dccb02

These tests are copied from WPT. I should not changed it directly.

Leko commented Nov 27, 2017

View reviewed changes

yosuke-furukawa reviewed Nov 27, 2017

View reviewed changes

test: replace this with null

0739723

makeBlock does not need `this`. update `this` with `null` to clarify the intent.

mscdex added assert Issues and PRs related to the assert subsystem. domain Issues and PRs related to the domain subsystem. querystring Issues and PRs related to the built-in querystring module. labels Nov 27, 2017

test: revert to normal function

fa4eb72

`function() {}` and `() => {}` are should be tested both. See also #17345 (comment)

addaleax approved these changes Nov 27, 2017

View reviewed changes

maclover7 approved these changes Nov 27, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Nov 27, 2017

CI does not post status to PR currently #17358

Closed

addaleax approved these changes Nov 28, 2017

View reviewed changes

yosuke-furukawa approved these changes Nov 28, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Nov 29, 2017

CI status in PR differs from status in ci.nodejs.org #17387

Closed

vsemozhetbyt closed this Nov 29, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added dont-land-on-v6.x labels Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

Uh oh!

Conversation

Leko commented Nov 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

Leko commented Nov 27, 2017

Uh oh!

Leko commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

Leko commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vsemozhetbyt Nov 27, 2017 •

edited

Loading