test: confirm correct app.function handler handling by zimeg · Pull Request #2139 · slackapi/bolt-js

zimeg · 2024-06-19T17:41:20Z

Summary

This PR tests the implementation of app.function handlers across a few cases with some refactors and fixes found along the way.

Changes that extend those found in #2026 and #2128:

Listener middleware can now be used with app.function
Types are exported from the types directory
Inline documentation is added for editor hover hints

Preview

Experiment with an app that uses listener middleware and functions with actions after checking out and building this branch:

$ slack create baker -t zimeg/slacks -b js.bolt.cookies
$ cd baker
$ npm install ../../path/to/bolt-js
$ slack run

Then add the cookies function as a step to a workflow.

Reviewers

The tests in src/App-custom-function.spec.ts cover the use cases I could think of! Please let me know of others that should be considered 🙏

Notes

Will follow up with updates to https://slack.dev in another PR to avoid cluttering this diff too much!

Requirements

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.

* ensure custom function middleware uses the configured token * ensure action handlers have middleware for function executions * ensure listener middleware is honored in function handlers * ensure custom function middleware arguments call correct apis * ensure custom function handlers are setup with valid arguments * document usage of attributes and handlers inline with jsdoc * refactor logic and types into custom function specific files

codecov · 2024-06-19T18:58:19Z

Codecov Report

Attention: Patch coverage is 88.23529% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.49%. Comparing base (ab80d52) to head (54f3692).

Files	Patch %	Lines
src/CustomFunction.ts	84.90%	0 Missing and 8 partials ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##           pre-stable-fixes    #2139      +/-   ##
====================================================
+ Coverage             80.87%   82.49%   +1.61%     
====================================================
  Files                    19       20       +1     
  Lines                  1637     1617      -20     
  Branches                461      461              
====================================================
+ Hits                   1324     1334      +10     
+ Misses                  205      179      -26     
+ Partials                108      104       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

WilliamBergamin

Good job 💯
Left a few comments

src/App-custom-function.spec.ts

WilliamBergamin · 2024-06-19T20:31:46Z

src/App-custom-function.spec.ts

+ * Overrides for spying on mocks are below this comment
+ */
+
+async function importApp(


This is nice 💯!
Any chance we could bring it out of this file so the pattern can be reused by other tests? No worries if you don't have time

WilliamBergamin · 2024-06-19T20:32:54Z

src/App.ts

  /**
- * Register CustomFunction middleware
- */
+   * Custom function listener and middleware that executes for a matching function callbackId.


src/App-custom-function.spec.ts

WilliamBergamin · 2024-06-19T20:50:35Z

src/CustomFunction.ts

+   * @param context - the incoming payload context.
+   * @link https://github.com/slackapi/bolt-js/pull/2026#discussion_r1467123047
+   */
+  private static selectToken(context: Context): string | undefined {


I think this logic is duplicated in the App.ts 🤔
Might be a good idea to consolidate it

Great call once more! Had to think a bit about where this might make sense - exporting from App.ts into CustomFunction.ts felt strange - and ended up creating middleware/context.ts for similar context operations. Open to adjustments though ✨

src/middleware/process.ts

Co-authored-by: William Bergamin <william.bergamin.coen@gmail.com>

zimeg · 2024-06-21T18:13:47Z

@WilliamBergamin appreciate all of those suggestions and the review. The code gets pushed in a better direction with each. I'm feeling solid about these changes and am interested in tagging a new beta with it, perhaps today, since most of the remaining changes seem to be testing related 👀

zimeg · 2024-06-21T22:28:30Z

Update: Holding off on tagging another beta with these changes - will mark it as draft for following up sometime soon.

WilliamBergamin

Thanks a lot for addressing all my comments 🙏

I went over the changes one last time and added one last comment that I was previously on the fence about, let me know what you think

Also I think we can move this PR out of draft

WilliamBergamin · 2024-06-25T14:01:31Z

src/types/middleware.ts

-  'functionInputs',
  'retryNum',
  'retryReason',
+  ...Array<keyof CustomFunctionContext>(),


What do you think about explicitly defining fields?

I think it can make the code clearer for future maintainers and prevent a field from being accidentally added through an inherited object 🤔

I'm partial to both approaches! The magic is nice to avoid forgetting to add it to context and needing to hunt for these fields, but magic is a dangerous art 😳

misscoded and others added 4 commits June 5, 2024 19:45

Fixes and polish for stable release

3c6492e

chore: merge w main

348fb6f

chore: merge w 'feat-functions'

c1a9127

zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch tests M-T: Testing work only labels Jun 19, 2024

zimeg self-assigned this Jun 19, 2024

zimeg added 2 commits June 19, 2024 11:55

chore: merge w 'feat-functions'

ab80d52

chore: merge w 'pre-stable-fixes'

c2d8184

WilliamBergamin requested changes Jun 19, 2024

View reviewed changes

zimeg and others added 7 commits June 20, 2024 09:49

test(fix): instantiate a new fake receiver before each test

2f5da21

refactor(test): initialize a constant response for asserts

a818873

test(fix): assert failure with builtin asserts for failure cases

d30153f

Co-authored-by: William Bergamin <william.bergamin.coen@gmail.com>

test(fix): quote templated strings with backtick for escape

6eb5fcc

test: confirm a fail argument is provided to action handlers

8a85665

test: confirm false attachments of bot tokens is correct on its own

fa31167

refactor: remove repeated logic for selecting a context token

54f3692

zimeg requested a review from WilliamBergamin June 21, 2024 18:12

zimeg marked this pull request as draft June 21, 2024 22:28

WilliamBergamin requested changes Jun 25, 2024

View reviewed changes

misscoded force-pushed the pre-stable-fixes branch 2 times, most recently from 9df1e68 to f7966e7 Compare August 7, 2024 21:45

filmaj deleted the branch slackapi:pre-stable-fixes August 12, 2024 19:20

filmaj closed this Aug 12, 2024

zimeg deleted the zimeg/test-custom-functions branch August 12, 2024 20:44

Conversation

zimeg commented Jun 19, 2024

Summary

Preview

Reviewers

Notes

Requirements

Uh oh!

codecov bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

WilliamBergamin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WilliamBergamin Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WilliamBergamin Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zimeg commented Jun 21, 2024

Uh oh!

zimeg commented Jun 21, 2024

Uh oh!

WilliamBergamin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 19, 2024 •

edited

Loading

WilliamBergamin Jun 19, 2024 •

edited

Loading

WilliamBergamin left a comment •

edited

Loading

WilliamBergamin Jun 25, 2024 •

edited

Loading