feat: add system events for conferences by dmitriichmil · Pull Request #493 · ExpressApp/pybotx

dmitriichmil · 2025-04-29T12:36:38Z

PR checklist

I've written good commit message for all commits
I've split changes into separate commits where it's appropriate
I've added the description of function to documentation
I've updated project version in pyproject.toml
I'll make a release when PR is merged
I'll bump pybotx in bot-template

codecov · 2025-04-29T12:44:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e2a3e66) to head (28b3142).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #493    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          122       125     +3     
  Lines         4281      4412   +131     
  Branches       367       370     +3     
==========================================
+ Hits          4281      4412   +131

Flag	Coverage Δ
unittests	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

osovskiyalexandr · 2025-04-29T21:23:47Z

tests/system_events/test_conference_changed.py

Для payload стоит использовать фабрику или фикстуру

добавил фабрику

osovskiyalexandr · 2025-04-29T21:25:15Z

tests/system_events/test_conference_changed.py

+        nonlocal conference_changed
+        conference_changed = event
+        # Drop `raw_command` from asserting
+        conference_changed.raw_command = None


Мутировать объект, который пришел от системы - плохая практика. Лучше работать с ним как с неизменяемым. Вместо того чтобы менять объект, используй dict из pydantic и exclude внутри него, это позволит создать сериализованное представление без лишних полей.

История дублируется, стоит вынести в утилиту. Что-то в духе:

def assert_events_equal_ignoring_fields(actual, expected, ignore_fields): assert actual.dict(exclude=set(ignore_fields)) == expected.dict(exclude=set(ignore_fields))

там dataclass, из-за этого немного по-другому сделал

osovskiyalexandr · 2025-04-29T21:26:10Z

tests/system_events/test_conference_created.py

+        nonlocal conference_created
+        conference_created = event
+        # Drop `raw_command` from asserting
+        conference_created.raw_command = None


Аналогично

osovskiyalexandr · 2025-04-29T21:26:55Z

tests/system_events/test_conference_created.py

+    datetime_formatter: Callable[[str], datetime],
+) -> None:
+    # - Arrange -
+    payload = {


Для payload стоит использовать фабрику или фикстуру

osovskiyalexandr · 2025-04-29T21:27:05Z

tests/system_events/test_conference_deleted.py

+    bot_account: BotAccountWithSecret,
+) -> None:
+    # - Arrange -
+    payload = {


Для payload стоит использовать фабрику или фикстуру

osovskiyalexandr · 2025-04-29T21:27:15Z

tests/system_events/test_conference_deleted.py

+        nonlocal conference_deleted
+        conference_deleted = event
+        # Drop `raw_command` from asserting
+        conference_deleted.raw_command = None


Аналогично

vladimirgubarik · 2025-04-30T07:52:03Z

pybotx/models/system_events/conference_changed.py

+    """Event `system:conference_changed`.
+
+    Attributes:
+        call_id: id conference.


Стоит описать все параметры в доксринге, а не только часть

osovskiyalexandr · 2025-05-04T10:07:36Z

tests/system_events/test_conference_changed.py

+        bot.async_execute_raw_bot_command(payload, verify_request=False)
+
+    # - Assert -
+    assert conference_changed == ConferenceChangedEvent(


В целом ОК, но я бы глянул в сторону factory-boy.
Плюс для сравнения deepdiff

vladimirgubarik · 2025-05-07T08:21:13Z

tests/system_events/test_conference_changed.py

+    payload = api_incoming_message_factory(
+        body="system:conference_changed",
+        command_type="system",
+        data=json.loads(conference_change_data.json()),


как будто бы не очень хорошо тестовый пайлоад, который ждешь в запросе создавать из внутренней модели
Я имею ввиду вот здесь:

payload = api_incoming_message_factory( body="system:conference_changed", command_type="system", data=json.loads(conference_change_data.json()), bot_id=bot_id, host=host, )

ты по сути используешь

class ConferenceChangedDataFactory(factory.Factory[BotAPIConferenceChangedData]): class Meta: model = BotAPIConferenceChangedData

BotAPIConferenceChangedData - внутренняя модель, не факт вообще что она правильно написана и соответствует приходящим данным.
И если ты ее поменяешь, то данные в тесте тоже могут поменяться, тест будет проходить, но по факту ломаться на реальных данных

поменял на DictFactory

osovskiyalexandr · 2025-05-07T08:36:44Z

tests/system_events/test_conference_changed.py

+    payload = api_incoming_message_factory(
+        body="system:conference_changed",
+        command_type="system",
+        data=json.loads(conference_change_data.json()),


conference_change_data.dict() - должен дать тот же результат, но быстрее

osovskiyalexandr · 2025-05-07T08:38:00Z

tests/system_events/test_conference_changed.py

+        ),
+    )
+
+    assert bool(diff) is False


assert diff == {}, diff

Так diff напечатает, что именно не совпало. А assert bool(diff) is False даст непонятный бинарный результат.

поправил

osovskiyalexandr · 2025-05-07T08:38:35Z

tests/system_events/test_conference_changed.py

+]
+
+
+async def test__conference_changed_succeed(


А зачем двойное подчеркивание после test?

общий стиль названия тестов такой

osovskiyalexandr · 2025-05-07T08:43:10Z

tests/conftest.py

+
+
+@pytest.fixture
+def conference_change_data() -> BotAPIConferenceChangedData:


Если говорить о дистанции, то возвращать фабрику, а не объект из фикстуры будет лучшим вариантом. Что даст тебе возможность в специфичных тестах менять только нужные поля, без переписывания всего объекта.

dmitriichmil self-assigned this Apr 29, 2025

osovskiyalexandr self-requested a review April 29, 2025 12:39

dmitriichmil force-pushed the feature/cd/system-events-conferences branch from 64fa430 to 3d12faa Compare April 29, 2025 12:43

osovskiyalexandr requested a review from vladimirgubarik April 29, 2025 21:16

osovskiyalexandr requested changes Apr 29, 2025

View reviewed changes

vladimirgubarik reviewed Apr 30, 2025

View reviewed changes

dmitriichmil force-pushed the feature/cd/system-events-conferences branch from 3d12faa to 6b944fa Compare April 30, 2025 08:47

osovskiyalexandr reviewed May 4, 2025

View reviewed changes

dmitriichmil force-pushed the feature/cd/system-events-conferences branch 3 times, most recently from 9a7e77e to cd3907c Compare May 6, 2025 15:30

vladimirgubarik reviewed May 7, 2025

View reviewed changes

osovskiyalexandr requested changes May 7, 2025

View reviewed changes

dmitriichmil force-pushed the feature/cd/system-events-conferences branch from cd3907c to 662f3bb Compare May 7, 2025 10:31

osovskiyalexandr approved these changes May 7, 2025

View reviewed changes

feat: add system events for conferences

28b3142

dmitriichmil force-pushed the feature/cd/system-events-conferences branch from 662f3bb to 28b3142 Compare May 7, 2025 11:58

dmitriichmil merged commit ea3498d into master May 7, 2025
9 checks passed

dmitriichmil deleted the feature/cd/system-events-conferences branch May 7, 2025 12:01



		@pytest.fixture
		def conference_change_data() -> BotAPIConferenceChangedData:

Conversation

dmitriichmil commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

codecov bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osovskiyalexandr Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osovskiyalexandr May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmitriichmil commented Apr 29, 2025 •

edited

Loading

codecov bot commented Apr 29, 2025 •

edited

Loading

osovskiyalexandr Apr 29, 2025 •

edited

Loading

osovskiyalexandr May 4, 2025 •

edited

Loading