Skip to content

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

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
lpinca:remove/timers-from-test-tls-socket-close
May 23, 2024
Merged

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

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented May 16, 2024

Fixes: #49902

@nodejs-github-bot 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 lpinca force-pushed the remove/timers-from-test-tls-socket-close branch from c679e6f to 5581681 Compare May 16, 2024 08:26
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 16, 2024

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

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

setImmediate(() => {
Copy link
Contributor

@aduh95 aduh95 May 23, 2024

Choose a reason for hiding this comment

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

No common.mustCall here?

Copy link
Member Author

@lpinca lpinca May 23, 2024

Choose a reason for hiding this comment

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

We have separate tests for setImmediate().

Copy link
Contributor

@aduh95 aduh95 May 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@lpinca lpinca May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aduh95 aduh95 May 23, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@lpinca lpinca May 23, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aduh95 aduh95 May 23, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@lpinca lpinca May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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

@aduh95 aduh95 May 23, 2024

Choose a reason for hiding this comment

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

No common.mustCallAtLeast?

Copy link
Member Author

@lpinca lpinca May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 23, 2024
@nodejs-github-bot 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 nodejs-github-bot merged commit a81d833 into nodejs:main May 23, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 23, 2024

Landed in a81d833

@lpinca 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
@lpinca @targos
test: remove timers from test-tls-socket-close
3ce7a9a 
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jun 7, 2024
Merged
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@lpinca @eliphazbouye
test: remove timers from test-tls-socket-close
749f64d 
Fixes: nodejs#49902
PR-URL: nodejs#53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@lpinca @bmeck
test: remove timers from test-tls-socket-close
4b53d2e 
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
@lpinca @marco-ippolito
test: remove timers from test-tls-socket-close
ef06a86 
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@lpinca @marco-ippolito
test: remove timers from test-tls-socket-close
048cbe3 
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 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. 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

@lpinca @nodejs-github-bot @aduh95