Skip to content

Comments

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

Merged
seratch merged 1 commit intoslackapi:mainfrom
Jamim:feature/python-3.12-on-ci
Nov 30, 2023
Merged

Enable testing for Python 3.12 and PyPy 3.10 on CI#1435
seratch merged 1 commit intoslackapi:mainfrom
Jamim:feature/python-3.12-on-ci

Conversation

@Jamim
Copy link
Contributor

@Jamim Jamim commented Nov 29, 2023 •
edited
Loading

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
Copy link

codecov bot commented Nov 29, 2023 •
edited
Loading

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 suggested changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@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
Contributor

@seratch seratch Nov 30, 2023

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
python-version: ${{ matrix.python-version }}
cache: pip
- name: Install compatible cryptography
if: matrix.python-version == 'pypy3.7' || matrix.python-version == 'pypy3.6'
Copy link
Contributor

@seratch seratch Nov 30, 2023

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
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
Contributor

@seratch seratch Nov 30, 2023

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

tests/slack_sdk_async/socket_mode/test_interactions_websockets.py
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
Contributor

@seratch seratch Nov 30, 2023

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

@Jamim Jamim Nov 30, 2023

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
Contributor

@seratch seratch Nov 30, 2023

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 Jamim force-pushed the feature/python-3.12-on-ci branch from dcc95b6 to ec27bf0 Compare November 30, 2023 01:45
@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
Contributor

@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
@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

1 more reviewer

@seratch seratch seratch approved these changes

Reviewers whose approvals may not affect merge requirements

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.

2 participants

@Jamim @seratch