Skip to content

test(socket-mode): assert a certain amount of close events happen after disconnecting #2187

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
Merged
zimeg merged 1 commit into from
Mar 18, 2025
Merged

test(socket-mode): assert a certain amount of close events happen after disconnecting #2187

zimeg merged 1 commit into from
Mar 18, 2025

Conversation

@zimeg
Copy link
Member

@zimeg zimeg commented Mar 18, 2025

Summary

This PR asserts the "should maintain one serial reconnection attempt if WSS server sends unexpected HTTP response during handshake, like a 409" test has an expected number of retries after performing test cleanups.

This fixes an issue where a failing assert causes the test runner to not complete! This is for better insights into #2138 and #2159 patterns following #2178.

Preview

This video shows this failing test before and after the changes 👾

errors.mov

Notes

Handfuls of reruns makes me believe more and more that this test is failing due to how precise CI handles milliseconds and opened ports. This PR doesn't update related logic, but I'm hoping failing test results give us insight into this!

Requirements

@zimeg zimeg added tests M-T: Testing work only pkg:socket-mode applies to `@slack/socket-mode` labels Mar 18, 2025
@zimeg zimeg self-assigned this Mar 18, 2025
@codecov
Copy link

codecov bot commented Mar 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.94%. Comparing base (0e0f55d) to head (96285ee).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2187   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files          38       38           
  Lines       10328    10328           
  Branches      652      652           
=======================================
  Hits         9496     9496           
  Misses        820      820           
  Partials       12       12
Flag Coverage Δ
cli-hooks ∅ <ø> (∅)
cli-test ∅ <ø> (∅)
oauth ∅ <ø> (∅)
socket-mode ∅ <ø> (∅)
web-api ∅ <ø> (∅)
webhook ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zimeg
Copy link
Member Author

zimeg commented Mar 18, 2025

🔬 https://github.com/slackapi/node-slack-sdk/actions/runs/13914505870/job/38935010216?pr=2187#step:9:180

  1) Integration tests with a WebSocket server
       unexpected socket messages sent to client
         should maintain one serial reconnection attempt if WSS server sends unexpected HTTP response during handshake, like a 409:

      unexpected number of times `close` event was raised during reconnection!
      + expected - actual

      -3
      +2
      
      at Context.<anonymous> (test/integration.spec.js:217:14)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

TBH this isn't what I expected, I assumed fewer than 2, but it's great to know we're off by just 1 instead of 100 🦠

@zimeg
Copy link
Member Author

zimeg commented Mar 18, 2025

🙏 I'll rerun these tests to avoid failing checks, but I'm hopeful that this is helpful in the quest for fewer errors.

WilliamBergamin
WilliamBergamin approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice find!! 💯 this should really help us in our quest to save the flaky tests

There is a lot going on in this file, and possibly a lot to improve, my biggest concern is around beforeEach and afterEach. beforeEach takes care of setting up a new socket mode client, I would expect afterEach to take care of cleaning it up 🤔 but for some reason this is left up to the tests, this could very well be the source of some flakiness

Maybe we should consider instantiating a new server and wss for each tests

@zimeg
Copy link
Member Author

zimeg commented Mar 18, 2025

@WilliamBergamin Thanks so much for reviewing along this quest! 🙏

Maybe we should consider instantiating a new server and wss for each tests

I'm quite interested in exploring this more. The setups you point out were becoming confusing to me when attempting to mock strange error states. This seems like another excellent follow up to explore if these tests continue to flake!

But for now I'll merge this PR so we can start catching these flakes ❄️ 🔍

@zimeg zimeg merged commit cb8269f into main Mar 18, 2025
57 checks passed
@zimeg zimeg deleted the zimeg-test-socket-mode-assert-late branch March 18, 2025 17:45
@WilliamBergamin WilliamBergamin mentioned this pull request Mar 21, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WilliamBergamin WilliamBergamin WilliamBergamin approved these changes

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

Assignees

@zimeg zimeg

Labels

pkg:socket-mode applies to `@slack/socket-mode` tests M-T: Testing work only

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zimeg @WilliamBergamin