Add tests for SocketModeReceiver (#750) by filmaj · Pull Request #1012 · slackapi/bolt-js

filmaj · 2021-07-20T16:39:36Z

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 · 2021-07-20T16:39:40Z

All committers have signed the CLA.

codecov · 2021-07-20T16:51:12Z

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.

src/receivers/SocketModeReceiver.spec.ts

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.

src/receivers/SocketModeReceiver.spec.ts

seratch · 2021-07-21T03:10:36Z

src/receivers/SocketModeReceiver.spec.ts

@@ -0,0 +1,288 @@
+/* eslint-disable @typescript-eslint/no-unsafe-call, @typescript-eslint/naming-convention */


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!)

src/receivers/SocketModeReceiver.spec.ts

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.

…thods. (slackapi#750)

filmaj · 2021-07-21T12:59:30Z

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 · 2021-07-21T14:18:28Z

@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 🎉

gitwave bot added the untriaged label Jul 20, 2021

filmaj changed the title ~~Add tests for SocketModeReceiver constructor (#750)~~ Add tests for SocketModeReceiver (#750) Jul 20, 2021

filmaj force-pushed the socketmodereceiver-tests branch from 9a281d0 to 177b66d Compare July 20, 2021 16:49

filmaj force-pushed the socketmodereceiver-tests branch from 866c15e to 1b6b5d6 Compare July 20, 2021 19:53

filmaj marked this pull request as ready for review July 20, 2021 19:55

filmaj requested a review from misscoded July 20, 2021 19:59

stevengill self-requested a review July 20, 2021 20:06

filmaj requested a review from seratch July 20, 2021 20:08

filmaj commented Jul 20, 2021

View reviewed changes

src/receivers/SocketModeReceiver.spec.ts Show resolved Hide resolved

filmaj commented Jul 20, 2021

View reviewed changes

src/receivers/SocketModeReceiver.spec.ts Show resolved Hide resolved

stevengill added tests M-T: Testing work only and removed untriaged labels Jul 20, 2021

stevengill assigned filmaj Jul 20, 2021

stevengill linked an issue Jul 20, 2021 that may be closed by this pull request

Add Tests for SocketModeReceiver #750

Closed

10 tasks

seratch approved these changes Jul 21, 2021

View reviewed changes

misscoded approved these changes Jul 21, 2021

View reviewed changes

Add basic tests for SocketModeReceiver constructor, start and stop me…

282637f

…thods. (slackapi#750)

filmaj force-pushed the socketmodereceiver-tests branch from 1b6b5d6 to 282637f Compare July 21, 2021 12:49

seratch merged commit 03355fb into slackapi:main Jul 21, 2021

seratch added this to the 3.6.0 milestone Jul 21, 2021

filmaj deleted the socketmodereceiver-tests branch July 21, 2021 17:03

filmaj mentioned this pull request Aug 6, 2021

ESLint Config Migration: Add test-specific ruleset #1044

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for SocketModeReceiver (#750)#1012

Add tests for SocketModeReceiver (#750)#1012
seratch merged 1 commit intoslackapi:mainfrom
filmaj:socketmodereceiver-tests

filmaj commented Jul 20, 2021 •

edited

Loading

Uh oh!

CLAassistant commented Jul 20, 2021 •

edited

Loading

Uh oh!

codecov bot commented Jul 20, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

seratch left a comment

Uh oh!

Uh oh!

seratch Jul 21, 2021

Uh oh!

Uh oh!

misscoded left a comment

Uh oh!

filmaj commented Jul 21, 2021

Uh oh!

seratch commented Jul 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -0,0 +1,288 @@
		/* eslint-disable @typescript-eslint/no-unsafe-call, @typescript-eslint/naming-convention */

Conversation

filmaj commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements (place an x in each [ ])

Uh oh!

CLAassistant commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seratch Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

misscoded left a comment

Choose a reason for hiding this comment

Uh oh!

filmaj commented Jul 21, 2021

Uh oh!

seratch commented Jul 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

filmaj commented Jul 20, 2021 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading