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

test: remove timers from test-tls-socket-close by lpinca · Pull Request #53019 · nodejs/node · GitHub

/ node Public

test: remove timers from test-tls-socket-close - #53019

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
lpinca:remove/timers-from-test-tls-socket-close
May 23, 2024
ConversationCommits (2)ChecksFiles changed
Merged

test: remove timers from test-tls-socket-close#53019
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
lpinca:remove/timers-from-test-tls-socket-close

Conversation

lpinca commented May 16, 2024

Copy link
Copy Markdown
Member

Fixes: #49902

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 16, 2024
lpinca force-pushed the remove/timers-from-test-tls-socket-close branch from c679e6f to 5581681 Compare May 16, 2024 08:26
lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024

This comment was marked as outdated.

Sign in to view
lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented May 16, 2024

Copy link
Copy Markdown
Collaborator

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

aduh95 approved these changes May 23, 2024
View reviewed changes
Comment thread test/parallel/test-tls-socket-close.js
netSocket.destroy();
assert.strictEqual(netSocket.destroyed, true);

setImmediate(() => {

aduh95 May 23, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No common.mustCall here?

lpinca May 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We have separate tests for setImmediate().

aduh95 May 23, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't really get what you mean, common.mustCall wouldn't be there to validate setImmediate behavior, but to ensure the assertion are actually run. Anyway, it doesn't really matter.

lpinca May 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We are testing that the callback of setImmediate() is called in other tests.

aduh95 May 23, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's not called if e.g. process.exit is called before it's picked up

lpinca May 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is no process.exit() in the test.

aduh95 May 23, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Adding common.mustCall would ensure we catch it if it was ever added

lpinca May 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've added it only where it is needed given the current assumptions. If the logic changes there are other more places where it would be needed.

Comment thread test/parallel/test-tls-socket-close.js
let serverTlsSocket;
const tlsServer = tls.createServer({ cert, key }, (socket) => {
serverTlsSocket = socket;
socket.on('data', (chunk) => {

aduh95 May 23, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No common.mustCallAtLeast?

lpinca May 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, only one byte is sent by the other peer.

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 23, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024
nodejs-github-bot merged commit a81d833 into nodejs:main May 23, 2024

nodejs-github-bot commented May 23, 2024

Copy link
Copy Markdown
Collaborator

Landed in a81d833

lpinca deleted the remove/timers-from-test-tls-socket-close branch May 23, 2024 11:55
targos pushed a commit that referenced this pull request Jun 1, 2024
test: remove timers from test-tls-socket-close
3ce7a9a 
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS mentioned this pull request Jun 7, 2024
Merged
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
test: remove timers from test-tls-socket-close
749f64d 
Fixes: nodejs#49902
PR-URL: nodejs#53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
test: remove timers from test-tls-socket-close
048cbe3 
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito mentioned this pull request Jul 19, 2024
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

aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

parallel.test-tls-socket-close is flaky

3 participants

Back | FazBrowse Home | New Git URL