Replace Flask-Sockets with aiohttp for testing #1012
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
=======================================
Coverage 91.76% 91.76%
=======================================
Files 181 181
Lines 6312 6312
=======================================
Hits 5792 5792
Misses 520 520 ☔ View full report in Codecov by Sentry. |
.github/workflows/tests.yml
Outdated
| @@ -23,7 +23,7 @@ jobs: | |||
| run: | | |||
| pip install -U pip | |||
| pip install -r requirements.txt | |||
| pip install -r requirements/testing_without_asyncio.txt | |||
There was a problem hiding this comment.
Can you revert this change? This part intentionally does not have aiohttp to detect unexectedly having aiohttp depedency to the core part (as the following name "Run tests without aiohttp" indicate)
There was a problem hiding this comment.
I've reverted this change but aoihttp will be required to run pytest tests/adapter_tests/socket_mode/, we can try using another synchronous library or move the CI step Run tests for Socket Mode adapters after Run tests for HTTP Mode adapters (asyncio-based libraries) let me know what you think
There was a problem hiding this comment.
pytest tests/adapter_tests/socket_mode/
Sorry, I should have clearly mentioned this but adding aiohttp for these tests is totally fine(indeed using a sync solution is ideal though). My concern was about tests under slack_sdk etc.
There was a problem hiding this comment.
Ohh I see, I've update the ci tests and dependencies to make them pass, let me know if we prefer a sync approach to this?
There was a problem hiding this comment.
Thank you! The only risk is that we may unintentionally add aiohttp imports to the non-asyncio Socket Mode adapters. Thus, making the changes only to the adapter tests should be safe enough. We can detect such in code reviews.
This PR bring the logic from slack-sdk #1445 to bolt python
Flask-Socketsis deprecated and it won't receive any fixes in the future, so it seems reasonable to replace it with some up-to-date library.aiohttphas been chosen since it was already presented as an optional requirement.Category (place an
xin each of the[ ])slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements (place an
xin 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.
./scripts/install_all_and_run_tests.shafter making the changes.