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

Jump to bottom

test: refactor test-child-process-env to use arrow functions #24482

Closed
wants to merge 3 commits into from
Closed

test: refactor test-child-process-env to use arrow functions #24482

wants to merge 3 commits into from

Conversation

sagirk
Copy link
Contributor

@sagirk sagirk commented Nov 19, 2018 •
edited
Loading

In test/parallel/test-child-process-env.js, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to this,
super or arguments. This results in shorter functions.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 19, 2018
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 19, 2018
@gireeshpunathil
Copy link
Member

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

@sagirk
Copy link
Contributor Author

sagirk commented Nov 20, 2018 •
edited
Loading

@thefourtheye The build failed, seemingly because of the last change.

How do I go about fixing it? Undoing the common.mustCall wrapper will fix it, but I want to understand if there's something else that we can do here.

image

Trott
Trott previously requested changes Nov 20, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Please remove the use of common.mustCall() as an exit event handler. Thanks!

@sagirk
test: refactor test-child-process-env to use arrow functions
0767eba 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.
@sagirk
test: wrap functions containing assertions inside a common.mustCall
91fa0e1
@sagirk
Revert "test: wrap functions containing assertions inside a `common.m…
dc4bdab 
…ustCall`"

This reverts commit 91fa0e1.
@sagirk sagirk force-pushed the refactor/test-child-process-env branch from a70411b to dc4bdab Compare November 21, 2018 08:05
@sagirk sagirk mentioned this pull request Nov 21, 2018
Closed
4 tasks
@sagirk
Copy link
Contributor Author

sagirk commented Nov 21, 2018

@Trott Done. Build passed. PTAL.

@gireeshpunathil
Copy link
Member

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

@gireeshpunathil
Copy link
Member

windows-fanned test unrelated:

03:38:14 not ok 284 parallel/test-trace-events-worker-metadata
03:38:14   ---
03:38:14   duration_ms: 8.340
03:38:14   severity: fail
03:38:14   exitcode: 1
03:38:14   stack: |-
03:38:14     assert.js:351
03:38:14         throw err;
03:38:14         ^
03:38:14     
03:38:14     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
03:38:14     
03:38:14       assert(traces.some((trace) =>
03:38:14         trace.cat === '__metadata' && trace.name === 'thread_name' &&
03:38:14           trace.args.name === 'WorkerThread 1'))
03:38:14     
03:38:14         at fs.readFile.common.mustCall (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-trace-events-worker-metadata.js:26:7)
03:38:14         at /home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:346:15
03:38:14         at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:54:3)
03:38:14   ...

@gireeshpunathil
Copy link
Member

so all is good, just waiting for @Trott to re-review and dismiss the change request.

@Trott Trott dismissed their stale review November 26, 2018 14:56

requested changes applied

@Trott
Copy link
Member

Trott commented Nov 28, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18989/

@gireeshpunathil
Copy link
Member

landed as 484ad3b

gireeshpunathil pushed a commit that referenced this pull request Nov 28, 2018
@sagirk @gireeshpunathil
test: use arrow function
484ad3b 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.

PR-URL: #24482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Nov 28, 2018
@sagirk @targos
test: use arrow function
0a492c7 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.

PR-URL: #24482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@sagirk sagirk deleted the refactor/test-child-process-env branch November 30, 2018 03:44
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
This was referenced Dec 7, 2018
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@sagirk @refack
test: use arrow function
82b04fc 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.

PR-URL: nodejs#24482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@sagirk @BethGriggs
test: use arrow function
2318c7f 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.

PR-URL: #24482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sagirk @rvagg
test: use arrow function
5770c52 
In `test/parallel/test-child-process-env.js`, callbacks use
anonymous closure functions. It is safe to replace them with arrow
functions since these callbacks don't contain references to `this`,
`super` or `arguments`. This results in shorter functions.

PR-URL: #24482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sagirk @gireeshpunathil @Trott @jasnell @thefourtheye @addaleax @cjihrig @BridgeAR @BethGriggs @nodejs-github-bot