Skip to content

Fix WebSocket related unit test failures#1072

Merged
seratch merged 3 commits intoslackapi:mainfrom
seratch:ws-unit-test-failure
Jul 29, 2021
Merged

Fix WebSocket related unit test failures#1072
seratch merged 3 commits intoslackapi:mainfrom
seratch:ws-unit-test-failure

Conversation

@seratch
Copy link
Copy Markdown
Contributor

@seratch seratch commented Jul 28, 2021

Summary

This pull request fixes the test failures for Socket Mode modules: #1071 (comment)

Werkzeug v2 was released in May. Flask-Sockets, the package we use for Socket Mode testing, is not yet compatible with the major version. This pull request updates the build settings to set Werkzeug's major version to 1 for now.

See also: slackapi/bolt-python#340

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 ./docs.sh?)
  • /docs-src-v2 (Documents, have you run ./docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • 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 python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@seratch seratch added tests M-T: Testing work only Version: 3x socket-mode labels Jul 28, 2021
@seratch seratch added this to the 3.9.0 milestone Jul 28, 2021
@seratch seratch requested a review from filmaj July 28, 2021 06:27
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 28, 2021 •
edited
Loading

Codecov Report

Merging #1072 (05a6da7) into main (6f369e4) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1072      +/-   ##
==========================================
+ Coverage   84.15%   84.19%   +0.03%     
==========================================
  Files          99       99              
  Lines        9255     9255              
==========================================
+ Hits         7789     7792       +3     
+ Misses       1466     1463       -3
Impacted Files Coverage Δ
slack_sdk/socket_mode/client.py 72.56% <0.00%> (+2.65%) ⬆️

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 6f369e4...05a6da7. Read the comment docs.

@seratch seratch mentioned this pull request Jul 28, 2021
Merged
16 tasks
filmaj
filmaj approved these changes Jul 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@filmaj filmaj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes, this helps a lot! This unblocked the tests/slack_sdk/socket_mode/test_websocket_client.py test for me. I still have a few failures to work through on my local machine, though. It is the end of my work day but I will look into the failures more deeply tomorrow.

EDIT: summary of my failures on my machine now is:

=============================== short test summary info ================================
FAILED tests/slack_sdk/scim/test_client.py::TestSCIMClient::test_groups - ConnectionR...
FAILED tests/slack_sdk/web/test_web_client.py::TestWebClient::test_issue_690_oauth_access
FAILED tests/web/test_web_client.py::TestWebClient::test_issue_690_oauth_access - Con...

Looks like all due to ConnectionResetByPeer - so perhaps related to that other issue you linked to previously. I will try re-running the tests a few times to see if this is some kind of test instability.

@seratch
Copy link
Copy Markdown
Contributor Author

seratch commented Jul 29, 2021

Thanks for sharing the details of the errors you've faced.

Looks like all due to ConnectionResetByPeer - so perhaps related to that other issue you linked to previously. I will try re-running the tests a few times to see if this is some kind of test instability.

Implementing auto-retry feature, which is the main feature addition in the next release, will mitigate the issue. I will start working on it shortly.

It seems that everything around this pull request looks fine. Let me merge this one.

@seratch seratch merged commit 553e3c1 into slackapi:main Jul 29, 2021
@seratch seratch deleted the ws-unit-test-failure branch July 29, 2021 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@filmaj filmaj filmaj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

socket-mode tests M-T: Testing work only Version: 3x

Projects

None yet

Milestone

3.9.0

Development

Successfully merging this pull request may close these issues.

2 participants

@seratch @filmaj