Skip to content

Commit 8d32182

Browse files
author
Irving Popovetsky
committed
Add 113+ tests expanding coverage from 32% to 66% and fix 4 critical bugs
Added comprehensive test suite (244 total tests, all passing) covering mentor request/volunteer flows, action handlers, and event utilities. Fixed high-severity bugs: missing error handling in _post_messages causing KeyError crashes, validation incorrectly accepting empty skillsets, KeyError in SlackAPIError handling, and incorrect state access in report dialog. Created reusable mock infrastructure for Slack/Airtable APIs.
1 parent fecd71a commit 8d32182

20 files changed

+3736
-7
lines changed

poetry.lock

Lines changed: 127 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pybot/endpoints/airtable/utils.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import logging
2+
13
from pybot._vendor.sirbot import SirBot
24
from pybot._vendor.slack import ROOT_URL, methods
35
from pybot._vendor.slack.events import Message
@@ -8,6 +10,8 @@
810

911
from .message_templates.messages import claim_mentee_attachment, mentor_request_text
1012

13+
logger = logging.getLogger(__name__)
14+
1115

1216
async def _get_requested_mentor(
1317
requested_mentor: str | None, slack: SlackAPI, airtable: AirtableAPI
@@ -88,7 +92,12 @@ def _create_messages(
8892

8993
async def _post_messages(parent: Message, children: list[Message], app: SirBot) -> None:
9094
response = await app.plugins["slack"].api.query(url=methods.CHAT_POST_MESSAGE, data=parent)
91-
timestamp = response["ts"]
95+
96+
# Safely get timestamp from response - if missing, children won't be threaded
97+
timestamp = response.get("ts")
98+
if not timestamp:
99+
logger.warning("Slack response missing 'ts' field - children messages will not be threaded")
100+
return
92101

93102
for child in children:
94103
child["thread_ts"] = timestamp

pybot/endpoints/slack/actions/mentor_volunteer.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ async def submit_mentor_volunteer(action: Action, app: SirBot) -> None:
5656
{"channel": MENTOR_CHANNEL, "users": [user_id]},
5757
)
5858
except SlackAPIError as error:
59-
logger.debug("Error during mentor channel invite %s", error.data["errors"])
59+
logger.debug("Error during mentor channel invite %s", error.data.get("errors", error.data))
6060

6161
request.on_submit_success()
6262

pybot/endpoints/slack/actions/report_message.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ async def send_report(action: Action, app: SirBot):
1616
"""
1717
slack_id = action["user"]["id"]
1818
details = action["submission"]["details"]
19-
message_details = json.loads(action.action["state"])
19+
message_details = json.loads(action["state"])
2020

2121
response = build_report_message(slack_id, details, message_details)
2222

pybot/endpoints/slack/message_templates/mentor_volunteer.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ def clear_skillsets(self) -> None:
4343
self.skillset_field_text = " "
4444

4545
def validate_self(self):
46-
if not self.skillsets:
46+
# Filter out empty/whitespace-only skillsets
47+
valid_skillsets = [s.strip() for s in self.skillsets if s.strip()]
48+
if not valid_skillsets:
4749
return False
4850

4951
self.clear_errors()

pyproject.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pytest-mock = "^3.12"
2323
black = "^25.0"
2424
ruff = "^0.14"
2525
requests = "^2.31"
26+
pytest-cov = "^7.0.0"
2627

2728
[tool.poetry.scripts]
2829
pybot-manage = "manage:main"

tests/conftest.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import copy
2+
from unittest.mock import AsyncMock, MagicMock
23

34
import pytest
45

@@ -7,6 +8,7 @@
78
from pybot._vendor.sirbot.plugins.slack import SlackPlugin
89
from pybot.plugins import AirtablePlugin, APIPlugin
910
from tests import data
11+
from tests.fixtures import SlackMock, AirtableMock, AdminSlackMock
1012

1113
pytest_plugins = ("pybot._vendor.slack.tests.plugin",)
1214

@@ -69,3 +71,78 @@ async def slack_bot(bot: SirBot):
6971
# Manually initialize the Slack API (idempotent, won't overwrite existing)
7072
await slack._initialize_api(bot)
7173
return bot
74+
75+
76+
@pytest.fixture
77+
def slack_mock(bot: SirBot) -> SlackMock:
78+
"""Pre-configured Slack API mock for testing."""
79+
return SlackMock(bot)
80+
81+
82+
@pytest.fixture
83+
def airtable_mock(bot: SirBot) -> AirtableMock:
84+
"""Pre-configured Airtable API mock for testing."""
85+
return AirtableMock(bot)
86+
87+
88+
@pytest.fixture
89+
def admin_slack_mock(bot: SirBot) -> AdminSlackMock:
90+
"""Pre-configured admin Slack API mock for channel invites."""
91+
return AdminSlackMock(bot)
92+
93+
94+
@pytest.fixture
95+
def mock_user_info_with_email():
96+
"""Standard user.info response with email."""
97+
return {
98+
"ok": True,
99+
"user": {
100+
"id": "U123TEST",
101+
"name": "testuser",
102+
"real_name": "Test User",
103+
"profile": {
104+
"email": "test@example.com",
105+
"real_name": "Test User",
106+
},
107+
},
108+
}
109+
110+
111+
@pytest.fixture
112+
def mock_user_info_no_email():
113+
"""User.info response without email (restricted profile)."""
114+
return {
115+
"ok": True,
116+
"user": {
117+
"id": "U123TEST",
118+
"name": "testuser",
119+
"real_name": "Test User",
120+
"profile": {
121+
"real_name": "Test User",
122+
},
123+
},
124+
}
125+
126+
127+
@pytest.fixture
128+
def mock_mentor_record():
129+
"""Standard mentor record from Airtable."""
130+
return {
131+
"id": "recMENTOR001",
132+
"fields": {
133+
"Name": "Jane Mentor",
134+
"Email": "mentor@example.com",
135+
"Slack Name": "janementor",
136+
"Skillsets": ["Python", "JavaScript", "AWS"],
137+
},
138+
}
139+
140+
141+
@pytest.fixture
142+
def mock_service_records():
143+
"""Standard service records from Airtable."""
144+
return [
145+
{"id": "recSVC001", "fields": {"Name": "Resume Review"}},
146+
{"id": "recSVC002", "fields": {"Name": "Mock Interview"}},
147+
{"id": "recSVC003", "fields": {"Name": "Code Review"}},
148+
]

0 commit comments

Comments
 (0)