MPT-14933 E2E for notifications/contacts #168
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds end-to-end test fixtures and synchronous/asynchronous test suites for the notifications.contacts API covering contact fixture creation, CRUD operations, block/unblock transitions, listing/filtering, and error cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/notifications/contacts/test_async_contacts.py (2)
9-10: Useasync_created_contactfixture for consistency in async test module.The async test file should use the
async_created_contactfixture instead of the synchronouscreated_contactfixture to maintain consistency and properly test the async workflow.Apply this diff:
-def test_create_contact(created_contact): # noqa: AAA01 +def test_create_contact(async_created_contact): # noqa: AAA01 - assert created_contact is not None + assert async_created_contact is not NoneNote: The
noqa: AAA01directive is not recognized by Ruff (it's for flake8-aaa plugin). You may remove it if you're not using flake8-aaa.
13-63: Maintain fixture consistency across async tests.Multiple async tests use the synchronous
created_contactfixture or derivatives (created_contact_id). For better consistency and to properly exercise the async workflow, consider:
- Using
async_created_contactin tests at lines 37, 47, and 60- Creating an
async_created_contact_idfixture that derives fromasync_created_contactfor use in lines 13 and 19Additionally, line 34 uses a hardcoded invalid contact ID
"CON-0000-0000-0000"instead of theinvalid_contact_idfixture that exists inconftest.py.Example fixture to add in
conftest.py:@pytest.fixture def async_created_contact_id(async_created_contact): return async_created_contact.idAnd update line 34:
async def test_get_contact_not_found(async_mpt_ops): service = async_mpt_ops.notifications.contacts with pytest.raises(MPTAPIError): - await service.get("CON-0000-0000-0000") + await service.get(invalid_contact_id)Don't forget to add
invalid_contact_idas a parameter totest_get_contact_not_found.tests/e2e/notifications/contacts/test_sync_contacts.py (1)
9-9: Optional: Remove unrecognizednoqadirectives.The
noqa: AAA01directives at lines 9 and 37 are for the flake8-aaa plugin (Arrange-Act-Assert pattern), which Ruff does not recognize. If you're not using flake8-aaa in your linting pipeline, these can be safely removed.-def test_created_contact(created_contact): # noqa: AAA01 +def test_created_contact(created_contact): assert created_contact is not None -def test_block_unblock_contact(mpt_ops, created_contact): # noqa: AAA01 +def test_block_unblock_contact(mpt_ops, created_contact): service = mpt_ops.notifications.contactsAlso applies to: 37-37
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/contacts/conftest.py(1 hunks)tests/e2e/notifications/contacts/test_async_contacts.py(1 hunks)tests/e2e/notifications/contacts/test_sync_contacts.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/notifications/contacts/test_sync_contacts.pytests/e2e/notifications/contacts/test_async_contacts.pytests/e2e/notifications/contacts/conftest.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/contacts/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/contacts/test_sync_contacts.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/contacts/conftest.py (3)
created_contact(18-21)created_contact_id(32-33)invalid_contact_id(37-38)tests/e2e/conftest.py (2)
mpt_ops(31-32)short_uuid(101-102)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/contacts/conftest.py (2)
tests/e2e/conftest.py (3)
short_uuid(101-102)mpt_ops(31-32)async_mpt_ops(36-39)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/notifications/contacts/test_sync_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/contacts/test_async_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
⏰ 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 (2)
tests/e2e/notifications/contacts/conftest.py (1)
1-38: Fixtures are well-structured.The fixture definitions are correct and follow established patterns. The
async_created_contactfixture at lines 25-28 is properly defined but currently unused by the async tests (see comments ontest_async_contacts.py).tests/e2e/notifications/contacts/test_sync_contacts.py (1)
9-64: Well-structured synchronous test suite.The test suite provides comprehensive coverage of CRUD operations, state transitions (block/unblock), error handling, and uses fixtures consistently. The test structure is clear and follows best practices.
albertsola
force-pushed
the
e2e/MPT-14933/Add-E2E-tests-for-notifications-contact-endpoints
branch
from
c752159 to
018aa83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/e2e/notifications/contacts/test_sync_contacts.py (3)
9-10: Remove unusednoqadirective.The
# noqa: AAA01comment is flagged as unused by Ruff. The AAA01 rule (Act-Arrange-Assert) is not configured in the linter.🔎 Suggested fix:
-def test_created_contact(created_contact): # noqa: AAA01 +def test_created_contact(created_contact): assert created_contact is not None
37-44: Remove unusednoqadirective.Same as line 9, the
# noqa: AAA01directive is unused.🔎 Suggested fix:
-def test_block_unblock_contact(mpt_ops, created_contact): # noqa: AAA01 +def test_block_unblock_contact(mpt_ops, created_contact): service = mpt_ops.notifications.contacts
61-64: Consider adding verification that the contact was actually deleted.The test calls
delete()but doesn't verify the resource is gone. A follow-upget()expectingMPTAPIErrorwould confirm the deletion succeeded.🔎 Suggested enhancement:
def test_delete_contact(mpt_ops, created_contact_id): service = mpt_ops.notifications.contacts service.delete(created_contact_id) # act + + with pytest.raises(MPTAPIError): + service.get(created_contact_id)tests/e2e/notifications/contacts/test_async_contacts.py (3)
9-10: Remove unusednoqadirective.The
# noqa: AAA01directive is flagged as unused by Ruff.🔎 Suggested fix:
-def test_create_contact(created_contact): # noqa: AAA01 +def test_create_contact(created_contact): assert created_contact is not None
30-34: Useinvalid_contact_idfixture for consistency.The sync version
test_get_contact_not_founduses theinvalid_contact_idfixture, but this async version hardcodes the ID. Using the fixture improves maintainability and consistency.🔎 Suggested fix:
-async def test_get_contact_not_found(async_mpt_ops): +async def test_get_contact_not_found(async_mpt_ops, invalid_contact_id): service = async_mpt_ops.notifications.contacts with pytest.raises(MPTAPIError): - await service.get("CON-0000-0000-0000") + await service.get(invalid_contact_id)
60-63: Consider adding verification that the contact was actually deleted.Same as the sync version - the test calls
delete()but doesn't verify the resource is gone.🔎 Suggested enhancement:
async def test_delete_contact(async_mpt_ops, created_contact): service = async_mpt_ops.notifications.contacts await service.delete(created_contact.id) + + with pytest.raises(MPTAPIError): + await service.get(created_contact.id)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/contacts/conftest.py(1 hunks)tests/e2e/notifications/contacts/test_async_contacts.py(1 hunks)tests/e2e/notifications/contacts/test_sync_contacts.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/notifications/contacts/conftest.pytests/e2e/notifications/contacts/test_async_contacts.pytests/e2e/notifications/contacts/test_sync_contacts.py
🧬 Code graph analysis (3)
tests/e2e/notifications/contacts/conftest.py (2)
tests/e2e/conftest.py (3)
short_uuid(101-102)mpt_ops(31-32)async_mpt_ops(36-39)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/contacts/test_async_contacts.py (5)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/contacts/conftest.py (2)
created_contact(18-21)created_contact_id(32-33)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)short_uuid(101-102)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/contacts/test_sync_contacts.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/contacts/conftest.py (3)
created_contact(18-21)created_contact_id(32-33)invalid_contact_id(37-38)tests/e2e/conftest.py (2)
mpt_ops(31-32)short_uuid(101-102)
🪛 Ruff (0.14.8)
tests/e2e/notifications/contacts/test_async_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/contacts/test_sync_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
⏰ 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 (11)
tests/e2e/notifications/contacts/conftest.py (3)
6-14: LGTM!The
contact_datafixture is well-structured with a unique email usingshort_uuidto prevent conflicts across test runs.
17-21: LGTM!The
created_contactfixture correctly uses the context manager pattern for resource lifecycle management.
31-38: LGTM!The
created_contact_idandinvalid_contact_idfixtures are straightforward and correctly implemented.tests/e2e/notifications/contacts/test_sync_contacts.py (4)
13-18: LGTM!The test correctly retrieves a contact by ID and verifies the returned resource.
21-27: LGTM!Good coverage of the list/filter functionality using
RQLQueryand iteration.
30-34: LGTM!Proper 404 error handling verification using
pytest.raises.
47-58: LGTM!The update test correctly verifies that the name field is persisted.
tests/e2e/notifications/contacts/test_async_contacts.py (4)
13-16: LGTM!Properly uses
awaitfor async contact retrieval.
19-27: LGTM!Good use of async iteration with
async forcomprehension for the list operation.
37-44: LGTM!The block/unblock workflow is correctly tested with proper status assertions.
47-57: LGTM!The update test correctly verifies that the name change is persisted.
albertsola
force-pushed
the
e2e/MPT-14933/Add-E2E-tests-for-notifications-contact-endpoints
branch
from
018aa83 to
44c4f5e
Compare
albertsola
force-pushed
the
e2e/MPT-14933/Add-E2E-tests-for-notifications-contact-endpoints
branch
from
44c4f5e to
972f019
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/notifications/contacts/conftest.py (1)
27-31: Theasync_created_contactfixture is still unused.Although the previous issue with incorrect async implementation has been resolved, this fixture remains unused throughout the test suite. Both
test_sync_contacts.pyandtest_async_contacts.pyuse the synchronouscreated_contactfixture, which works correctly with async tests per pytest-asyncio behavior. Consider removing this fixture to reduce maintenance burden unless it's intended for future use.🔎 Verify fixture usage across the test suite
#!/bin/bash # Description: Confirm that async_created_contact is not referenced in any test files # Search for usage of async_created_contact in test files rg -n "async_created_contact" tests/e2e/notifications/contacts/ --type=py
🧹 Nitpick comments (4)
tests/e2e/notifications/contacts/test_sync_contacts.py (2)
9-9: Remove unusednoqadirective.Ruff reports that
AAA01is an unknown linting code. Thisnoqacomment appears to be for a pytest plugin that may not be active in your linting configuration.🔎 Apply this diff to remove the unused directive:
-def test_created_contact(created_contact): # noqa: AAA01 +def test_created_contact(created_contact): assert created_contact is not None
37-37: Remove unusednoqadirective.Similar to line 9, this
AAA01code is not recognized by Ruff.🔎 Apply this diff to remove the unused directive:
-def test_block_unblock_contact(mpt_ops, created_contact): # noqa: AAA01 +def test_block_unblock_contact(mpt_ops, created_contact): service = mpt_ops.notifications.contactstests/e2e/notifications/contacts/test_async_contacts.py (2)
9-9: Remove unusednoqadirective.Ruff reports that
AAA01is an unknown linting code. This directive appears unnecessary.🔎 Apply this diff to remove the unused directive:
-def test_create_contact(created_contact): # noqa: AAA01 +def test_create_contact(created_contact): assert created_contact is not None
30-34: Use theinvalid_contact_idfixture for consistency.Line 34 hardcodes the invalid contact ID
"CON-0000-0000-0000", while the synchronous equivalent test intest_sync_contacts.py(line 30) uses theinvalid_contact_idfixture. Using the fixture improves maintainability and ensures consistency across test files.🔎 Apply this diff to use the fixture:
-async def test_get_contact_not_found(async_mpt_ops): +async def test_get_contact_not_found(async_mpt_ops, invalid_contact_id): service = async_mpt_ops.notifications.contacts with pytest.raises(MPTAPIError): - await service.get("CON-0000-0000-0000") + await service.get(invalid_contact_id)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/contacts/conftest.py(1 hunks)tests/e2e/notifications/contacts/test_async_contacts.py(1 hunks)tests/e2e/notifications/contacts/test_sync_contacts.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/notifications/contacts/test_async_contacts.pytests/e2e/notifications/contacts/test_sync_contacts.pytests/e2e/notifications/contacts/conftest.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/contacts/conftest.py
📚 Learning: 2025-11-27T00:05:54.701Z
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.
Applied to files:
tests/e2e/notifications/contacts/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/contacts/test_async_contacts.py (6)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/contacts/conftest.py (2)
created_contact(21-24)created_contact_id(35-36)tests/e2e/notifications/contacts/test_sync_contacts.py (6)
test_get_contact(13-18)test_list_contacts(21-27)test_get_contact_not_found(30-34)test_block_unblock_contact(37-44)test_update_contact(47-58)test_delete_contact(61-64)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)short_uuid(101-102)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/contacts/conftest.py (2)
tests/e2e/conftest.py (3)
short_uuid(101-102)mpt_ops(31-32)async_mpt_ops(36-39)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/notifications/contacts/test_async_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/contacts/test_sync_contacts.py
9-9: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
⏰ 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/notifications/contacts/conftest.py (1)
9-17: LGTM! Fixtures are well-structured.The
contact_data,created_contact,created_contact_id, andinvalid_contact_idfixtures are correctly implemented and properly used throughout the test suite.Also applies to: 20-24, 34-36, 39-41
tests/e2e/notifications/contacts/test_sync_contacts.py (1)
13-18: LGTM! Test coverage is comprehensive.The test suite thoroughly covers the contact API lifecycle: retrieval, listing with RQL filtering, error handling, state transitions (block/unblock), updates, and deletion. All assertions are appropriate and the test logic is sound.
Also applies to: 21-27, 30-34, 37-44, 47-58, 61-64
tests/e2e/notifications/contacts/test_async_contacts.py (1)
13-16: LGTM! Async test implementations are correct.All async operations are properly awaited, async iteration is used correctly (line 24), and the test logic mirrors the synchronous test suite appropriately. The use of the synchronous
created_contactfixture in async tests is valid per pytest-asyncio behavior.Also applies to: 19-27, 37-44, 47-57, 60-63
|
d3rky
deleted the
e2e/MPT-14933/Add-E2E-tests-for-notifications-contact-endpoints
branch
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.