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.

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

socket-mode: Handling WS errors during handshake #2099

Merged
merged 5 commits into from
Dec 9, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add a unit test for connect() method of SlackWebSocket
Filip Maj committed Nov 25, 2024
commit 2fecb13f6b689285659bed7e1d2cacbf5c67dce3
2 changes: 2 additions & 0 deletions packages/socket-mode/package.json
Original file line number Diff line number Diff line change
@@ -57,11 +57,13 @@
"@tsconfig/recommended": "^1.0.7",
"@types/chai": "^4",
"@types/mocha": "^10",
"@types/proxyquire": "^1.3.31",
"@types/sinon": "^17",
"c8": "^10.1.2",
"chai": "^4",
"mocha": "^10",
"nodemon": "^3.1.0",
"proxyquire": "^2.1.3",
"shx": "^0.3.2",
"sinon": "^19",
"source-map-support": "^0.5.21",
48 changes: 45 additions & 3 deletions packages/socket-mode/src/SlackWebSocket.spec.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import { ConsoleLogger } from '@slack/logger';
import { assert } from 'chai';
import EventEmitter from 'eventemitter3';
import proxyquire from 'proxyquire';
import sinon from 'sinon';

import { SlackWebSocket } from './SlackWebSocket';
proxyquire.noPreserveCache();
Copy link
Member

Choose a reason for hiding this comment

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

Superb test setups all throughout 👏 This adds so much confidence around the changes.

With some of the retry timing we might have to lookout for flakiness of tests due to the CI runners - nothing a rerun might not solve, but with the matrix setups I think our chances of this happening increase eversomuch 👾

import logModule from './logger';

// A slightly spruced up event emitter aiming at mocking out the `ws` library's `WebSocket` class
class WSMock extends EventEmitter {
// biome-ignore lint/suspicious/noExplicitAny: event listeners can accept any args
addEventListener(evt: string, fn: (...args: any[]) => void) {
this.addListener.call(this, evt, fn);
}
}

describe('SlackWebSocket', () => {
const sandbox = sinon.createSandbox();

let SlackWebSocket: typeof import('./SlackWebSocket').SlackWebSocket;
beforeEach(() => {
SlackWebSocket = proxyquire.load('./SlackWebSocket', {
ws: {
WebSocket: WSMock,
},
}).SlackWebSocket;
});
afterEach(() => {
sandbox.restore();
});
@@ -38,4 +53,31 @@ describe('SlackWebSocket', () => {
assert.isFalse(logFactory.called);
});
});
describe('WebSocket event handling', () => {
it('should call disconnect() if websocket emits an error', async () => {
// an exposed event emitter pretending its a websocket
const ws = new WSMock();
// mock out the `ws` library and have it return our event emitter mock
SlackWebSocket = proxyquire.load('./SlackWebSocket', {
ws: {
WebSocket: class Fake {
constructor() {
// biome-ignore lint/correctness/noConstructorReturn: for test mocking purposes
return ws;
}
},
},
}).SlackWebSocket;
const sws = new SlackWebSocket({
url: 'whatevs',
client: new EventEmitter(),
clientPingTimeoutMS: 1,
serverPingTimeoutMS: 1,
});
const discStub = sinon.stub(sws, 'disconnect');
sws.connect();
ws.emit('error', { error: new Error('boom') });
sinon.assert.calledOnce(discStub);
});
});
});
2 changes: 1 addition & 1 deletion packages/socket-mode/test/integration.spec.js
Original file line number Diff line number Diff line change
@@ -132,7 +132,7 @@ describe('Integration tests with a WebSocket server', () => {
});
describe('unexpected socket messages sent to client', () => {
const debugLoggerSpy = sinon.stub(); // add the following to expose further logging: .callsFake(console.log);
const noop = () => { };
const noop = () => {};
beforeEach(() => {
client = new SocketModeClient({
appToken: 'whatever',

Unchanged files with check annotations Beta

this.logger.debug(`Before trying to reconnect, this client will wait for ${msBeforeRetry} milliseconds`);
return new Promise((res, _rej) => {
setTimeout(() => {
if (this.shuttingDown) {
this.logger.debug('Client shutting down, will not attempt reconnect.');
} else {
this.logger.debug('Continuing with reconnect...');
this.emit(State.Reconnecting);
cb.apply(this).then(res);
}

Check warning on line 238 in packages/socket-mode/src/SocketModeClient.ts

Codecov / codecov/patch

packages/socket-mode/src/SocketModeClient.ts#L232-L238

Added lines #L232 - L238 were not covered by tests
}, msBeforeRetry);
});
}