Skip to content

Fixed fetching of names for callable objects (#215) - #216

Merged
seratch merged 10 commits into
slackapi:mainfrom
nickovs:middleware-class-names
Jan 20, 2021
Merged

seratch merged 10 commits into
slackapi:mainfrom
nickovs:middleware-class-names

Conversation

@nickovs

@nickovs nickovs commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

Added new function name_for_callable() in util.utils.
Fixed name properties for CustomMiddleware and AsyncCustomMiddleware to use the new function.
Modified listener/thread_runner.py and listener/asyncio_runner.py to use the new function to name lazy functions.
Added test cases for middleware and lazy function cases.

This addresses the issue in #215.

Category (place an x in each of the [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • 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.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

Added new function name_for_callable() in util.utils.
Fixed name properties for CustomMiddleware and AsyncCustomMiddleware to use the new function.
Modified listener/thread_runner.py and listener/asyncio_runner.py to use the new function to name lazy functions.
Added test cases for middleware and lazy function cases.
@CLAassistant

CLAassistant commented Jan 20, 2021 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mwbrooks
mwbrooks requested a review from seratch January 20, 2021 01:26
@mwbrooks mwbrooks added bug Something isn't working area:async area:sync labels Jan 20, 2021
@mwbrooks
mwbrooks self-requested a review January 20, 2021 01:28
@mwbrooks

Copy link
Copy Markdown
Member

Thanks for the PR @nickovs! 🎸

When you have a moment, would you mind signing the CLA so that we can merge your work? It's quick and digital, just select the link from the CLAssistant.

In the meantime, we'll review it and let the CI run the tests 🙂

@nickovs

nickovs commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

@mwbrooks I have signed the CLA. There seems to be a bug in your CLA assistant. There are two different email addresses in my commits (one home, one work). Both are linked to my GitHub account and the commit history for my branch correctly shows both commits as belonging to the @nickovs user, but for some reason the CLA Assistant is only showing one identity as having signed the CLA. When I click on the link to sign it again it says that I have already signed the CLA and won't let me Sogn it again.

@seratch

seratch commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

@nickovs Could you squash them into one commit using the email address used for signing the CLA?

nickovs and others added 6 commits January 19, 2021 19:18
Added new function name_for_callable() in util.utils.
Fixed name properties for CustomMiddleware and AsyncCustomMiddleware to use the new function.
Modified listener/thread_runner.py and listener/asyncio_runner.py to use the new function to name lazy functions.
Added test cases for middleware and lazy function cases.
@nickovs

nickovs commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

OK. I did a git commit --amend ... and that seems to have satisfied the CLA Assistant.

@seratch

seratch commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

@nickovs Thanks for promptly submitting this PR! I am going to check this in detail later today (I live in +09:00 timezone). One thing, it seems the following tests for adapters fro AWS Lamdba are failing. Have you already checked this out?

FAILED tests/adapter_tests/test_aws_chalice.py::TestAwsChalice::test_lazy_listeners
FAILED tests/adapter_tests/test_aws_lambda.py::TestAWSLambda::test_lazy_listeners

@seratch seratch self-assigned this Jan 20, 2021
@seratch seratch added this to the 1.2.3 milestone Jan 20, 2021
@nickovs

nickovs commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

@seratch Regarding the tests, I just noticed that. When I run the tests locally, whether on the main branch or on my new code, several of the tests are very unreliable, frequently failing with ConnectionResetError: [Errno 54] Connection reset by peer exceptions trying to talk to the mock servers. I was just trying to separate out if these issues in the lazy listeners are to do with my code or the generally flakey test harness.

@seratch

seratch commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

several of the tests are very unreliable, frequently failing with ConnectionResetError: [Errno 54] Connection reset by peer exceptions trying to talk to the mock servers.

Yes ... this is a known issue that happens on local machine. #157

Re-running the same tests by ./scripts/run_tests.sh {test file path} should help for most cases. e.g.,

./scripts/run_tests.sh tests/adapter_tests/test_aws_chalice.py
./scripts/run_tests.sh tests/adapter_tests/

@nickovs

nickovs commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

Actually it was just a dumb cut-and-paste error. I've pushed a fix.

I added type hints to the new name_for_callable() utility function while I was there.

@codecov

codecov Bot commented Jan 20, 2021

Copy link
Copy Markdown

Codecov Report

Merging #216 (53917de) into main (03a0ad4) will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
+ Coverage   91.61%   91.62%   +0.01%     
==========================================
  Files         159      159              
  Lines        4760     4767       +7     
==========================================
+ Hits         4361     4368       +7     
  Misses        399      399              
Impacted Files Coverage Δ
slack_bolt/listener/asyncio_runner.py 84.61% <50.00%> (ø)
slack_bolt/listener/thread_runner.py 89.62% <75.00%> (ø)
slack_bolt/middleware/async_custom_middleware.py 96.15% <100.00%> (+0.15%) ⬆️
slack_bolt/middleware/custom_middleware.py 100.00% <100.00%> (ø)
slack_bolt/util/utils.py 94.11% <100.00%> (+1.01%) ⬆️

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 03a0ad4...53917de. Read the comment docs.

@seratch

seratch commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

Thanks - LGTM

@seratch
seratch merged commit f0ee5a1 into slackapi:main Jan 20, 2021
seratch added a commit that referenced this pull request Jan 20, 2021
@nickovs
nickovs deleted the middleware-class-names branch June 3, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:async area:sync bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants