Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable testing for Python 3.12 and PyPy 3.10 on CI #1435

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Enable testing for Python 3.12 and PyPy 3.10 on CI #1435

merged 1 commit into from
Nov 30, 2023

Conversation

Jamim
Copy link
Contributor

@Jamim Jamim commented Nov 29, 2023 •
edited

Summary

Hello,

I believe it would be nice to ensure support for Python 3.12 and PyPy,
so I'd like to suggest changes that:

  • enable testing for Python 3.12 and PyPy 3.10 on CI
  • resolve/suppress some warnings
  • update classifiers at setup.py
  • bump pytest to 7.x
  • update constraints for flake8 to allow pip do its job and resolve compatibility issues
  • refactor timestamp conversion logic in order to resolve a bunch of E721 flake8 errors
  • fix some regexps with invalid escaping at setup.py
  • fix spontaneous failures of TestInteractionsWebsockets.test_interactions

Best regards!

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

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

@codecov Codecov
Copy link

codecov bot commented Nov 29, 2023 •
edited

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (661eeec) 85.38% compared to head (ec27bf0) 85.45%.

Files Patch % Lines
slack_sdk/oauth/installation_store/internals.py 82.60% 4 Missing ⚠️
...dk/oauth/installation_store/models/installation.py 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1435      +/-   ##
==========================================
+ Coverage   85.38%   85.45%   +0.06%     
==========================================
  Files         111      111              
  Lines       12134    12113      -21     
==========================================
- Hits        10361    10351      -10     
+ Misses       1773     1762      -11

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

seratch
seratch requested changes Nov 30, 2023
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for sending such a great pull request! I appreciate your time and efforts here. One thing I would like to revise is that we can simplify the test settings for PyPy runtime. I believe running tests with the latest version should be enough for this project as most users are with CPython, plus I don't think this SDK can have issues with specific version of PyPy considering its functionalities.

.github/workflows/ci-build.yml
matrix:
python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11']
python-version:
- '3.12'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just missing! Thank you!

.github/workflows/ci-build.yml Outdated
@@ -23,12 +36,17 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: pip
- name: Install compatible cryptography
if: matrix.python-version == 'pypy3.7' || matrix.python-version == 'pypy3.6'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like to avoid making the build settings complex just to support old pypy versions. Can you remove pypy 3.6 to 3.9? I don't believe this SDK can cause issues only for old pypy runtimes as this SDK mainly does web API calls, parsing HTTP request data, and database access.

pytest.ini
@@ -6,4 +6,5 @@ log_date_format = %Y-%m-%d %H:%M:%S
filterwarnings =
ignore:"@coroutine" decorator is deprecated since Python 3.8, use "async def" instead:DeprecationWarning
ignore:The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.:DeprecationWarning
ignore:slack.* package is deprecated. Please use slack_sdk.* package instead.*:UserWarning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the warnings are noisy. Filtering them out would be fine

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tests/slack_sdk_async/socket_mode/test_interactions_websockets.py
@@ -90,7 +90,9 @@ async def socket_mode_listener(
expected.sort()

count = 0
while count < 10 and len(received_messages) < len(expected):
while count < 10 and (
len(received_messages) < len(expected) or len(received_socket_mode_requests) < len(socket_mode_envelopes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! It took me well over an hour to figure out what's going on with PyPy (and CPython from time to time) and complete this PR 😅

slack_sdk/oauth/installation_store/internals.py
TimestampType = TypeVar("TimestampType", float, int)


def _timestamp_to_type(ts: Union[TimestampType, datetime, str], target_type: Type[TimestampType]) -> TimestampType:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactoring

@seratch seratch self-assigned this Nov 30, 2023
@seratch seratch added tests M-T: Testing work only web-client oauth labels Nov 30, 2023
@seratch seratch added this to the 3.26.1 milestone Nov 30, 2023
@Jamim
Enable testing for Python 3.12 and PyPy 3.10 on CI
ec27bf0 
These changes also:
  * resolve/suppress some warnings
  * update classifiers at setup.py
  * bump pytest to 7.x
  * update constraints for flake8 to allow pip
    do its job and resolve compatibility issues
  * refactor timestamp conversion logic in order to
    resolve a bunch of E721 flake8 errors
  * fix some regexes with invalid escaping at setup.py
  * fix spontaneous failures of TestInteractionsWebsockets.test_interactions
@Jamim
Copy link
Contributor Author

Jamim commented Nov 30, 2023

Hello @seratch,
Thank you for the review! I've submitted an updated version of the changes.

@Jamim Jamim changed the title Enable testing for Python 3.12 and PyPy 3.6-3.10 on CI Enable testing for Python 3.12 and PyPy 3.10 on CI Nov 30, 2023
seratch
seratch approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me now! Thank you so much again for the great contribution!

@seratch seratch merged commit 9c4d37c into slackapi:main Nov 30, 2023
12 checks passed
@seratch seratch mentioned this pull request Nov 30, 2023
Merged
16 tasks
@Jamim Jamim deleted the feature/python-3.12-on-ci branch November 30, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

Assignees

@seratch seratch

Labels
cla:signed oauth tests M-T: Testing work only web-client
Projects
None yet
Milestone
3.26.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Jamim @seratch