Add tests for SocketModeReceiver (#750)

[filmaj]

Summary

Saw there was a low priority "add tests for this untested thing" task (#750) so I thought I'd give it a shot! This is a first pass at adding tests for the SocketModeReceiver module.

I did have trouble replicating the way the e.g. ExpressReceiver test create a new FakeServer instance in its beforeEach setup method. When I tried to use the same approach, I would get this TypeScript error:

src/receivers/SocketModeReceiver.spec.ts:49:35 - error TS2339: Property 'fakeCreateServer' does not exist on type 'Suite'.

49         withHttpCreateServer(this.fakeCreateServer),
                                     ~~~~~~~~~~~~~~~~

By moving the FakeServer instantiation to within the test, that avoided the issue - though I'd like to keep the test style consistent across test files if possible! Not sure why copying the approach from ExpressReceiver didn't work for me 🤔 . From what I can tell, the type of this within a beforeEach block is Context whereas the type of this within an it block is Suite - so I'm not sure how it works in ExpressReceiver ❓

Requirements (place an x in each [ ])

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #1012 (1b6b5d6) into main (7bcbfb6) will increase coverage by 2.06%.
The diff coverage is n/a.

❗ Current head 1b6b5d6 differs from pull request most recent head 282637f. Consider uploading reports for the commit 282637f to get more accurate results

@@            Coverage Diff             @@
##             main    #1012      +/-   ##
==========================================
+ Coverage   66.41%   68.48%   +2.06%     
==========================================
  Files          13       13              
  Lines        1212     1212              
  Branches      357      357              
==========================================
+ Hits          805      830      +25     
+ Misses        338      309      -29     
- Partials       69       73       +4     

Impacted Files	Coverage Δ
src/receivers/SocketModeReceiver.ts	57.69% <0.00%> (+46.15%)	⬆️
src/receivers/render-html-for-install-path.ts	100.00% <0.00%> (+50.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bcbfb6...282637f. Read the comment docs.

[seratch]

Looks great to me; You're so quick!

I left a few comments. If you also think we should have one more test case for the stop method, update the test before merging this PR.

Note that we usually merge PRs with squashed one commit (as long as there is no reason to have multiple commits). You can do either force-push with a squashed commit or using the "Squash and merge" button in the GitHub web UI.

[seratch]

Adding this test suite is a great start!

In addition to the tests in this PR, we can add a test case to run await receiver#stop(). I know the test won't be a crucial test asset, but it slightly improves the code coverage. On the other hand, we cannot run receiver#start() as it blocks the test execution and the test results in a timeout error.

Also, mainly for your information, let me mention a few other things. As these are nice-to-haves in future development, you don't need to have anything further in this PR.

• Currently, we don't have any integration tests using both App and any of the built-in receivers. App.spec.ts uses only FakeReceiver, which does not do anything. We may want to have different ways to ensure the combination of App and a receiver works as we expect.

• (This may be more like an improvement on @slack/socket-mode side) In the future, we may want to have some tests verifying WebSocket communications (incl. auto-reconnection) and payload handling code. Currently, we don't have any tests for this, even on the @slack/socket-mode library side. Having some tests for the pattern will make future development / debugging reported issues.

  • For your reference, we have some tests and mock WS servers in Python/Java projects:
    • https://github.com/slackapi/bolt-python/blob/main/tests/adapter_tests/socket_mode/mock_socket_mode_server.py
    • https://github.com/slackapi/python-slack-sdk/blob/main/tests/slack_sdk/socket_mode/mock_socket_mode_server.py
    • https://github.com/slackapi/java-slack-sdk/blob/main/bolt-socket-mode/src/test/java/test_locally/SocketModeAppTest.java

I hope these are helpful to you (and anyone who read this comment!)

[misscoded]

LGTM! Always happy to have more tests 🙌🏼

From what I can tell, the type of this within a beforeEach block is Context whereas the type of this within an it block is Suite - so I'm not sure how it works in ExpressReceiver ❓

I believe the cause of it grabbing onto Suite instead of Context is the use of arrow functions (ie, no bound this). Switching them out for function (which is used in the ExpressReceiver tests) is one way to address this and should make it possible to incorporate the beforeEach block to contain the more repetitive bits.

[filmaj]

Thanks for the reviews @misscoded @seratch !

I believe the cause of it grabbing onto Suite instead of Context is the use of arrow functions (ie, no bound this). Switching them out for function (which is used in the ExpressReceiver tests) is one way to address this and should make it possible to incorporate the beforeEach block to contain the more repetitive bits.

Thanks for explaining that @misscoded, this is today's edition of "Today I Learned" 😊 . That indeed addressed the issue and I think it looks a bit cleaner now.

If you also think we should have one more test case for the stop method, update the test before merging this PR.

Good call @seratch, I've added a very basic test for both start and stop methods (simply checks that the appropriate SocketModeClient underlying method is invoked).

Also, mainly for your information, let me mention a few other things [regarding integration testing in the other client SDKs]...

This is super handy @seratch, thanks for pointing it out. I will take some time to read through these and have a think about how bolt-js could follow a similar path to ensure the same level of integration testing coverage.

Note that we usually merge PRs with squashed one commit (as long as there is no reason to have multiple commits). You can do either force-push with a squashed commit or using the "Squash and merge" button in the GitHub web UI.

I've rebased this branch with the latest from main and squashed everything down to a single commit.

Assuming the latest changes are approved by you all, I'm not sure if it is OK to merge my own PR or not on the team (still figuring out The Way We Do Things) but even if that is the case, feel free to merge it in if you think it is acceptable.

This was fun 😄

[seratch]

@filmaj Thanks for having the tests for both start and stop methods. Indeed, we can easily have those using the stub objects for the underlying SocketModeClient 👍

Assuming the latest changes are approved by you all, I'm not sure if it is OK to merge my own PR or not on the team (still figuring out The Way We Do Things) but even if that is the case, feel free to merge it in if you think it is acceptable.

You can merge it on your own now but I would like to have the honor of merging your first-ever pull request for this project 😄 Great work 🎉