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.

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

tests: ensure consistent behavior around url format #1600

Merged

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This PR adds unit test that validate the url used to send a request to Slack.

Testing

NA

Category

  • 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 (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements

  • 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.

@WilliamBergamin WilliamBergamin added the tests M-T: Testing work only label Nov 22, 2024
@WilliamBergamin WilliamBergamin self-assigned this Nov 22, 2024
@WilliamBergamin
Copy link
Contributor Author

I think the urlopen spying I implemented may be messing with test that proceed it 🤔 maybe there is a better approach to this?

@seratch
Copy link
Member

seratch commented Nov 25, 2024

@WilliamBergamin How about simply spinning a mock web server up and verifying the request path on the mock server side? We have some test utilities doing so, so it'd be much easier to set up.

@WilliamBergamin
Copy link
Contributor Author

@seratch this was my original approach but it seemed like the mock server did not support verifying the request path

I've brought over some changes we applied to the mock server in the bolt project, this resolved the issue 💯
I'll try to send a follow up PR to clean up this area of the tests, I believe we only use the threaded mock server implementation I think we can remove the multiprocessing logic

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.

@WilliamBergamin Merging #1602 before this caused git conflicts on this PR. Once they're resolved, this PR looks great to me

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.91%. Comparing base (860bf9b) to head (67dc5be).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1600      +/-   ##
==========================================
+ Coverage   84.89%   84.91%   +0.01%     
==========================================
  Files         113      113              
  Lines       12622    12622              
==========================================
+ Hits        10716    10718       +2     
+ Misses       1906     1904       -2     

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

@WilliamBergamin WilliamBergamin merged commit 5464f89 into slackapi:main Nov 27, 2024
12 checks passed
@WilliamBergamin WilliamBergamin deleted the add-tests-around-url-requests branch November 27, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants