[MPT-14870] Added Accounts users e2e tests #140
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughReplaces update mixins with file-upload-aware variants in the Users API client, adds upload key config, reorganizes and adds account/user fixtures in the parent conftest, removes duplicate local fixtures, and introduces new synchronous and asynchronous end-to-end user tests (get, filter, update-with-file, block/unblock, 404 cases). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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 (4)
tests/e2e/accounts/users/conftest.py (1)
6-6: Consider removing unused noqa directive.The
noqa: WPS430directive suppresses a wemake-python-styleguide warning. If the project doesn't use that linter (Ruff doesn't recognize it), this directive can be removed for cleaner code.Apply this diff to remove the unused directive:
- def _user( # noqa: WPS430 + def _user(tests/e2e/accounts/conftest.py (2)
129-136: Enhance teardown error reporting for consistency.The sync fixture's teardown doesn't include error details, while the async version (lines 164-168) does include the error title. For consistent debugging experience, consider adding error details to the sync version as well.
Apply this diff to match the async version's error reporting:
- except MPTAPIError: + except MPTAPIError as error: print( # noqa: WPS421 - f"TEARDOWN - Unable to delete account user {ret_account_user.id}", + f"TEARDOWN - Unable to delete account user {ret_account_user.id}: " + f"{getattr(error, 'title', str(error))}" )
80-80: Consider removing unused noqa directives.The
noqadirectives for WPS codes (WPS430, WPS420, WPS421) suppress wemake-python-styleguide warnings. If the project doesn't use that linter (Ruff doesn't recognize these codes), these directives can be removed for cleaner code.Also applies to: 118-118, 134-134, 149-149, 165-165
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
146-146: Consider removing unused noqa directive.The
noqa: AAA01directive suppresses a pytest-arrange-act-assert plugin warning. If the project doesn't use that plugin (Ruff doesn't recognize it), this directive can be removed.Apply this diff:
-def test_account_user_group_post(created_account_user_group): # noqa: AAA01 +def test_account_user_group_post(created_account_user_group):Note: The sync test function correctly uses the async fixture
created_account_user_group- pytest-asyncio automatically resolves async fixtures for both sync and async test functions. Based on learnings, this pattern is valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpt_api_client/resources/accounts/users.py(3 hunks)tests/e2e/accounts/accounts_users/conftest.py(0 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(2 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(0 hunks)tests/e2e/accounts/conftest.py(3 hunks)tests/e2e/accounts/users/conftest.py(1 hunks)tests/e2e/accounts/users/test_async_users.py(1 hunks)tests/e2e/accounts/users/test_sync_users.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
- tests/e2e/accounts/accounts_users/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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (4)
tests/e2e/accounts/users/test_async_users.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)async_mpt_ops(36-39)tests/e2e/accounts/conftest.py (3)
user_id(55-56)invalid_user_id(14-16)async_created_account_user(140-168)tests/unit/resources/accounts/test_users.py (1)
users_service(9-10)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/users/conftest.py (1)
user_factory(5-25)
tests/e2e/accounts/users/conftest.py (1)
tests/e2e/conftest.py (1)
uuid_str(106-107)
mpt_api_client/resources/accounts/users.py (1)
mpt_api_client/http/mixins.py (2)
AsyncUpdateFileMixin(219-256)UpdateFileMixin(147-184)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)user_id(117-118)tests/e2e/accounts/conftest.py (4)
async_created_account_user(140-168)account_id(30-31)user_id(55-56)account_user_factory(79-105)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)
🪛 Ruff (0.14.6)
tests/e2e/accounts/users/conftest.py
6-6: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/conftest.py
80-80: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
118-118: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
149-149: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
146-146: 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 (6)
mpt_api_client/resources/accounts/users.py (1)
6-6: LGTM! File upload support correctly implemented.The migration from
UpdateMixintoUpdateFileMixin(and async variants) along with the configuration of_upload_file_keyand_upload_data_keyproperly enables file uploads for user icon updates. The changes align with the mixin implementations shown in the relevant code snippets.Also applies to: 10-10, 30-31, 35-35, 76-76
tests/e2e/accounts/users/test_sync_users.py (1)
1-82: LGTM! Comprehensive sync e2e test coverage.The test suite provides thorough coverage of user operations including retrieval, filtering, updates with file attachments, error handling for invalid IDs, and block/unblock functionality. The test structure is clean and follows best practices.
tests/e2e/accounts/users/test_async_users.py (1)
1-70: LGTM! Comprehensive async e2e test coverage.The async test suite properly mirrors the sync version with correct async/await patterns throughout, including proper async iteration on line 28. Test coverage is thorough and well-structured.
tests/e2e/accounts/users/conftest.py (1)
1-25: LGTM! User factory fixture correctly implemented.The
user_factoryfixture provides a clean pattern for generating user payloads with sensible defaults and email uniqueness. The structure is appropriate for the/usersendpoint (distinct fromaccount_user_factorywhich serves the/accounts/{id}/usersendpoint).tests/e2e/accounts/conftest.py (1)
13-16: LGTM! Centralized fixture pattern implemented effectively.The new fixtures provide excellent reusability across test modules with proper teardown handling. The sync and async variants of
created_account_userenable consistent test patterns while ensuring resource cleanup even when teardown fails.Also applies to: 54-56, 78-105, 108-136, 139-168
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
32-35: LGTM! Migration to centralized async fixture completed successfully.The consistent replacement of
created_account_userwithasync_created_account_useracross all test functions properly leverages the centralized fixture pattern. The fixture teardown is now handled uniformly, improving test maintainability.Also applies to: 91-95, 98-104, 110-114, 133-141, 155-160, 175-180
robcsegal
force-pushed
the
MPT-14870-add-e-2-e-tests-for-users-endpoints
branch
from
e70332f to
64aa405
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/accounts/users/conftest.py (1)
4-25: Drop obsolete# noqa: WPS430to satisfy RuffRuff is flagging the inline
# noqa: WPS430on the inner_usercallable as an unknown/unused directive (RUF100). Since this rule no longer applies in your current tooling, you can safely remove it.-@pytest.fixture -def user_factory(uuid_str): - def _user( # noqa: WPS430 +@pytest.fixture +def user_factory(uuid_str): + def _user( email: str | None = None, # Must be unique in Marketplace first_name: str = "E2E Created", last_name: str = "User", ):tests/e2e/accounts/conftest.py (1)
78-105: Tighten teardown for multiple created users and remove obsoletenoqacodesThe account-user fixtures are generally solid, but there are two small points worth addressing:
- Teardown only tracks the last created user
Both
created_account_userandasync_created_account_userstore a singleret_account_user. If a test calls the inner_created_account_user()more than once, only the last created user is deleted in teardown, potentially leaving earlier ones behind. If you expect multiple calls per test, consider tracking a list of created users and deleting them all in the finalizer.
- Remove unused WPS
noqadirectives (RUF100)Ruff reports the
WPS430/WPS420/WPS421noqadirectives as unknown/unused. Since your current linter stack no longer uses these codes, you can safely drop them:@pytest.fixture def account_user_factory(account_id, user_group_id, uuid_str): - def _account_user( # noqa: WPS430 + def _account_user( email: str | None = None, # Must be unique in Marketplace first_name: str = "E2E Created", last_name: str = "Account User", @@ def _created_account_user( first_name: str = "E2E Created", last_name: str = "Account User", ): """Create an account user with the given first and last name.""" - nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_user @@ try: users_obj.delete(ret_account_user.id) except MPTAPIError: - print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}", ) @@ async def _created_account_user( first_name: str = "E2E Created", last_name: str = "Account User", ): """Create an account user with the given first and last name.""" - nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_user @@ try: await users_obj.delete(ret_account_user.id) except MPTAPIError as error: - print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}: " f"{getattr(error, 'title', str(error))}" )Also applies to: 108-137, 139-168
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
107-129: Consider asserting updated fields, not just non-NoneIn
test_update_account_user, the test currently only asserts that the result ofupdateis notNone. To make the e2e check stronger, consider also asserting that the returned resource reflects the updated data (e.g., first/last name or other relevant fields) so regressions in the update path are caught earlier.
146-151: Remove unused# noqa: AAA01to satisfy RuffRuff reports
# noqa: AAA01ontest_account_user_group_postas an unknown/unused directive (RUF100). Since this suppression is no longer recognized, you can drop it.-def test_account_user_group_post(created_account_user_group): # noqa: AAA01 +def test_account_user_group_post(created_account_user_group): """Test creating an account user group.""" result = created_account_user_group assert result is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpt_api_client/resources/accounts/users.py(3 hunks)tests/e2e/accounts/accounts_users/conftest.py(0 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(2 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(0 hunks)tests/e2e/accounts/conftest.py(3 hunks)tests/e2e/accounts/users/conftest.py(1 hunks)tests/e2e/accounts/users/test_async_users.py(1 hunks)tests/e2e/accounts/users/test_sync_users.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
- tests/e2e/accounts/accounts_users/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/accounts/users/test_sync_users.py
- tests/e2e/accounts/users/test_async_users.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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (4)
mpt_api_client/resources/accounts/users.py (1)
mpt_api_client/http/mixins.py (2)
AsyncUpdateFileMixin(219-256)UpdateFileMixin(147-184)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
tests/e2e/accounts/conftest.py (3)
async_created_account_user(140-168)account_id(30-31)account_user_factory(79-105)
tests/e2e/accounts/conftest.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
users(10-11)mpt_api_client/resources/accounts/account.py (2)
users(54-58)users(74-78)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
users(10-11)
tests/e2e/accounts/users/conftest.py (1)
tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 Ruff (0.14.7)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
146-146: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/conftest.py
80-80: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
118-118: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
149-149: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/users/conftest.py
6-6: Unused noqa directive (unknown: WPS430)
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)
mpt_api_client/resources/accounts/users.py (1)
2-11: Users file-upload integration aligns with mixin contractSwitching to
UpdateFileMixin[User]/AsyncUpdateFileMixin[User]and defining_upload_file_key = "icon"and_upload_data_key = "user"inUsersServiceConfigcleanly wires the user icon upload into the existing HTTP mixins. Both sync and async services pick up these attributes via multiple inheritance, soupdate(..., file=...)will correctly send the file under"icon"and the JSON under"user".Also applies to: 24-41, 75-83
tests/e2e/accounts/conftest.py (1)
13-17: Centralized user ID fixtures look good
invalid_user_idanduser_idbeing defined here keeps account/user-related tests consistent and localized around theaccounts.*config keys, and they plug cleanly into the 404 and happy-path tests.Also applies to: 54-57
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
30-42: Async account-user-group fixture composition looks correct
created_account_user_groupcomposesasync_created_account_userandcreated_user_groupcleanly, and the teardown path mirrors creation via the sameusers(...).groups(...).delete(...)call. Awaitingasync_created_account_user()is appropriate here because the async fixture yields an async callable, not a coroutine, so the test/fixture code correctly awaits the inner function rather than the fixture itself. Based on learnings, this matches the intended pytest-asyncio fixture semantics.
robcsegal
force-pushed
the
MPT-14870-add-e-2-e-tests-for-users-endpoints
branch
from
64aa405 to
d0455ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/accounts/conftest.py (1)
13-16: LGTM! Fixture consolidation improves test maintainability.The consolidation of shared fixtures (
invalid_user_id,user_id,account_user_factory,created_account_user,async_created_account_user) into the parent conftest.py improves code reuse across multiple test modules. The teardown logic with error handling ensures proper cleanup even when deletions fail.However, the static analysis tool has identified unused
noqadirectives. Consider cleaning these up:- def _account_user( # noqa: WPS430 + def _account_user( email: str | None = None, # Must be unique in Marketplace first_name: str = "E2E Created", last_name: str = "Account User", ):- nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_user account_user_data = account_user_factory( first_name=first_name, last_name=last_name,- except MPTAPIError: - print( # noqa: WPS421 + except MPTAPIError: + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}",And similar changes for the async fixture at lines 149 and 165.
Also applies to: 54-56, 78-105, 108-137, 139-167
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
32-35: LGTM! Proper migration to async fixture.The migration from
created_account_usertoasync_created_account_useris correctly implemented. All fixtures and tests properly await the async fixture when needed, and the sync test function at line 146 correctly receives the resolved fixture value (not a coroutine), consistent with pytest-asyncio behavior. Based on learnings, async fixtures are automatically resolved before being passed to test functions.However, the static analysis tool has identified an unused
noqadirective at line 146:-def test_account_user_group_post(created_account_user_group): # noqa: AAA01 +def test_account_user_group_post(created_account_user_group): """Test creating an account user group."""Also applies to: 39-40, 46-46, 91-95, 98-104, 107-128, 131-143, 153-170, 173-188
tests/e2e/accounts/users/conftest.py (1)
4-25: LGTM! Reusable user payload fixture.The
user_factoryfixture provides a clean, reusable way to generate user payloads for testing. The implementation correctly generates unique emails usinguuid_strand includes all required fields (firstName, lastName, email, status, settings).However, the static analysis tool has identified an unused
noqadirective:- def _user( # noqa: WPS430 + def _user( email: str | None = None, # Must be unique in Marketplace first_name: str = "E2E Created", last_name: str = "User",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpt_api_client/resources/accounts/users.py(3 hunks)tests/e2e/accounts/accounts_users/conftest.py(0 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(2 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(0 hunks)tests/e2e/accounts/conftest.py(3 hunks)tests/e2e/accounts/users/conftest.py(1 hunks)tests/e2e/accounts/users/test_async_users.py(1 hunks)tests/e2e/accounts/users/test_sync_users.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
- tests/e2e/accounts/accounts_users/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/accounts/users/test_async_users.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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (5)
tests/e2e/accounts/conftest.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (3)
user_id(117-118)e2e_config(89-92)uuid_str(106-107)mpt_api_client/resources/accounts/account.py (2)
users(54-58)users(74-78)
mpt_api_client/resources/accounts/users.py (1)
mpt_api_client/http/mixins.py (2)
AsyncUpdateFileMixin(219-256)UpdateFileMixin(147-184)
tests/e2e/accounts/users/test_sync_users.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
mpt_vendor(19-20)mpt_ops(31-32)tests/e2e/accounts/conftest.py (2)
user_id(55-56)invalid_user_id(14-16)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
users(10-11)test_filter_account_users(74-82)tests/e2e/accounts/users/conftest.py (1)
user_factory(5-25)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/accounts/conftest.py (3)
async_created_account_user(140-168)account_id(30-31)account_user_factory(79-105)
tests/e2e/accounts/users/conftest.py (1)
tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 Ruff (0.14.7)
tests/e2e/accounts/conftest.py
80-80: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
118-118: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
149-149: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
146-146: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/users/conftest.py
6-6: Unused noqa directive (unknown: WPS430)
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)
mpt_api_client/resources/accounts/users.py (1)
6-6: LGTM! File upload support correctly implemented.The replacement of
UpdateMixinwithUpdateFileMixin(and async variants) correctly enables file upload support for user icon updates. The upload keys are properly configured to match the multipart form data structure expected by the API.Also applies to: 10-10, 30-31, 35-35, 76-76
tests/e2e/accounts/conftest.py (1)
5-5: LGTM! Exception handling import added.The import of
MPTAPIErroris correctly added to support error handling in teardown logic for the new fixtures.tests/e2e/accounts/users/test_sync_users.py (1)
1-82: LGTM! Comprehensive e2e test coverage.The new test file provides thorough end-to-end test coverage for synchronous user operations including:
- User retrieval by valid and invalid IDs
- RQL-based filtering
- User updates with file uploads
- Block/unblock operations
- Proper 404 error handling
The tests follow consistent patterns and appropriately use shared fixtures from the parent conftest.py.
robcsegal
force-pushed
the
MPT-14870-add-e-2-e-tests-for-users-endpoints
branch
from
d0455ab to
76abcb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/accounts/conftest.py (1)
80-80: Remove obsolete noqa directives.Ruff doesn't recognize the wemake-python-styleguide codes (WPS430, WPS420, WPS421). Either remove these directives or ensure the appropriate linter is configured.
Apply this diff to remove the unused directives:
- def _account_user( # noqa: WPS430 + def _account_user( email: str | None = None, # Must be unique in Marketplace- nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_user- print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}",- nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_user- print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}: "Also applies to: 118-118, 134-134, 149-149, 165-165
tests/e2e/accounts/users/conftest.py (1)
6-6: Remove obsolete noqa directive.Ruff doesn't recognize the wemake-python-styleguide code (WPS430). Either remove this directive or ensure the appropriate linter is configured.
Apply this diff:
- def _user( # noqa: WPS430 + def _user( email: str | None = None, # Must be unique in Marketplacetests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
146-146: Remove obsolete noqa directive.Ruff doesn't recognize the AAA01 code. Either remove this directive or ensure the appropriate linter is configured.
Apply this diff:
-def test_account_user_group_post(created_account_user_group): # noqa: AAA01 +def test_account_user_group_post(created_account_user_group): """Test creating an account user group."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpt_api_client/resources/accounts/users.py(3 hunks)tests/e2e/accounts/accounts_users/conftest.py(0 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(2 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(0 hunks)tests/e2e/accounts/conftest.py(3 hunks)tests/e2e/accounts/users/conftest.py(1 hunks)tests/e2e/accounts/users/test_async_users.py(1 hunks)tests/e2e/accounts/users/test_sync_users.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
- tests/e2e/accounts/accounts_users/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/accounts/users/test_async_users.py
- mpt_api_client/resources/accounts/users.py
- tests/e2e/accounts/users/test_sync_users.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (1)
tests/e2e/accounts/users/conftest.py (1)
tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 Ruff (0.14.7)
tests/e2e/accounts/conftest.py
80-80: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
118-118: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
149-149: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
146-146: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/users/conftest.py
6-6: Unused noqa directive (unknown: WPS430)
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 (13)
tests/e2e/accounts/conftest.py (6)
5-5: LGTM! Appropriate error handling import.The MPTAPIError import is correctly used in the teardown sections of both fixture variants.
13-16: LGTM! Shared fixture reduces duplication.Moving
invalid_user_idto the parent conftest appropriately supports reuse across both users and accounts-users test modules.
54-56: LGTM! Exposes user ID for tests.The fixture correctly extracts the user ID from e2e configuration.
78-105: LGTM! Factory pattern provides flexible test data.The factory correctly constructs account user payloads with sensible defaults and optional overrides.
108-137: LGTM! Sync fixture correctly manages lifecycle.The fixture properly creates account users with a factory function pattern, uses nonlocal to track state, and handles teardown errors gracefully.
139-168: LGTM! Async variant mirrors sync fixture correctly.The async fixture properly uses await for all async operations and extracts error titles in the teardown handler for better debugging.
tests/e2e/accounts/users/conftest.py (1)
4-25: LGTM! Factory provides consistent user payloads.The fixture correctly constructs user dictionaries with sensible defaults, unique email generation, and proper settings structure.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (6)
32-35: LGTM! Fixture correctly migrated to async variant.The fixture properly awaits
async_created_account_user()to obtain user data and uses it for group creation.
53-60: LGTM! Test structure improved.The whitespace changes improve readability by separating the operation from the assertion.
62-70: LGTM! Result stored before assertion.Storing the result in a variable before asserting is good practice for debugging and clarity.
72-78: LGTM! Test correctly validates 404 error.The test properly stores the users object and verifies that accessing an invalid user ID raises MPTAPIError with a 404 status.
80-89: LGTM! Filter test improved.Collecting results into a list before asserting length is clearer and easier to debug.
91-95: LGTM! Tests correctly migrated to async fixture.All tests properly await the
async_created_account_user()factory function to obtain user data and then perform their respective operations. The pattern is consistent across create, delete, update, resend invite, and group operations.Also applies to: 98-104, 107-128, 131-143, 153-170, 173-188
|
Added Accounts users e2e tests. Moving some fixtures because they're shared among a couple of Account resources.
https://softwareone.atlassian.net/browse/MPT-14870
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.