FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: refactor test-async-wrap-getasyncid by santigimeno · Pull Request #18727 · nodejs/node · GitHub

/ node Public

test: refactor test-async-wrap-getasyncid - #18727

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:async-wrap-getasyncid-to-parallel
ConversationCommits (1)ChecksFiles changed
Closed

test: refactor test-async-wrap-getasyncid#18727
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:async-wrap-getasyncid-to-parallel

Conversation

santigimeno commented Feb 12, 2018

Copy link
Copy Markdown
Member

By not using common.PORT anymore.

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

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 12, 2018

santigimeno commented Feb 12, 2018

Copy link
Copy Markdown
Member Author

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

apapirovski commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

The problem with this test is unfortunately more than just common.PORT. It's incredibly sensitive to GC so running it in parallel will just exacerbate that.

I have a PR that proposes a potential solution: #18307 but I'm not entirely happy with it so I've marked it as blocked. If you have any thoughts, would love to hear them.

santigimeno commented Feb 12, 2018

Copy link
Copy Markdown
Member Author

@apapirovski thanks for the info. Would you be ok with the changes while keeping the test in sequential until a solution for the GC issue is found? I think removing unnecessary uses of common.PORT is a good idea.

apapirovski commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

Yeah, I don't see why not. 👍

jasnell commented Feb 14, 2018

Copy link
Copy Markdown
Member

yeah, this test definitely needs to stay in sequential. the other changes lgtm

test: refactor test-async-wrap-getasyncid
a515ebc 
So it does not use `common.PORT` anymore.
santigimeno force-pushed the async-wrap-getasyncid-to-parallel branch from d18af51 to a515ebc Compare February 18, 2018 09:25

santigimeno commented Feb 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Updated so it just removes the use of common.PORT. CI: https://ci.nodejs.org/job/node-test-pull-request/13246/

santigimeno changed the title test: move async-wrap-getasyncid to parallel test: refactor test-async-wrap-getasyncid Feb 20, 2018
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018

apapirovski commented Mar 4, 2018

Copy link
Copy Markdown
Contributor

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

apapirovski commented Mar 4, 2018

Copy link
Copy Markdown
Contributor

Landed in a865e8b

apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
test: refactor test-async-wrap-getasyncid
a865e8b 
So it does not use `common.PORT` anymore.

PR-URL: #18727
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
test: refactor test-async-wrap-getasyncid
d883376 
So it does not use `common.PORT` anymore.

PR-URL: nodejs#18727
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins mentioned this pull request Mar 6, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
test: refactor test-async-wrap-getasyncid
2bd98db 
So it does not use `common.PORT` anymore.

PR-URL: nodejs#18727
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Aug 17, 2018
test: refactor test-async-wrap-getasyncid
d7b657f 
So it does not use `common.PORT` anymore.

PR-URL: #18727
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins mentioned this pull request Aug 17, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR approved these changes
+1 more reviewer
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

5 participants

Back | FazBrowse Home | New Git URL