MPT-14932 add sync and async E2E tests for notification categories #137
Conversation
WalkthroughAdds two pytest fixtures and two new end-to-end test modules (async and sync) for notification categories, covering create, get, update, list/filter, publish/unpublish, delete, and not-found scenarios with teardown cleanup and error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Found Jira issue key in the title: MPT-14932 |
jentyk
force-pushed
the
feat/MPT-14932
branch
4 times, most recently
from
51879f0 to
71a42f9
Compare
jentyk
requested review from
albertsola,
alephsur,
d3rky,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/e2e/notifications/categories/test_async_categories.py (2)
9-18: Drop obsolete# noqa: WPS421to satisfy RuffRuff reports RUF100 here because
WPS421is unknown in the current lint configuration. Since the print in tests is acceptable, you can just remove the outdatednoqadirective.- except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + except MPTAPIError as error: + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")
88-92: Reuseinvalid_category_idfixture for consistencyHere you hard-code the invalid ID instead of using the
invalid_category_idfixture defined intests/e2e/notifications/categories/conftest.py, which you already use intest_delete_category_not_found. Reusing the fixture keeps the value consistent across tests.-async def test_category_not_found(async_mpt_vendor): +async def test_category_not_found(async_mpt_vendor, invalid_category_id): @@ - with pytest.raises(MPTAPIError): - await service.get("NCT-000-000-000") + with pytest.raises(MPTAPIError): + await service.get(invalid_category_id)tests/e2e/notifications/categories/test_sync_categories.py (1)
9-17: Remove outdated# noqa: WPS421on teardown printAs in the async tests, Ruff flags
WPS421as an unknown/unusednoqacode. Since the print in test teardown is acceptable, you can drop the directive.- except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + except MPTAPIError as error: + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/notifications/categories/conftest.py (1)
tests/e2e/conftest.py (1)
short_uuid(101-102)
tests/e2e/notifications/categories/test_sync_categories.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
mpt_ops(31-32)mpt_client(43-44)tests/e2e/notifications/categories/conftest.py (1)
category_data(5-10)
tests/e2e/notifications/categories/test_async_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)async_mpt_vendor(24-27)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 GitHub Actions: PR build and merge
tests/e2e/notifications/categories/test_async_categories.py
[error] 20-20: RUF029 Function test_create_category is declared async, but doesn't await or use async features. Command failed: docker compose run --service-ports app_test
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (1)
tests/e2e/notifications/categories/conftest.py (1)
4-15: Category fixtures are clear and reusableThe
category_dataandinvalid_category_idfixtures are well-scoped and give you unique, easily identifiable test data without leaking implementation details into individual tests. No changes needed.
jentyk
force-pushed
the
feat/MPT-14932
branch
from
71a42f9 to
a21a332
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/e2e/notifications/categories/test_async_categories.py (2)
20-23: Fix incorrect await and remove async from test function.Line 21 incorrectly awaits
async_created_category.name. The fixture yields the created category object (not a coroutine), so accessing.nameis synchronous. Additionally, as flagged in past reviews, this test contains no await statements and triggers RUF029.Based on learnings, async fixtures are automatically resolved by pytest-asyncio, so the test receives the yielded value directly.
Apply this diff:
-async def test_create_category(async_created_category, category_data): - assert await async_created_category.name == category_data["name"] +def test_create_category(async_created_category, category_data): + assert async_created_category.name == category_data["name"] assert async_created_category.description == category_data["description"]
95-101: Assert not-found after delete to validate behavior.After deleting the category, calling
getmust assert thatMPTAPIErroris raised, consistent withtest_category_not_foundandtest_delete_category_not_found. As written, the test passes regardless of whethergetsucceeds or fails, leaving the delete behavior unverified.async def test_delete_category(async_mpt_ops, async_created_category): service = async_mpt_ops.notifications.categories await service.delete(async_created_category.id) - await service.get(async_created_category.id) + with pytest.raises(MPTAPIError): + await service.get(async_created_category.id)tests/e2e/notifications/categories/test_sync_categories.py (1)
92-98: Add assertion to verify deleted category raises error on get.The test calls
service.get(created_category.id)afterservice.delete()without asserting the expectedMPTAPIError. This causes a correct 404 response to fail the test with an unhandled exception, while a hypothetical successful get (indicating delete didn't work) would pass silently. This also addresses the AAA01 pipeline failure.def test_delete_category(mpt_ops, created_category): service = mpt_ops.notifications.categories service.delete(created_category.id) - service.get(created_category.id) + with pytest.raises(MPTAPIError): + service.get(created_category.id)
🧹 Nitpick comments (2)
tests/e2e/notifications/categories/test_async_categories.py (1)
17-17: Remove or update the noqa directive.The
# noqa: WPS421directive is unused because Ruff doesn't recognize wemake-python-styleguide codes. Either remove it entirely or suppress the equivalent Ruff rule if needed.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")tests/e2e/notifications/categories/test_sync_categories.py (1)
17-17: Remove or update the noqa directive.The
# noqa: WPS421directive is unused because Ruff doesn't recognize wemake-python-styleguide codes. Either remove it entirely or suppress the equivalent Ruff rule if needed.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (3)
tests/e2e/notifications/categories/conftest.py (1)
tests/e2e/conftest.py (1)
short_uuid(101-102)
tests/e2e/notifications/categories/test_sync_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
mpt_ops(31-32)mpt_client(43-44)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/categories/test_async_categories.py (2)
mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)mpt_api_client/models/model.py (1)
id(119-121)
🪛 GitHub Actions: PR build and merge
tests/e2e/notifications/categories/test_sync_categories.py
[error] 20-20: wemake-python-styleguide: AAA01 no Act block found in test
[error] 66-66: wemake-python-styleguide: AAA05 blank line in block
[error] 69-69: wemake-python-styleguide: AAA05 blank line in block
[error] 71-71: wemake-python-styleguide: AAA03 expected 1 blank line before Act block, found none
[error] 78-78: wemake-python-styleguide: AAA05 blank line in block
[error] 80-80: wemake-python-styleguide: AAA03 expected 1 blank line before Act block, found none
[error] 92-92: wemake-python-styleguide: AAA01 no Act block found in test
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (1)
tests/e2e/notifications/categories/conftest.py (1)
4-15: LGTM!The fixtures are well-structured and provide clean test data for the E2E test suite.
jentyk
force-pushed
the
feat/MPT-14932
branch
2 times, most recently
from
a3b32fc to
76c4e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tests/e2e/notifications/categories/test_async_categories.py (3)
88-93: Use the invalid_category_id fixture for consistency.The test hardcodes
"NCT-000-000-000"instead of using theinvalid_category_idfixture defined in conftest.py. Using the fixture improves maintainability and consistency withtest_delete_category_not_found.Apply this diff:
-async def test_category_not_found(async_mpt_vendor): +async def test_category_not_found(async_mpt_vendor, invalid_category_id): service = async_mpt_vendor.notifications.categories with pytest.raises(MPTAPIError): - await service.get("NCT-000-000-000") + await service.get(invalid_category_id)
20-22: Removeasynckeyword and clean up unnecessary noqa directive.The function is declared
asyncbut contains noawaitstatements, triggering a pipeline error. Additionally, the# noqa: RUF029directive is unnecessary since RUF029 is not enabled in the current configuration.Based on learnings, async fixtures are automatically resolved by pytest-asyncio, so this test can be synchronous.
-async def test_create_category(async_created_category, category_data): # noqa: RUF029 +def test_create_category(async_created_category, category_data): assert async_created_category.name == category_data["name"] assert async_created_category.description == category_data["description"]
95-101: Assert not-found after delete to validate behavior.After deleting the category, calling
getmust assert thatMPTAPIErroris raised, consistent withtest_category_not_foundandtest_delete_category_not_found. As written, the test passes regardless of whethergetsucceeds or fails.async def test_delete_category(async_mpt_ops, async_created_category): service = async_mpt_ops.notifications.categories await service.delete(async_created_category.id) - await service.get(async_created_category.id) + with pytest.raises(MPTAPIError): + await service.get(async_created_category.id)tests/e2e/notifications/categories/test_sync_categories.py (3)
64-74: Fix AAA pattern violations flagged by pipeline.The test has incorrect blank line placement for the Arrange-Act-Assert pattern. Remove blank lines within the Arrange block and add a blank line before the Act block.
Apply this diff:
def test_publish_category(mpt_ops, created_category): service = mpt_ops.notifications.categories - unpublish_note_data = {"note": "Unpublishing category for testing"} service.unpublish(created_category.id, unpublish_note_data) - + note_data = {"note": "Publishing category for testing"} result = service.publish(created_category.id, note_data) assert result.id == created_category.id
76-83: Fix AAA pattern violations flagged by pipeline.The test has incorrect blank line placement. Remove the blank line within the Arrange block and add one before the Act block.
Apply this diff:
def test_unpublish_category(mpt_ops, created_category): service = mpt_ops.notifications.categories - + unpublish_note_data = {"note": "Unpublishing category for testing"} result = service.unpublish(created_category.id, unpublish_note_data) assert result.id == created_category.id
92-97: Add assertion to verify deleted category raises error on get.The test calls
service.get(created_category.id)afterservice.delete()without asserting the expectedMPTAPIError. This causes a correct 404 response to fail the test with an unhandled exception.def test_delete_category(mpt_ops, created_category): service = mpt_ops.notifications.categories service.delete(created_category.id) - service.get(created_category.id) + with pytest.raises(MPTAPIError): + service.get(created_category.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/notifications/categories/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.691Z
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-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (2)
tests/e2e/notifications/categories/test_sync_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
mpt_ops(31-32)mpt_client(43-44)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/categories/test_async_categories.py (3)
mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 GitHub Actions: PR build and merge
tests/e2e/notifications/categories/test_sync_categories.py
[error] 20-20: wemake-python-styleguide violation: AAA01 no Act block found in test.
[error] 66-66: wemake-python-styleguide violation: AAA05 blank line in block.
[error] 69-69: wemake-python-styleguide violation: AAA05 blank line in block.
[error] 71-71: wemake-python-styleguide violation: AAA03 expected 1 blank line before Act block.
[error] 78-78: wemake-python-styleguide violation: AAA05 blank line in block.
[error] 80-80: wemake-python-styleguide violation: AAA03 expected 1 blank line before Act block.
[error] 92-92: wemake-python-styleguide violation: AAA01 no Act block found in test.
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
20-20: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
jentyk
force-pushed
the
feat/MPT-14932
branch
3 times, most recently
from
bd34c36 to
4abca55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/e2e/notifications/categories/test_async_categories.py (2)
20-23: Removeasyncfrom test function.The test is declared
asyncbut contains noawaitstatements. Based on learnings, async fixtures are automatically resolved by pytest-asyncio, so this test can be synchronous. The# noqa: RUF029directive is ineffective (Ruff reports it as unused because RUF029 is not enabled).Apply this diff:
-async def test_create_category(async_created_category, category_data): # noqa: RUF029 +def test_create_category(async_created_category, category_data): assert async_created_category.name == category_data["name"] assert async_created_category.description == category_data["description"]Based on learnings, async fixtures are automatically resolved before being passed to test functions.
95-101: Critical: Missing assertion after delete.The test calls
await service.get(async_created_category.id)on Line 100 after deletion without asserting that it raisesMPTAPIError. This causes the test to fail with an unhandled exception when delete works correctly (404 response), while paradoxically passing if delete fails (category still retrievable).Apply this diff:
async def test_delete_category(async_mpt_ops, async_created_category): service = async_mpt_ops.notifications.categories await service.delete(async_created_category.id) - await service.get(async_created_category.id) + with pytest.raises(MPTAPIError): + await service.get(async_created_category.id)tests/e2e/notifications/categories/test_sync_categories.py (1)
92-98: Critical: Missing assertion after delete.The test calls
service.get(created_category.id)on Line 97 after deletion without asserting that it raisesMPTAPIError. This causes the test to fail with an unhandled exception when delete works correctly (404 response), while paradoxically passing if delete fails (category still retrievable).Apply this diff:
def test_delete_category(mpt_ops, created_category): service = mpt_ops.notifications.categories service.delete(created_category.id) # act - service.get(created_category.id) + with pytest.raises(MPTAPIError): + service.get(created_category.id)
🧹 Nitpick comments (2)
tests/e2e/notifications/categories/test_sync_categories.py (1)
17-17: Remove unusednoqadirective.Ruff reports the
# noqa: WPS421directive as unused/unknown. If wemake-python-styleguide is not active in your linting pipeline, remove this directive.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")tests/e2e/notifications/categories/test_async_categories.py (1)
17-17: Remove unusednoqadirective.Ruff reports the
# noqa: WPS421directive as unused/unknown. If wemake-python-styleguide is not active in your linting pipeline, remove this directive.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/notifications/categories/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.691Z
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-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (2)
tests/e2e/notifications/categories/test_sync_categories.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)
tests/e2e/notifications/categories/test_async_categories.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)async_mpt_vendor(24-27)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
20-20: Unused noqa directive (non-enabled: RUF029)
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/notifications/categories/test_async_categories.py (1)
95-101: Assert not-found after delete to validate behavior.After deleting the category, calling
getmust assert thatMPTAPIErroris raised, consistent withtest_category_not_foundandtest_delete_category_not_found. As written, the test passes regardless of whethergetsucceeds or fails, leaving the delete behavior unverified.Apply this diff:
async def test_delete_category(async_mpt_ops, async_created_category): service = async_mpt_ops.notifications.categories await service.delete(async_created_category.id) - await service.get(async_created_category.id) + with pytest.raises(MPTAPIError): + await service.get(async_created_category.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/notifications/categories/test_async_categories.py(1 hunks)
🧰 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.691Z
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-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🪛 GitHub Actions: PR build and merge
tests/e2e/notifications/categories/test_async_categories.py
[error] 21-21: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
[error] 22-22: B015 Pointless comparison at end of function scope. Did you mean to return the expression result?
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
21-21: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
22-22: Pointless comparison at end of function scope. Did you mean to return the expression result?
(B015)
🔇 Additional comments (10)
tests/e2e/notifications/categories/test_async_categories.py (10)
1-7: LGTM!The imports and module-level flaky marker are appropriate for async E2E testing.
25-32: LGTM!The test correctly verifies category retrieval and validates the returned data.
34-45: LGTM!The update test correctly validates that category fields are updated as expected.
47-53: LGTM!The test correctly uses async iteration and validates the created category appears in the listing.
55-65: LGTM!The filter test correctly demonstrates RQL query usage and validates the filtered results.
67-77: LGTM!The test correctly validates the publish workflow by first unpublishing and then publishing the category.
79-86: LGTM!The test correctly validates the unpublish operation.
88-93: LGTM!The test correctly validates not-found error handling using the shared fixture.
103-107: LGTM!The test correctly validates error handling when attempting to delete a non-existent category.
9-18: Thenoqa: WPS421directive is correct and should be retained.This project uses wemake-python-styleguide, so the directive suppresses a legitimate linter rule for print statements in teardown code. No changes needed.
jentyk
force-pushed
the
feat/MPT-14932
branch
from
3fe179c to
7f348a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/e2e/notifications/categories/test_sync_categories.py (1)
17-17: Consider removing unusednoqadirective if WPS linter is not in use.Ruff flags the
WPS421noqa as unused since it doesn't recognize wemake-python-styleguide rules. If your project no longer uses the WPS linter, you can safely remove this directive.Apply this diff if WPS linter is not configured:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")tests/e2e/notifications/categories/test_async_categories.py (1)
17-17: Consider removing unusednoqadirectives if WPS/AAA linters are not in use.Ruff flags
WPS421andAAA01noqa directives as unused since it doesn't recognize wemake-python-styleguide or AAA rules. If your project no longer uses those linters, you can safely remove these directives.Apply this diff if WPS/AAA linters are not configured:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")-def test_create_category(async_created_category, category_data): # noqa: AAA01 +def test_create_category(async_created_category, category_data):Also applies to: 20-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/notifications/categories/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (2)
tests/e2e/notifications/categories/test_async_categories.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)async_mpt_vendor(24-27)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/categories/test_sync_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
mpt_ops(31-32)mpt_client(43-44)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
20-20: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
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
jentyk
force-pushed
the
feat/MPT-14932
branch
from
7f348a6 to
a55338c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/notifications/categories/test_sync_categories.py (1)
9-18: Remove unusednoqadirective on Line 17.The
noqa: WPS421directive is flagged as unknown by Ruff. The WPS prefix indicates it's from wemake-python-styleguide, which may not be active in your current linting setup.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")tests/e2e/notifications/categories/test_async_categories.py (2)
9-18: Remove unusednoqadirective on Line 17.The
noqa: WPS421directive is flagged as unknown by Ruff. The WPS prefix indicates it's from wemake-python-styleguide, which may not be active in your current linting setup.Apply this diff:
- print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete category {category.id}: {error.title}")
20-23: Remove unusednoqadirective on Line 20.The
noqa: AAA01directive is flagged as unused by Ruff. The test now passes AAA validation, so the suppression is no longer needed.Apply this diff:
-def test_create_category(async_created_category, category_data): # noqa: AAA01 +def test_create_category(async_created_category, category_data): assert async_created_category.name == category_data["name"] assert async_created_category.description == category_data["description"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🧰 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.691Z
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-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (3)
tests/e2e/notifications/categories/conftest.py (1)
tests/e2e/conftest.py (1)
short_uuid(101-102)
tests/e2e/notifications/categories/test_async_categories.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)async_mpt_vendor(24-27)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/notifications/categories/test_sync_categories.py (1)
test_create_category(20-23)
tests/e2e/notifications/categories/test_sync_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
mpt_ops(31-32)mpt_client(43-44)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.6)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
20-20: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
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 (21)
tests/e2e/notifications/categories/conftest.py (2)
4-10: LGTM!The fixture correctly generates unique category test data using the
short_uuidfixture dependency.
13-15: LGTM!The fixture provides a consistent invalid category ID for not-found test scenarios.
tests/e2e/notifications/categories/test_sync_categories.py (10)
1-7: LGTM!Imports and test markers are appropriate for e2e testing.
20-24: LGTM!The test correctly verifies that the created category matches the input data. The inline
# actcomment satisfies the AAA linter for fixture-based tests.
26-33: LGTM!The test follows proper AAA pattern and correctly verifies category retrieval.
35-46: LGTM!The test follows proper AAA pattern and correctly verifies category updates.
48-54: LGTM!The test follows proper AAA pattern and correctly verifies that the created category appears in the list.
56-63: LGTM!The test follows proper AAA pattern and correctly verifies filtering by ID using RQLQuery.
65-74: LGTM!The test follows proper AAA pattern with the unpublish setup in the Arrange block and correctly verifies the publish operation.
76-83: LGTM!The test follows proper AAA pattern and correctly verifies the unpublish operation.
85-90: LGTM!The test correctly verifies that attempting to get a non-existent category raises
MPTAPIError.
92-105: LGTM!Both deletion tests are correct:
test_delete_categoryproperly verifies that getting a deleted category raisesMPTAPIErrortest_delete_category_not_foundcorrectly verifies that deleting a non-existent category raisesMPTAPIErrortests/e2e/notifications/categories/test_async_categories.py (9)
1-7: LGTM!Imports and test markers are consistent with the synchronous test module.
25-32: LGTM!The test follows proper AAA pattern and correctly uses async/await for category retrieval.
34-45: LGTM!The test follows proper AAA pattern and correctly uses async/await for category updates.
47-53: LGTM!The test follows proper AAA pattern and correctly uses async iteration to verify the created category appears in the list.
55-65: LGTM!The test follows proper AAA pattern and correctly uses async iteration with RQLQuery filtering.
67-76: LGTM!The test follows proper AAA pattern with all setup in a contiguous Arrange block (lines 68-71), followed by a blank line, then the Act block, and finally the Assert block.
78-85: LGTM!The test follows proper AAA pattern with the Arrange block (lines 79-80), a separating blank line, the Act block, and the Assert block.
87-92: LGTM!The test correctly uses the
invalid_category_idfixture and verifies that attempting to get a non-existent category raisesMPTAPIError.
94-107: LGTM!Both deletion tests are correct:
test_delete_categoryproperly verifies that getting a deleted category raisesMPTAPIErrortest_delete_category_not_foundcorrectly verifies that deleting a non-existent category raisesMPTAPIErrorBoth use proper async/await syntax.
jentyk
force-pushed
the
feat/MPT-14932
branch
from
a2b4f6b to
ab9fd46
Compare
albertsola
left a comment
albertsola
left a comment
There was a problem hiding this comment.
Looks like most tests are skipped.
Approved, but we should remove the skip once the bug is fixed.
jentyk
force-pushed
the
feat/MPT-14932
branch
from
ab9fd46 to
6769a91
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/notifications/categories/conftest.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(1 hunks)tests/e2e/notifications/categories/test_sync_categories.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/notifications/categories/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.691Z
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-11-27T00:05:54.691Z
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.691Z
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/categories/test_async_categories.py
🧬 Code graph analysis (2)
tests/e2e/notifications/categories/test_sync_categories.py (2)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)
tests/e2e/notifications/categories/test_async_categories.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (2)
RQLQuery(115-524)any(260-270)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)async_mpt_vendor(24-27)tests/e2e/notifications/categories/conftest.py (2)
category_data(5-10)invalid_category_id(14-15)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.7)
tests/e2e/notifications/categories/test_sync_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
17-17: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
20-20: 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
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.