refactor: drop requests dep, route all providers through lib/http urllib wrapper (#393)
Five provider modules (pinterest, threads, instagram, tiktok, youtube_yt) and watchlist.py each carried a try/except `requests` import with parallel urllib + requests branches. The urllib path already used the stdlib-only wrapper at `lib/http.py` (retries, 429 handling, HTTPError). This collapses every dual-branch into a single `http.get`/`http.post` call and removes the `requests` dependency from `pyproject.toml`. Also drops 4 transitive deps (urllib3, certifi, charset-normalizer, idna) from the lockfile, leaving the skill stdlib-only at runtime. Tests for tiktok comments and watchlist delivery were rewritten to mock `lib.http` directly instead of the now-removed `requests` module. Out of scope but flagged during review: the 13 surviving SC call sites share a near-identical scaffold and would benefit from a `http.scrapecreators_get(url, params, token, ...)` helper. Filed for a follow-up PR rather than expanding scope here.
This commit is contained in:
Trevin Chow
@@ -2,13 +2,14 @@
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from unittest.mock import Mock, patch
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent / "skills" / "last30days" / "scripts"))
|
||||
|
||||
import watchlist
|
||||
from lib.http import HTTPError
|
||||
|
||||
|
||||
# === Tests for _format_delivery_message() ===
|
||||
@@ -18,7 +19,7 @@ def test_format_message_announce_mode():
|
||||
message = watchlist._format_delivery_message(
|
||||
"Test Topic", {"new": 5, "updated": 2}, "announce"
|
||||
)
|
||||
|
||||
|
||||
assert "📰" in message
|
||||
assert "Test Topic" in message
|
||||
assert "5 new" in message
|
||||
@@ -30,7 +31,7 @@ def test_format_message_silent_mode():
|
||||
message = watchlist._format_delivery_message(
|
||||
"Test Topic", {"new": 5, "updated": 2}, "silent"
|
||||
)
|
||||
|
||||
|
||||
assert "📰" not in message
|
||||
assert "Test Topic" in message
|
||||
assert "5 new" in message
|
||||
@@ -41,7 +42,7 @@ def test_format_message_default_mode():
|
||||
message = watchlist._format_delivery_message(
|
||||
"Test Topic", {"new": 5, "updated": 2}, "default"
|
||||
)
|
||||
|
||||
|
||||
assert "complete" in message.lower()
|
||||
assert "Test Topic" in message
|
||||
|
||||
@@ -51,44 +52,35 @@ def test_format_message_handles_zero_counts():
|
||||
message = watchlist._format_delivery_message(
|
||||
"Test Topic", {"new": 0, "updated": 0}, "announce"
|
||||
)
|
||||
|
||||
|
||||
assert "0 new" in message
|
||||
assert "0 updated" in message
|
||||
|
||||
|
||||
# === Tests for _send_slack_webhook() ===
|
||||
|
||||
@patch('watchlist.requests')
|
||||
def test_send_slack_webhook_format(mock_requests):
|
||||
@patch('watchlist.http.post')
|
||||
def test_send_slack_webhook_format(mock_post):
|
||||
"""Test that Slack webhook uses correct format."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
watchlist._send_slack_webhook(
|
||||
"https://hooks.slack.com/services/TEST",
|
||||
"Test message"
|
||||
)
|
||||
|
||||
# Verify POST was called with correct format
|
||||
assert mock_requests.post.called
|
||||
call_args = mock_requests.post.call_args
|
||||
|
||||
|
||||
assert mock_post.called
|
||||
call_args = mock_post.call_args
|
||||
|
||||
assert call_args[0][0] == "https://hooks.slack.com/services/TEST"
|
||||
assert call_args[1]["json"] == {"text": "Test message"}
|
||||
assert call_args[1]["headers"]["Content-Type"] == "application/json"
|
||||
assert call_args[1]["json_data"] == {"text": "Test message"}
|
||||
assert call_args[1]["timeout"] == 10
|
||||
|
||||
|
||||
@patch('watchlist.requests')
|
||||
def test_send_slack_webhook_raises_on_error(mock_requests):
|
||||
@patch('watchlist.http.post')
|
||||
def test_send_slack_webhook_raises_on_error(mock_post):
|
||||
"""Test that Slack webhook raises on HTTP error."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 400
|
||||
mock_response.raise_for_status.side_effect = Exception("HTTP 400")
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
with pytest.raises(Exception, match="HTTP 400"):
|
||||
mock_post.side_effect = HTTPError("HTTP 400", 400)
|
||||
|
||||
with pytest.raises(HTTPError, match="HTTP 400"):
|
||||
watchlist._send_slack_webhook(
|
||||
"https://hooks.slack.com/services/TEST",
|
||||
"Test message"
|
||||
@@ -97,40 +89,32 @@ def test_send_slack_webhook_raises_on_error(mock_requests):
|
||||
|
||||
# === Tests for _send_generic_webhook() ===
|
||||
|
||||
@patch('watchlist.requests')
|
||||
def test_send_generic_webhook_format(mock_requests):
|
||||
@patch('watchlist.http.post')
|
||||
def test_send_generic_webhook_format(mock_post):
|
||||
"""Test that generic webhook uses correct format."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
watchlist._send_generic_webhook(
|
||||
"https://webhook.example.com/hook",
|
||||
"Test message"
|
||||
)
|
||||
|
||||
# Verify POST was called with correct format
|
||||
assert mock_requests.post.called
|
||||
call_args = mock_requests.post.call_args
|
||||
|
||||
|
||||
assert mock_post.called
|
||||
call_args = mock_post.call_args
|
||||
|
||||
assert call_args[0][0] == "https://webhook.example.com/hook"
|
||||
|
||||
json_data = call_args[1]["json"]
|
||||
|
||||
json_data = call_args[1]["json_data"]
|
||||
assert json_data["message"] == "Test message"
|
||||
assert json_data["source"] == "last30days"
|
||||
assert "timestamp" in json_data
|
||||
assert isinstance(json_data["timestamp"], float)
|
||||
|
||||
|
||||
@patch('watchlist.requests')
|
||||
def test_send_generic_webhook_raises_on_error(mock_requests):
|
||||
@patch('watchlist.http.post')
|
||||
def test_send_generic_webhook_raises_on_error(mock_post):
|
||||
"""Test that generic webhook raises on HTTP error."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 500
|
||||
mock_response.raise_for_status.side_effect = Exception("HTTP 500")
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
with pytest.raises(Exception, match="HTTP 500"):
|
||||
mock_post.side_effect = HTTPError("HTTP 500", 500)
|
||||
|
||||
with pytest.raises(HTTPError, match="HTTP 500"):
|
||||
watchlist._send_generic_webhook(
|
||||
"https://webhook.example.com/hook",
|
||||
"Test message"
|
||||
@@ -140,150 +124,120 @@ def test_send_generic_webhook_raises_on_error(mock_requests):
|
||||
# === Tests for _deliver_findings() ===
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_sends_when_new_greater_than_zero(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_sends_when_new_greater_than_zero(mock_post, mock_get_setting):
|
||||
"""Test that delivery fires when new > 0."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
# Verify webhook was called
|
||||
assert mock_requests.post.called
|
||||
|
||||
assert mock_post.called
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_skips_when_new_is_zero(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_skips_when_new_is_zero(mock_post, mock_get_setting):
|
||||
"""Test that delivery is skipped when new=0."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 0, "updated": 5})
|
||||
|
||||
# Verify webhook was NOT called
|
||||
assert not mock_requests.post.called
|
||||
|
||||
assert not mock_post.called
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_skips_when_channel_empty(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_skips_when_channel_empty(mock_post, mock_get_setting):
|
||||
"""Test that delivery is skipped when delivery_channel is empty."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
# Verify webhook was NOT called
|
||||
assert not mock_requests.post.called
|
||||
|
||||
assert not mock_post.called
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_uses_slack_format_for_slack_urls(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_uses_slack_format_for_slack_urls(mock_post, mock_get_setting):
|
||||
"""Test that Slack URLs trigger Slack-specific format."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://hooks.slack.com/services/TEST",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
# Verify Slack format was used
|
||||
call_args = mock_requests.post.call_args
|
||||
json_data = call_args[1]["json"]
|
||||
|
||||
json_data = mock_post.call_args[1]["json_data"]
|
||||
assert "text" in json_data
|
||||
assert "Test Topic" in json_data["text"]
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_uses_generic_format_for_other_urls(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_uses_generic_format_for_other_urls(mock_post, mock_get_setting):
|
||||
"""Test that non-Slack URLs trigger generic format."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
# Verify generic format was used
|
||||
call_args = mock_requests.post.call_args
|
||||
json_data = call_args[1]["json"]
|
||||
|
||||
json_data = mock_post.call_args[1]["json_data"]
|
||||
assert "message" in json_data
|
||||
assert "source" in json_data
|
||||
assert "timestamp" in json_data
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_handles_failure_gracefully(mock_requests, mock_get_setting, capsys):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_handles_failure_gracefully(mock_post, mock_get_setting, capsys):
|
||||
"""Test that delivery failures don't crash the process."""
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
# Simulate HTTP error
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 500
|
||||
mock_response.raise_for_status.side_effect = Exception("HTTP 500")
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
|
||||
mock_post.side_effect = HTTPError("HTTP 500", 500)
|
||||
|
||||
# Should not raise, just log to stderr
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
# Verify error was logged
|
||||
|
||||
captured = capsys.readouterr()
|
||||
assert "Delivery failed" in captured.err
|
||||
|
||||
|
||||
@patch('watchlist.store.get_setting')
|
||||
@patch('watchlist.requests')
|
||||
def test_deliver_findings_respects_delivery_mode(mock_requests, mock_get_setting):
|
||||
@patch('watchlist.http.post')
|
||||
def test_deliver_findings_respects_delivery_mode(mock_post, mock_get_setting):
|
||||
"""Test that different delivery modes produce different messages."""
|
||||
mock_response = Mock()
|
||||
mock_response.status_code = 200
|
||||
mock_requests.post.return_value = mock_response
|
||||
|
||||
# Test announce mode
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "announce",
|
||||
}.get(key, default)
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
announce_message = mock_requests.post.call_args[1]["json"]["message"]
|
||||
|
||||
announce_message = mock_post.call_args[1]["json_data"]["message"]
|
||||
assert "📰" in announce_message
|
||||
|
||||
# Test silent mode
|
||||
|
||||
mock_get_setting.side_effect = lambda key, default="": {
|
||||
"delivery_channel": "https://webhook.example.com/test",
|
||||
"delivery_mode": "silent",
|
||||
}.get(key, default)
|
||||
|
||||
|
||||
watchlist._deliver_findings("Test Topic", {"new": 5, "updated": 2})
|
||||
|
||||
silent_message = mock_requests.post.call_args[1]["json"]["message"]
|
||||
|
||||
silent_message = mock_post.call_args[1]["json_data"]["message"]
|
||||
assert "📰" not in silent_message
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user