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

test: replace function with arrow function #17345

wants to merge 5 commits into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Nov 27, 2017

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
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 27, 2017
@vsemozhetbyt vsemozhetbyt added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 27, 2017
@@ -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.

@@ -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}`; }

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

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

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

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

makeBlock does not need `this`.
update `this` with `null` to clarify the intent.
@mscdex 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
@vsemozhetbyt
Copy link
Contributor

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

@vsemozhetbyt
Copy link
Contributor

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

@Leko
Copy link
Contributor Author

Leko commented Nov 27, 2017

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

@Leko
Copy link
Contributor Author

Leko commented Nov 29, 2017

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

@vsemozhetbyt
Copy link
Contributor

@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
Copy link
Contributor Author

Leko commented Nov 29, 2017

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

@vsemozhetbyt
Copy link
Contributor

One CI failure seems unrelated. Landing...

vsemozhetbyt pushed a commit that referenced this pull request Nov 29, 2017
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 <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Squashed and landed in 3c62f33

Thank you, @Leko!

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
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 <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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 <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. domain Issues and PRs related to the domain subsystem. querystring Issues and PRs related to the built-in querystring module. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants