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: remove timers from test-tls-socket-close #53019

Merged
merged 2 commits into from
May 23, 2024
Merged

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

merged 2 commits into from
May 23, 2024

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

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

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

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

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

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

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

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

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

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
@@ -15,6 +15,10 @@ const cert = fixtures.readKey('agent2-cert.pem');
let serverTlsSocket;
const tlsServer = tls.createServer({ cert, key }, (socket) => {
serverTlsSocket = socket;
socket.on('data', (chunk) => {
Copy link
Contributor

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

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
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

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>
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@lpinca @lukins-cz
test: remove timers from test-tls-socket-close
d476ff6 
Fixes: nodejs#49902
PR-URL: nodejs#53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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