Skip to content

Conversation

@gismya
Copy link
Contributor

@gismya gismya commented May 22, 2024

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

PR to address #238. Adds a disconnect method to the event hub

Test

@gismya gismya requested a review from a team as a code owner May 22, 2024 11:41
@ffMathy
Copy link
Contributor

ffMathy commented May 22, 2024

Thank you so much for this! ❤️


test("Disconnecting should disconnect the socket", () => {
eventHub.disconnect();
expect(disconnectCalled).toBe(true);
Copy link
Contributor

@jimmycallin jimmycallin May 22, 2024

Choose a reason for hiding this comment

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

Suggested change
expect(disconnectCalled).toBe(true);
expect(eventHub._socketIo.disconnect).toHaveBeenCalled();

think you can do something like this, a bit more idiomatic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'm setting _socketIo to undefined as part of the disconnect.

Copy link
Contributor

@jimmycallin jimmycallin May 22, 2024

Choose a reason for hiding this comment

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

ah, I guess you then could do this:

const disconnectFn = vi.fn()
...
disconnect: disconnectFn,
...
expect(disconnectFn).toHaveBeenCalled()

but we can go with this approach as well

@gismya gismya merged commit 817259e into main May 23, 2024
@gismya gismya deleted the explicitly-disconnect-event-hub branch May 23, 2024 05:25
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.

4 participants