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-async-wrap-getasyncid #18727

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
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 nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 12, 2018
@santigimeno
Copy link
Member Author

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

@apapirovski
Copy link
Member

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
Copy link
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
Copy link
Member

Yeah, I don't see why not. 👍

@jasnell
Copy link
Member

jasnell commented Feb 14, 2018

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

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

santigimeno commented Feb 18, 2018 •
edited
Loading

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

@santigimeno santigimeno changed the title test: move async-wrap-getasyncid to parallel test: refactor test-async-wrap-getasyncid Feb 20, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
@apapirovski
Copy link
Member

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

@apapirovski
Copy link
Member

Landed in a865e8b

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

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

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

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

PR-URL: #18727
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Merged
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

@apapirovski apapirovski apapirovski 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.
5 participants
@santigimeno @apapirovski @jasnell @BridgeAR @nodejs-github-bot