Skip to content

test: deflake test-http-destroyed-socket-write2 - #36120

Merged
lpinca merged 1 commit into
nodejs:masterfrom
lpinca:fix/36081
Nov 18, 2020
Merged

test: deflake test-http-destroyed-socket-write2#36120
lpinca merged 1 commit into
nodejs:masterfrom
lpinca:fix/36081

Conversation

@lpinca

@lpincalpinca commented Nov 14, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.

Fixes: #36081
Fixes: #4066

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 14, 2020
@lpinca

lpinca commented Nov 14, 2020

Copy link
Copy Markdown
MemberAuthor
./tools/test.py -J --repeat=500 test/parallel/test-http-destroyed-socket-write2.js

was failing before with timeouts on macOS, now it exits with 0.

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 14, 2020

Copy link
Copy Markdown
Collaborator

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

rickyes
rickyes reviewed Nov 15, 2020
View reviewed changes
Comment threadtest/parallel/test-http-destroyed-socket-write2.js Outdated

@rickyesrickyesNov 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a need for .mustCall here?

@lpincalpincaNov 15, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, otherwise there is no guarantee that the listener of the 'request' event added on line 33 is called.

@lpincalpincaNov 15, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Anyway we can use common.mustCall() there (line 33) if it is easier to grok.

@lpincalpincaNov 15, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think I misunderstood. Do you mean adding a callback to this req.write()? If so, I think it is not needed. It's out of the scope of the test.

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 15, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Nov 17, 2020

Copy link
Copy Markdown
Collaborator

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

@lpinca
test: deflake test-http-destroyed-socket-write2
e682814
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.
PR-URL: nodejs#36120Fixes: nodejs#36081Fixes: nodejs#4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@lpinca
lpinca merged commit e682814 into nodejs:masterNov 18, 2020
@lpinca

lpinca commented Nov 18, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in e682814.

@lpinca
lpinca deleted the fix/36081 branch November 18, 2020 17:52
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@lpinca@codebytere
test: deflake test-http-destroyed-socket-write2
4746649
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.
PR-URL: #36120Fixes: #36081Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@codebyterecodebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@lpinca@BethGriggs
test: deflake test-http-destroyed-socket-write2
fa14657
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.
PR-URL: #36120Fixes: #36081Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@lpinca@BethGriggs
test: deflake test-http-destroyed-socket-write2
ed1a114
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.
PR-URL: #36120Fixes: #36081Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@lpinca@BethGriggs
test: deflake test-http-destroyed-socket-write2
3e77536
Ensure that the write occurs in the same tick where the socket is
destroyed by the other peer.
PR-URL: #36120Fixes: #36081Fixes: #4066
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
+1 more reviewer
@rickyesrickyesrickyes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test-http-destroyed-socket-write2 timeout Investigate flaky test-http-destroyed-socket-write2

4 participants

@lpinca@nodejs-github-bot@Trott@rickyes