Skip to content

test: fix test-http-agent-keepalive again - #488

Merged
santigimeno merged 1 commit into
node-v24.x-nsolid-v6.xfrom
santi/fix_kalive_test
Jul 6, 2026
Merged

test: fix test-http-agent-keepalive again#488
santigimeno merged 1 commit into
node-v24.x-nsolid-v6.xfrom
santi/fix_kalive_test

Conversation

@santigimeno

@santigimeno santigimeno commented Jun 29, 2026

Copy link
Copy Markdown
Member

Roll it back to previous version as the http keepalive sec fix no longer adds the event listener.

Summary by CodeRabbit

  • Tests
    • Updated automated checks to verify socket listener cleanup behavior more precisely, reducing the chance of listener leaks and improving connection stability.

@santigimeno
santigimeno requested a review from RafaelGSS June 29, 2026 15:36
@santigimeno santigimeno self-assigned this Jun 29, 2026
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8dbebf75-e17b-4624-9f25-4ccb5776d2a9

📥 Commits

Reviewing files that changed from the base of the PR and between 9a91b04 and e5b2a4c.

📒 Files selected for processing (1)
  • test/addons/nsolid-tracing/test-http-agent-keepalive.js

Walkthrough

In checkListeners, the expected data event listener count for a non-destroyed socket was changed from 1 to 0.

Changes

Test assertion fix

Layer / File(s) Summary
Socket data listener assertion
test/addons/nsolid-tracing/test-http-agent-keepalive.js
checkListeners now expects zero data listeners on a non-destroyed socket instead of one.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Poem

A rabbit counted listeners near and far,
Found one where zero should clearly be तार?
With a twitch of the nose,
The assertion now knows,
And the socket sits quiet as a star.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is clearly related to the main change by naming the affected test and describing the fix.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch santi/fix_kalive_test

Comment @coderabbitai help to get the list of available commands.

Roll it back to previous version as the http keepalive sec fix no longer
adds the event listener.

Signed-off-by: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #488
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@santigimeno
santigimeno force-pushed the santi/fix_kalive_test branch from 9a91b04 to e5b2a4c Compare July 6, 2026 14:55
@santigimeno
santigimeno merged commit e5b2a4c into node-v24.x-nsolid-v6.x Jul 6, 2026
12 of 18 checks passed
@santigimeno
santigimeno deleted the santi/fix_kalive_test branch July 6, 2026 15:02
santigimeno added a commit that referenced this pull request Jul 21, 2026
Roll it back to previous version as the http keepalive sec fix no longer
adds the event listener.

Signed-off-by: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #488
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants