More ExpressReceiver tests #1407
Conversation
| const MockApp = await importApp(overrides); | ||
| // Act | ||
| const app = new MockApp({ token: '', appToken: '', developerMode: true }); | ||
| const app = new MockApp({ logger: fakeLogger, token: '', appToken: '', developerMode: true }); |
There was a problem hiding this comment.
Unrelated to the ExpressReceiver pull requests, but the extra output as I was iterating on tests was annoying me so I decided to silence them.
Codecov Report
@@ Coverage Diff @@
## main #1407 +/- ##
==========================================
+ Coverage 79.61% 82.03% +2.42%
==========================================
Files 17 17
Lines 1452 1453 +1
Branches 428 428
==========================================
+ Hits 1156 1192 +36
+ Misses 199 165 -34
+ Partials 97 96 -1
Continue to review full report at Codecov.
|
| @@ -211,7 +211,7 @@ describe('App built-in middleware and mechanism', () => { | |||
| * @param orderUp The order it should be called when processing middleware up the chain | |||
| */ | |||
| const assertOrderMiddleware = (orderDown: number, orderUp: number) => async ({ next }: { next?: NextFn }) => { | |||
| await delay(100); | |||
| await delay(10); | |||
There was a problem hiding this comment.
Every millisecond counts!
| @@ -258,7 +258,7 @@ export default class ExpressReceiver implements Receiver { | |||
| this.app.use(this.router); | |||
| } | |||
|
|
|||
| private async requestHandler(req: Request, res: Response): Promise<void> { | |||
| public async requestHandler(req: Request, res: Response): Promise<void> { | |||
There was a problem hiding this comment.
I hope moving this to public is OK, otherwise, testing this was extremely difficult - I would need to orchestrate both the Receiver and the App very specifically, and trigger entire middleware chain processing in order to test this private method.
There was a problem hiding this comment.
I'll let Kaz make the call on whether this is okay to move to public.
There was a problem hiding this comment.
Ideally, non-public is safer but I don't see any big risks by doing this. Go ahead 👍
| @@ -500,7 +500,7 @@ export function verifySignatureAndParseBody( | |||
| return parseRequestBody(body, contentType); | |||
| } | |||
|
|
|||
| function buildBodyParserMiddleware(logger: Logger): RequestHandler { | |||
| export function buildBodyParserMiddleware(logger: Logger): RequestHandler { | |||
There was a problem hiding this comment.
Exported this once more for ease of testing. Another option is to move this method to a separate module in its own file. Let me know if anyone has a preference.
There was a problem hiding this comment.
Impressive new tests! I'll hold off on an approval so that @seratch can make the call on switching to public
| @@ -258,7 +258,7 @@ export default class ExpressReceiver implements Receiver { | |||
| this.app.use(this.router); | |||
| } | |||
|
|
|||
| private async requestHandler(req: Request, res: Response): Promise<void> { | |||
| public async requestHandler(req: Request, res: Response): Promise<void> { | |||
There was a problem hiding this comment.
I'll let Kaz make the call on whether this is okay to move to public.
| @@ -500,7 +500,7 @@ export function verifySignatureAndParseBody( | |||
| return parseRequestBody(body, contentType); | |||
| } | |||
|
|
|||
| function buildBodyParserMiddleware(logger: Logger): RequestHandler { | |||
| export function buildBodyParserMiddleware(logger: Logger): RequestHandler { | |||
| @@ -258,7 +258,7 @@ export default class ExpressReceiver implements Receiver { | |||
| this.app.use(this.router); | |||
| } | |||
|
|
|||
| private async requestHandler(req: Request, res: Response): Promise<void> { | |||
| public async requestHandler(req: Request, res: Response): Promise<void> { | |||
There was a problem hiding this comment.
Ideally, non-public is safer but I don't see any big risks by doing this. Go ahead 👍
|
@filmaj Thanks a lot for adding these tests! |
Summary
Just trying to push the code coverage up