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: use destructuring on require #24455

Closed
wants to merge 1 commit into from
Closed

test: use destructuring on require #24455

wants to merge 1 commit into from

Conversation

juanarbol
Copy link
Member

@juanarbol juanarbol commented Nov 17, 2018 •
edited
Loading

Uses destructuring for spawn, spawnSync and writeFileSync require intest-tick-processor-polyfill-brokenfile.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 17, 2018
@Trott
Copy link
Member

Trott commented Nov 18, 2018

Hi, @juanarbol! Welcome and thanks for the pull request! The change is fine, I guess, but it's not clear to me that this is a significant improvement over the existing code. Curious to get the opinion of other collaborators.

@juanarbol
Copy link
Member Author

I took idea from Nodetodo, just make more clear an specific test. It's quite hard find anything to help ☹️

@Trott
Copy link
Member

Trott commented Nov 18, 2018

I took idea from Nodetodo

I'm not sure what you mean. There's nothing on the Node Todo website that suggests that introducing destructuring in a situation like this is desirable. Can you point me to where you got the suggestion?

It's quite hard find anything to help

I'll email you a suggested task.

@juanarbol
Copy link
Member Author

Sure, I saw this commit in Nodetodo's list (Commits From NodeTodo Activities), it's kind of a code refactor of a particular unit test. I read some test files and found almost same "problem" in this particular file changed on this commit (mine), It's like an inspiration, I guess.
Thanks for reviewing! Feels so good haha

@Trott
Copy link
Member

Trott commented Nov 20, 2018

Sure, I saw this commit in Nodetodo's list

Ah, I see. That commit uses destructuring to replace assigning a property to a variable. This change, though, replaces a require() without a property being assigned. The former arguably improves readability. I'm not sure this has the same effect.

@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2018
@addaleax
Copy link
Member

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

@addaleax
Copy link
Member

Landed in e0893f0

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@addaleax addaleax closed this Nov 24, 2018
pull bot pushed a commit to SimenB/node that referenced this pull request Nov 24, 2018
@juanarbol @addaleax
test: use destructuring on require
e0893f0 
PR-URL: nodejs#24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Nov 24, 2018
@juanarbol @targos
test: use destructuring on require
c384ce9 
PR-URL: #24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@juanarbol @rvagg
test: use destructuring on require
363d3c6 
PR-URL: #24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
codebytere pushed a commit that referenced this pull request Jan 13, 2019
@juanarbol @codebytere
test: use destructuring on require
755bb96 
PR-URL: #24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@juanarbol @refack
test: use destructuring on require
907ce9c 
PR-URL: nodejs#24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Jan 15, 2019
Merged
codebytere pushed a commit that referenced this pull request Jan 29, 2019
@juanarbol @codebytere
test: use destructuring on require
013b0d0 
PR-URL: #24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
@juanarbol @codebytere
test: use destructuring on require
106a1a1 
PR-URL: #24455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Jan 29, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@juanarbol juanarbol deleted the test-tick-destructuring branch January 19, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juanarbol @Trott @addaleax @jasnell @trivikr @nodejs-github-bot