MPT-14939 E2E for audit event_types #175
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughAdds pytest fixtures for audit event types and two new end-to-end test modules (sync + async) that exercise get, filter, update, and not-found scenarios for EventType entities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-12T15:02:20.732ZApplied to files:
🧬 Code graph analysis (2)tests/e2e/audit/event_types/conftest.py (3)
tests/e2e/audit/event_types/test_async_event_types.py (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
Comment |
albertsola
force-pushed
the
e2e/MPT-14939/audit-event-types
branch
from
bf04eee to
45155b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/audit/event_types/test_async_event_types.py (1)
20-29: Avoid variable shadowing in list comprehension.The variable
event_typein the async comprehension (line 25) shadows the fixture parameterevent_type, which reduces clarity. Consider using a different name for the loop variable.🔎 Proposed fix
async def test_filter_event_types( async_event_types_service: AsyncEventTypesService, event_type: EventType ) -> None: iterator = async_event_types_service.filter(RQLQuery(id=event_type.id)).iterate() - result = [event_type async for event_type in iterator] + result = [et async for et in iterator] assert len(result) == 1 assert result[0].id == event_type.idtests/e2e/audit/event_types/conftest.py (1)
23-28: Consider using iterator pattern for efficiency.Materializing the entire list of event types is unnecessary when only the first item is needed. Using
next(iter(...), None)would be more efficient, especially if the system has many event types.🔎 Proposed fix
@pytest.fixture def event_type(event_types_service: EventTypesService) -> EventType: - event_types = list(event_types_service.iterate()) - if not event_types: + event_type = next(iter(event_types_service.iterate()), None) + if event_type is None: pytest.skip("No event types found in the system.") - return event_types[0] + return event_type
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/audit/event_types/conftest.pytests/e2e/audit/event_types/test_async_event_types.pytests/e2e/audit/event_types/test_sync_event_types.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/audit/event_types/test_sync_event_types.pytests/e2e/audit/event_types/conftest.pytests/e2e/audit/event_types/test_async_event_types.py
🧬 Code graph analysis (3)
tests/e2e/audit/event_types/test_sync_event_types.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/audit/event_types.py (2)
EventType(13-14)EventTypesService(25-32)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/audit/event_types/conftest.py (3)
event_types_service(14-15)event_type(24-28)event_type_update_data(32-35)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/audit/event_types/conftest.py (3)
mpt_api_client/mpt_client.py (2)
AsyncMPTClient(20-71)MPTClient(74-130)mpt_api_client/resources/audit/event_types.py (3)
AsyncEventTypesService(35-42)EventType(13-14)EventTypesService(25-32)tests/e2e/conftest.py (2)
mpt_vendor(19-20)async_mpt_vendor(24-27)
tests/e2e/audit/event_types/test_async_event_types.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/audit/event_types.py (2)
AsyncEventTypesService(35-42)EventType(13-14)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/audit/event_types/conftest.py (3)
async_event_types_service(19-20)event_type(24-28)event_type_update_data(32-35)mpt_api_client/models/model.py (1)
id(119-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
tests/e2e/audit/event_types/test_async_event_types.py (2)
31-39: LGTM!The update test correctly verifies that the ID remains unchanged after the update operation.
41-43: LGTM!The 404 error handling test is correctly implemented with appropriate exception matching.
tests/e2e/audit/event_types/conftest.py (3)
13-15: LGTM!The sync service fixture is correctly structured.
18-20: LGTM!The async service fixture is correctly structured.
31-35: LGTM!The update data fixture provides appropriate test data.
tests/e2e/audit/event_types/test_sync_event_types.py (4)
12-17: LGTM!The test correctly retrieves an event type and validates both the
idandkeyfields.
19-24: LGTM!The filter test correctly validates that RQL filtering returns the expected event type.
26-34: LGTM!The update test correctly verifies that the ID remains unchanged after the update operation.
36-38: LGTM!The 404 error handling test is correctly implemented with appropriate exception matching.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/audit/event_types/test_async_event_types.py (2)
20-29: Consider renaming the loop variable to avoid shadowing the fixture.On line 25, the list comprehension's loop variable
event_typeshadows the fixture parameter of the same name. While this doesn't cause runtime issues (the fixture isn't used after line 23), renaming the loop variable (e.g., toetoritem) would improve clarity.🔎 Suggested refactor
- result = [event_type async for event_type in iterator] + result = [et async for et in iterator] assert len(result) == 1 - assert result[0].id == event_type.id + assert result[0].id == event_type.id
31-39: Consider verifying that the update was applied.The test asserts the
idremains unchanged, but doesn't verify that thedescriptionfield was actually updated. If this matches the pattern used in sync tests, this is fine; otherwise, consider adding an assertion likeassert result.description == event_type_update_data["description"].
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/audit/event_types/conftest.pytests/e2e/audit/event_types/test_async_event_types.pytests/e2e/audit/event_types/test_sync_event_types.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/audit/event_types/test_sync_event_types.py
- tests/e2e/audit/event_types/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:29.261Z
Learning: In pytest with pytest-asyncio, test functions that use async fixtures but don't perform any await operations themselves should remain synchronous (def, not async def) to avoid linter complaints about unnecessary async functions. The async fixture resolution is handled automatically by pytest-asyncio.
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/audit/event_types/test_async_event_types.py
🧬 Code graph analysis (1)
tests/e2e/audit/event_types/test_async_event_types.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/audit/event_types.py (2)
AsyncEventTypesService(35-42)EventType(13-14)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/audit/event_types/conftest.py (3)
async_event_types_service(19-20)event_type(24-26)event_type_update_data(30-33)mpt_api_client/models/model.py (1)
id(119-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
tests/e2e/audit/event_types/test_async_event_types.py (1)
41-43: LGTM! The 404 error handling test is well-structured.The test correctly verifies that attempting to fetch a non-existent event type raises
MPTAPIErrorwith a 404 status. The regex pattern matches the error message format, and the use of a clearly non-existent ID (EVT-000-000) is appropriate.
albertsola
force-pushed
the
e2e/MPT-14939/audit-event-types
branch
from
45155b9 to
a1beef5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/audit/event_types/conftest.pytests/e2e/audit/event_types/test_async_event_types.pytests/e2e/audit/event_types/test_sync_event_types.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/audit/event_types/test_async_event_types.py
- tests/e2e/audit/event_types/test_sync_event_types.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/audit/event_types/conftest.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
tests/e2e/audit/event_types/conftest.py (3)
1-10: LGTM!Imports are well-organized and include all necessary types for the fixtures.
13-20: LGTM!Both service fixtures follow the standard pattern for exposing sync and async service instances from the client.
29-33: LGTM!The update data fixture provides appropriate test data for event type updates.
albertsola
force-pushed
the
e2e/MPT-14939/audit-event-types
branch
from
a1beef5 to
3044703
Compare
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.