Add more unit tests for built-in middleware and ExpressReceiver #357
Conversation
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
+ Coverage 73% 83.2% +10.19%
==========================================
Files 7 7
Lines 500 500
Branches 146 146
==========================================
+ Hits 365 416 +51
+ Misses 103 55 -48
+ Partials 32 29 -3
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| it('should verify requests', async () => { | ||
| let errorResult: any; | ||
| runWithValidRequest(buildExpressRequest(), errorResult); | ||
| const state: any = {}; |
There was a problem hiding this comment.
The tests I added before failed to detect failures while parsing the body. This pull request also fixes it.
| @@ -29,8 +29,9 @@ | |||
| "build": "tsc", | |||
| "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output", | |||
| "lint": "tslint --project .", | |||
| "test-lint": "tslint \"src/**/*.spec.ts\" && tslint \"src/test-helpers.ts\"", | |||
There was a problem hiding this comment.
I noticed tslint has not been enabled for unit tests. I've enabled it and fixed all the warnings.
There was a problem hiding this comment.
I think it is fine to run linting on our tests as it encourages we use the same code style.
Question, why don't we just include linting the tests as part of the lint command instead of creating a test-lint command. I think we would just need to update https://github.com/slackapi/bolt/blob/master/tsconfig.json#L66-L67
There was a problem hiding this comment.
@stevengill In my understanding, the reason why they're excluded is to avoid including spec files in npm package. Do you have some ideas to simplify the configuration without breaking anything?
There was a problem hiding this comment.
Oh I didn't know that. Thanks for the explanation.
10.2 % coverage increased 🎉 |
| @@ -29,8 +29,9 @@ | |||
| "build": "tsc", | |||
| "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output", | |||
| "lint": "tslint --project .", | |||
| "test-lint": "tslint \"src/**/*.spec.ts\" && tslint \"src/test-helpers.ts\"", | |||
There was a problem hiding this comment.
I think it is fine to run linting on our tests as it encourages we use the same code style.
Question, why don't we just include linting the tests as part of the lint command instead of creating a test-lint command. I think we would just need to update https://github.com/slackapi/bolt/blob/master/tsconfig.json#L66-L67
|
@stevengill Thanks for your review. Can you approve this PR? |
Summary
This pull request adds more unit tests for
ExpressReceiverand built-in middleware. It improves the code coverage by5.44%5.8% (from 73.00% to 78.80%).https://codecov.io/gh/seratch/bolt/tree/702ca613900a36ca09147cf086c67cc58522c4c7
The only changes introduced to the main code is allowing users to access the following functions.
I believe there is no issue with it. But, if there is a plan to change the internals of
ExpressReceivera lot, I can put it on hold until then. I myself don't have such plans.Requirements (place an
xin each[ ])