[MPT-14865] Added Accounts account by id users endpoints e2e tests #131
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
2 times, most recently
from
4646731 to
fc98886
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
fc98886 to
650a448
Compare
|
Warning Rate limit exceeded@robcsegal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughRefactors AccountsUserGroups service mixins and adds sync/async update methods, adds a test user ID to e2e config, introduces several pytest fixtures (UUID, user factory, invalid user id), and adds new synchronous and asynchronous end-to-end tests plus unit tests validating the update flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
2 times, most recently
from
c8bdc1e to
8fdacf2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
9-38: Account-user fixture cleanup only tracks the last created user
created_account_userkeeps a singleret_account_userreference. If a test ever callscreated_account_user()multiple times, only the last created user will be cleaned up in teardown. Also, intest_delete_account_user, the user is explicitly deleted in the test and then (expectedly) fails deletion again in teardown, causing a “TEARDOWN - Unable to delete…” message on the happy path.If you anticipate multiple users per test or want quieter logs, consider tracking a list of created users and deleting each once, or having the fixture skip teardown when the test already performed the delete.
125-155: Consider asserting more on update and group-update responses
test_update_account_user,test_account_user_resend_invite, andtest_account_user_group_updatecurrently only assert that the returned objects are notNone. To better guard against regressions in the update / resend / group-association behavior, you could also assert on key fields (e.g., updatedfirst_name/last_nameor that returned group ids match the request).Also applies to: 163-177
tests/e2e/conftest.py (1)
111-133: New licensee/user ID fixtures are correct; consider deduplicating with licensees conftestThese fixtures correctly pull from
e2e_configand will be reused across e2e modules. Given thattests/e2e/accounts/licensees/conftest.pyalready defines similarlicensee_*fixtures, you might later want to centralize them in one conftest (and drop the duplicates) to avoid divergence if the config keys ever change.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
14-14: Static analysis hints are false positives.The
noqadirectives reference wemake-python-styleguide (WPS) codes, which Ruff doesn't recognize. You can either:
- Keep them if your project uses wemake-python-styleguide in CI
- Remove them if only using Ruff
- Configure Ruff to ignore RUF100 for these patterns
Based on static analysis hints.
Also applies to: 19-19, 37-37, 55-55, 80-80
171-175: Consider making this test async for consistency.This is the only synchronous test function in an otherwise fully async test module. While pytest-asyncio can handle sync tests consuming async fixtures, making it async would be more consistent with the rest of the test suite.
Apply this diff to align with other tests:
-def test_account_user_group_post(created_account_user_group): +async def test_account_user_group_post(created_account_user_group): """Test creating an account user group.""" created_account_user_group_data = created_account_user_group assert created_account_user_group_data is not None
83-89: Consider strengthening test assertions.Several tests only verify that a response was returned (
is not None) without checking the actual data or operation success. E2e tests benefit from more comprehensive assertions.Examples of improvements:
For test_get_account_user_by_id (line 88):
- assert account_user is not None + assert account_user is not None + assert account_user.id == user_idFor test_update_account_user (line 155):
- assert updated_account_user is not None + assert updated_account_user is not None + assert updated_account_user.first_name == "E2E Updated" + assert updated_account_user.last_name == "Account User"For test_delete_account_user (lines 132-134):
await async_mpt_vendor.accounts.accounts.users(account_id=account_id).delete( account_user_data.id ) # Verify deletion with pytest.raises(MPTAPIError, match=r"404 Not Found"): await async_mpt_vendor.accounts.accounts.users(account_id=account_id).get( account_user_data.id )Also applies to: 123-127, 129-135, 137-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(4 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(3 hunks)setup.cfg(1 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/http/test_mixins.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/accounts/accounts_users/conftest.py (1)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)
tests/e2e/conftest.py (1)
tests/e2e/accounts/licensees/conftest.py (4)
licensee_id(15-16)invalid_licensee_id(20-21)licensee_account_id(5-6)licensee_group_id(10-11)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (9)
AsyncManagedResourceWithNoUpdateIdMixin(681-684)ManagedResourceWithNoUpdateIdMixin(675-678)UpdateWithNoUpdateIdMixin(59-74)update(45-56)update(62-74)update(170-202)update(235-246)update(252-264)update(361-393)
mpt_api_client/resources/accounts/accounts_user_groups.py (1)
mpt_api_client/http/mixins.py (2)
AsyncManagedResourceWithNoUpdateIdMixin(681-684)ManagedResourceWithNoUpdateIdMixin(675-678)
🪛 Ruff (0.14.5)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
35-35: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
51-51: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
72-72: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
13-13: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
214-214: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
1151-1151: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
1171-1171: Unused noqa directive (unknown: WPS431)
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/accounts_user_groups.py (1)
2-7: Switch toManagedResourceWithNoUpdateIdMixinfor user groups looks correctUsing
ManagedResourceWithNoUpdateIdMixin/AsyncManagedResourceWithNoUpdateIdMixinhere cleanly models thePUT /accounts/{account_id}/users/{user_id}/groupssemantics (no id in the path) while still keepingget/deleteby id via the existing mixins. This matches how the e2e tests exercise.groups(...).update(...)and.groups(...).delete(group_id).Also applies to: 23-37
mpt_api_client/http/mixins.py (1)
59-75: New NoUpdateId update/managed mixins are consistent with existing HTTP mixin patternsThe
UpdateWithNoUpdateIdMixin/AsyncUpdateWithNoUpdateIdMixinimplementations correctly mirrorCreateMixinbehavior while targetingself.path(no id in the URL), and the newModifiableResourceWithNoUpdateIdMixin/ManagedResourceWithNoUpdateIdMixin(and async variants) compose them in the same way as the existing id-based mixins. This keeps the public API shape coherent while supporting endpoints that update on the collection URL.Also applies to: 249-265, 653-684
e2e_config.test.json (1)
2-24: Newaccounts.user.idtest config entry is consistent and wiredThe added
"accounts.user.id"key follows the existing naming scheme and is correctly consumed by theuser_idfixture intests/e2e/conftest.py, so the e2e tests can rely on a single source of truth for that id.setup.cfg (1)
34-56: Per-file WPS ignores align with the new unit/e2e testsThe updated
per-file-ignoresentries fortests/unit/http/test_mixins.pyandtests/e2e/accounts/accounts_users/*.pyare scoped to test modules that legitimately need looser style checks, without broadening ignores elsewhere.tests/unit/http/test_mixins.py (1)
33-59: Unit coverage for NoUpdateId update/managed mixins is solidThe new dummy services and fixtures for
UpdateWithNoUpdateIdMixin/AsyncUpdateWithNoUpdateIdMixincorrectly target the/api/v1/update-no-id/endpoint, and the tests validate both the PUT body and that the mocked route is hit once. Thetest_*managed_resource_with_no_id_mixinchecks mirror the existing managed mixin tests, confirming thatcreate/update/delete/getare all present on the new composites.Also applies to: 212-228, 277-291, 141-180
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
1-6: LGTM!The imports are appropriate, and marking the entire module as flaky is a good practice for e2e tests that may be subject to external dependencies and timing issues.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
8fdacf2 to
8f22ca9
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
8f22ca9 to
c740e11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
tests/unit/http/test_mixins.py (2)
1105-1119: Remove unusednoqadirective.The
noqa: WPS431comment on line 1114 is flagged as unused by Ruff, suggesting it's not needed for the current linting setup.Apply this diff:
- class ManagedServiceNoUpdateId(CreatableResourceMixin[DummyModel]): # noqa: WPS431 + class ManagedServiceNoUpdateId(CreatableResourceMixin[DummyModel]): """Dummy service class for testing required methods."""
1122-1139: Remove unusednoqadirective.The
noqa: WPS431comment on line 1131 is flagged as unused by Ruff.Apply this diff:
- class AsyncCreatableResourceService(AsyncCreatableResourceMixin[DummyModel]): # noqa: WPS431 + class AsyncCreatableResourceService(AsyncCreatableResourceMixin[DummyModel]): """Dummy service class for testing required methods."""tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (3)
9-38: Remove unusednoqadirectives.Ruff flags the
noqacomments on lines 14, 19, and 35 as unused. These can be safely removed.Apply this diff:
- def _created_account_user( # noqa: WPS430 + 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 account_user_data = account_user_factory( first_name=first_name, last_name=last_name, @@ -32,7 +32,7 @@ try: mpt_vendor.accounts.accounts.users(account_id=account_id).delete(ret_account_user.id) except MPTAPIError: - print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}", )
40-52: Remove unusednoqadirective.The
noqacomment on line 51 is flagged as unused.Apply this diff:
except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete user group: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete user group: {error.title}")
54-73: Remove unusednoqadirective.The
noqacomment on line 72 is flagged as unused.Apply this diff:
except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete account user group: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete account user group: {error.title}")tests/e2e/accounts/accounts_users/conftest.py (1)
9-36: Remove unusednoqadirective.The
noqa: WPS430comment on line 11 is flagged as unused by Ruff.Apply this diff:
- 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",Otherwise, the factory correctly generates unique emails by default, addressing the email uniqueness requirement mentioned in past reviews.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
9-40: Remove unusednoqadirectives.Ruff flags the
noqacomments on lines 14, 19, and 37 as unused.Apply this diff:
- async def _created_account_user( # noqa: WPS430 + 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 account_user_data = account_user_factory( first_name=first_name, last_name=last_name, @@ -34,7 +34,7 @@ ret_account_user.id ) except MPTAPIError: - print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}", )
42-56: Remove unusednoqadirective.The
noqacomment on line 55 is flagged as unused.Apply this diff:
except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete user group: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete user group: {error.title}")
58-81: Remove unusednoqadirective.The
noqacomment on line 80 is flagged as unused.Apply this diff:
except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete account user group: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete account user group: {error.title}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)setup.cfg(1 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- mpt_api_client/http/mixins.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/e2e/conftest.py (1)
tests/e2e/accounts/licensees/conftest.py (4)
licensee_id(15-16)invalid_licensee_id(20-21)licensee_account_id(5-6)licensee_group_id(10-11)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
mpt_api_client/http/mixins.py (4)
update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(32-43)update(54-65)
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
mpt_api_client/http/mixins.py (6)
AsyncCreatableResourceMixin(631-634)CreatableResourceMixin(627-628)update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/catalog/products.py (2)
update(103-129)update(215-241)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (5)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (3)
async_mpt_vendor(24-27)async_mpt_ops(36-39)user_id(137-138)tests/e2e/accounts/accounts_users/conftest.py (2)
account_user_factory(10-36)invalid_user_id(5-6)tests/e2e/accounts/conftest.py (1)
account_id(22-23)mpt_api_client/models/model.py (1)
id(119-121)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (2)
AsyncCreatableResourceMixin(631-634)CreatableResourceMixin(627-628)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 Ruff (0.14.5)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
1114-1114: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
1131-1131: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
35-35: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
51-51: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
72-72: 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 (19)
setup.cfg (2)
47-47: LGTM!The additional linter suppressions are appropriate for a test file that includes new test cases for CreatableResourceMixin variants.
53-53: LGTM!The linter suppressions for the new e2e test directory are standard and appropriate.
tests/unit/http/test_mixins.py (3)
12-12: LGTM!The new mixin imports are necessary for the test scaffolding added below.
Also applies to: 17-17
31-47: LGTM!The test service classes provide appropriate scaffolding for testing the new CreatableResourceMixin variants.
49-56: LGTM!The fixtures follow the established pattern for test service instantiation.
tests/unit/resources/accounts/test_accounts_user_groups.py (4)
1-5: LGTM!The additional imports are necessary for the new update tests.
43-56: LGTM!Correctly updated the parametrized mixin tests to reflect that CreatableResourceMixin provides create and delete (and get), but not update. The update method is now implemented separately in the service class.
59-84: LGTM!The test correctly validates the synchronous update method, verifying both the request payload and response handling.
86-110: LGTM!The async update test correctly mirrors the synchronous version and properly validates the asynchronous flow.
mpt_api_client/resources/accounts/accounts_user_groups.py (4)
2-9: LGTM!The import changes correctly reflect the switch from ManagedResourceMixin to CreatableResourceMixin and add the ResourceData type needed for the update methods.
24-30: LGTM!The switch to CreatableResourceMixin is appropriate for this resource where updates are applied to the collection path rather than individual resource paths.
32-43: LGTM!The custom update method correctly implements PUT to the collection path, which is appropriate for updating account user group associations.
54-65: LGTM!The async update method correctly mirrors the synchronous implementation.
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
1-6: LGTM!The imports and flaky marker are appropriate for e2e tests that may have intermittent failures.
75-196: LGTM!The test suite provides comprehensive coverage of account user operations, including CRUD, filtering, invite management, and group associations.
tests/e2e/accounts/accounts_users/conftest.py (1)
4-6: LGTM!The invalid_user_id fixture provides a constant value for testing error handling.
tests/e2e/conftest.py (1)
105-107: LGTM!The uuid_str fixture provides full UUID strings for generating unique test data, complementing the existing short_uuid fixture.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
1-6: LGTM!The imports and flaky marker are appropriate for async e2e tests.
177-212: LGTM!The remaining async tests correctly cover update and delete operations for account user groups.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
c740e11 to
288d468
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
288d468 to
be20cdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/http/test_mixins.py (1)
1114-1138: Clean up unusednoqadirectives for WPS431Ruff reports the
# noqa: WPS431comments on the inner classes intest_sync_creatable_resource_mixinandtest_async_creatable_resource_mixinas unused (RUF100). If you still need to silence wemake’s WPS431 here, it’s better to move that suppression intosetup.cfgper-file-ignores; otherwise, you can just drop thesenoqacomments.This keeps ruff noise down without changing test behavior.
tests/e2e/accounts/accounts_users/conftest.py (1)
4-36: Account user factory shape and email generation look solidThe
account_user_factorybuilds the expected payload structure (user/account/groups/invitation) and now usesemail: str | None = Nonewith a UUID-based fallback fromuuid_str, which addresses uniqueness concerns for test-created users.Ruff does flag the
# noqa: WPS430on_account_useras an unused directive (RUF100), so if wemake doesn’t strictly require suppressing WPS430 here, you could safely drop thatnoqato clean up lint noise.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
14-72: Optional: trim unusednoqadirectives flagged by RuffRuff reports several
# noqa: WPS4xxcomments in this file (e.g., for WPS430, WPS420, WPS421) as unused (RUF100). If wemake isn’t enforcing those particular rules here, you could drop those inlinenoqatags to reduce linter noise; otherwise, consider moving them intosetup.cfgper-file-ignores for this path so both tools stay in sync.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
14-80: Optional: remove or centralize WPS-relatednoqacommentsRuff reports several
# noqa: WPS4xxdirectives in this file (for WPS430, WPS420, WPS421) as unused (RUF100). If wemake isn’t actively enforcing those checks here, consider dropping the inlinenoqacomments. If you need them for flake8, moving them intosetup.cfgper-file-ignores for this path will avoid ruff’s “unknown code” complaints while still suppressing wemake rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)setup.cfg(1 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- mpt_api_client/http/mixins.py
- e2e_config.test.json
- mpt_api_client/resources/accounts/accounts_user_groups.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (3)
tests/e2e/conftest.py (2)
mpt_vendor(19-20)user_id(117-118)tests/e2e/accounts/accounts_users/conftest.py (2)
account_user_factory(10-36)invalid_user_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
mpt_api_client/http/mixins.py (4)
update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(32-43)update(54-65)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[error] 171-171: RUF029: Function test_account_user_group_post is declared async, but doesn't await or use async features.
🪛 Ruff (0.14.6)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
35-35: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
51-51: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
72-72: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
1114-1114: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
1131-1131: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (7)
setup.cfg (1)
47-54: Per-file ignores for new/expanded tests look consistentThe extra ignores for
tests/unit/http/test_mixins.pyand the new entry fortests/e2e/accounts/accounts_users/*.pyalign with how other test modules are configured for wemake rules. No issues from a tooling/maintenance standpoint.tests/unit/http/test_mixins.py (1)
31-57: Creatable dummy services and fixtures are coherent with existing mixin tests
DummyCreatableResourceService/AsyncDummyCreatableResourceServiceand their fixtures are wired the same way as the other dummy services in this module (endpoint, model, collection key), so they’re well-aligned for any future behavior-level tests you might add aroundCreatableResourceMixin/AsyncCreatableResourceMixin. The current mixin API surface checks at the bottom of the file are also consistent.tests/unit/resources/accounts/test_accounts_user_groups.py (1)
59-110: Update tests match the AccountsUserGroupsService contractBoth sync and async
test_*account_user_groups_updatecorrectly:
- Hit the expected
PUT /public/v1/accounts/{account_id}/users/{user_id}/groupsURL.- Send the JSON body as a compact-encoded list of group ids.
- Assert that the returned model mirrors the mocked response payload.
This aligns with the
update(resource_data: ResourceData)implementations inaccounts_user_groups.py.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
9-73: Fixture-based lifecycle management for users and groups is reasonableThe
created_account_user,created_user_group, andcreated_account_user_groupfixtures encapsulate create/teardown well and make the individual tests concise. The teardown paths correctly guard against failures (e.g., double-deletes intest_delete_account_user) by catchingMPTAPIError, so they won’t interfere with subsequent tests.
75-196: Sync e2e coverage of account users and user-groups looks completeThe tests cover:
- Happy-path
get,fetch_page, filtering viaRQLQuery, and404behavior.- User lifecycle: create, delete, update, and resend_invite.
- Group association lifecycle: create (
.groups(...).create), update (.groups(...).update), and delete (.groups(...).delete).These flows are aligned with the client API and the new Accounts user-groups update method. Any extra assertions you might later want (e.g., verifying updated names or group IDs) can be layered on without changing the structure.
tests/e2e/conftest.py (1)
105-118: New shared fixtures cleanly support the accounts-users e2e tests
uuid_stranduser_idare minimal, focused fixtures:
uuid_strcentralizes UUID generation for tests that need unique identifiers (e.g., emails inaccount_user_factory).user_idcleanly exposesaccounts.user.idfrome2e_configto tests that require a known existing user.The additions fit well with the existing e2e fixture layout.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
9-81: Async fixtures and teardown logic are consistent and safeThe async fixtures for account users, user groups, and account-user-groups correctly:
- Use the async clients (
async_mpt_vendor,async_mpt_ops) with awaited create/delete calls.- Capture created resources for teardown, catching
MPTAPIErrorto avoid teardown failures leaking into unrelated tests.This mirrors the sync test module’s structure while respecting async semantics.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
be20cdb to
7a4955a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
mpt_api_client/http/mixins.py (1)
627-634: Fix redundant wording in docstrings.Both docstrings contain "resource resources" which is redundant. The phrase should be simplified.
Apply this diff to improve the docstrings:
class CreatableResourceMixin[Model](CreateMixin[Model], GetMixin[Model], DeleteMixin): - """Managed resource mixin to read, create, and delete a resource resources.""" + """Managed resource mixin to read, create, and delete resources.""" class AsyncCreatableResourceMixin[Model]( AsyncCreateMixin[Model], AsyncGetMixin[Model], AsyncDeleteMixin ): - """Managed resource mixin to read, create, and delete a resource resources.""" + """Managed resource mixin to read, create, and delete resources."""tests/unit/http/test_mixins.py (1)
1105-1139: Clean up unused noqa directives.The tests correctly verify that the new
CreatableResourceMixinandAsyncCreatableResourceMixinprovide create, get, and delete methods (but not update). However, static analysis indicates thenoqa: WPS431directives on lines 1114 and 1131 are unused.Based on static analysis hints, remove the unused noqa directives:
- class ManagedServiceNoUpdateId(CreatableResourceMixin[DummyModel]): # noqa: WPS431 + class ManagedServiceNoUpdateId(CreatableResourceMixin[DummyModel]): """Dummy service class for testing required methods."""- class AsyncCreatableResourceService(AsyncCreatableResourceMixin[DummyModel]): # noqa: WPS431 + class AsyncCreatableResourceService(AsyncCreatableResourceMixin[DummyModel]): """Dummy service class for testing required methods."""tests/e2e/accounts/accounts_users/conftest.py (1)
4-36: Fixtures look correct; email uniqueness handled viauuid_str
invalid_user_idandaccount_user_factorylook good. Usinguuid_strto generatee2e_{uuid_str}@dummy.comgives each test-run a unique email while keeping call sites simple and fixes the previous uniqueness concern.One minor point: Ruff reports
RUF100for# noqa: WPS430(unknown code). If wemake-python-styleguide isn’t in use anymore, consider dropping theseWPS…noqacomments (or mapping them to actual Ruff codes) to avoid unused-noqanoise.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
9-81: Async fixtures and teardown logic look solidThe async fixtures for account users, user groups, and account user groups are well-structured: they yield usable callables/objects and reliably attempt teardown with
MPTAPIErrorhandling, matching the sync counterparts. This should give stable lifecycle coverage for the async e2e suite.Optional: Ruff is flagging the
# noqa: WPS4xx/WPS421directives asRUF100(unknownnoqacodes). If wemake isn’t part of your lint stack anymore, you could drop thoseWPS…noqacomments here to reduce lint noise, or update them to the corresponding Ruff rule codes if you still want the exemptions.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
9-196: Sync e2e coverage for account users and groups looks consistent; consider minor lint and assertion polishThe sync fixtures and tests mirror the async suite cleanly: user/group/account-user-group lifecycle is exercised end-to-end (get/list/404/filter/create/delete/update/resend_invite and group CRUD), with teardown guarded by
MPTAPIError. The overall structure and API usage look correct and should provide solid coverage.A couple of optional refinements:
- Ruff reports
RUF100on the# noqa: WPS4xx/WPS421comments (unknown codes). If wemake isn’t active anymore, you can safely drop theseWPS…noqadirectives, or replace them with appropriate Ruff codes if you still want to suppress specific checks.- For stronger behavioral checks (if you want to tighten tests later), you could assert on updated fields, e.g., verifying the updated user’s first name in
test_update_account_userrather than justis not None.Both are non-blocking; the current implementation is acceptable as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)setup.cfg(1 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/resources/accounts/test_accounts_user_groups.py
- setup.cfg
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (5)
mpt_api_client/resources/accounts/accounts_user_groups.py (5)
mpt_api_client/http/mixins.py (6)
AsyncCreatableResourceMixin(631-634)CreatableResourceMixin(627-628)update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/accounts/buyers.py (2)
update(82-108)update(170-196)mpt_api_client/resources/catalog/products.py (2)
update(103-129)update(215-241)mpt_api_client/http/base_service.py (1)
path(28-30)mpt_api_client/http/types.py (1)
json(40-42)
mpt_api_client/http/mixins.py (1)
mpt_api_client/models/model.py (1)
Model(65-125)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (6)
AsyncCreatableResourceMixin(631-634)AsyncCreateWithIconMixin(289-319)AsyncManagedResourceMixin(621-624)AsyncModifiableResourceMixin(611-614)AsyncUpdateWithIconMixin(322-357)CreatableResourceMixin(627-628)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (4)
tests/e2e/conftest.py (3)
mpt_vendor(19-20)mpt_ops(31-32)user_id(117-118)tests/e2e/accounts/accounts_users/conftest.py (1)
account_user_factory(10-36)tests/e2e/accounts/conftest.py (1)
account_id(22-23)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[error] 171-171: Ruff: Function test_account_user_group_post is declared async, but doesn't await or use async features. (RUF029)
🪛 Ruff (0.14.6)
tests/unit/http/test_mixins.py
1114-1114: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
1131-1131: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
35-35: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
51-51: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
72-72: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (5)
tests/unit/http/test_mixins.py (1)
31-56: LGTM!The test service classes and fixtures are well-structured and follow the existing patterns in the codebase.
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
4-6: LGTM!The switch from
ManagedResourceMixintoCreatableResourceMixinis appropriate for this use case where updates don't require a resource ID. The import ofResourceDatasupports the new update methods.Also applies to: 9-9, 25-25, 47-47
32-43: LGTM!The update methods correctly implement collection-level PUT operations. This pattern is appropriate for managing user group memberships where the entire set of groups is replaced rather than updating individual group resources. The implementations properly:
- Use
self.pathto target the collection endpoint- Pass
resource_dataas JSON payload- Return the model parsed from the response
Both sync and async implementations follow the established patterns in the codebase.
Also applies to: 54-65
e2e_config.test.json (1)
12-12: Confirmaccounts.user.idmatches an existing user for the configured accountThe new
accounts.user.identry is fine structurally; just ensureUSR-9673-3314actually exists and belongs toaccounts.account.idin the e2e environment, otherwise the new get/filter tests will be flaky or consistently failing.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
171-175: I'll verify the review comment by examining the test file and checking the relevant context.
<function_calls>
cat -n tests/e2e/accounts/accounts_users/test_async_accounts_users.py | sed -n '165,180p'
</function_calls>
<function_calls>
wc -l tests/e2e/accounts/accounts_users/test_async_accounts_users.py
</function_calls>Let me check if the file exists first:
<function_calls>
find . -name "test_async_accounts_users.py" -type f
</function_calls>Let me search more broadly:
<function_calls>
find . -path "/tests/e2e/accounts/accounts_users/" -name "*.py" | head -20
</function_calls>Let me check the overall directory structure:
<function_calls>
git ls-files | grep -E "test.*accounts.*users" | head -20
</function_calls>Let me get a broader view:
<function_calls>
git ls-files | grep -E "test_async" | head -20
</function_calls><function_calls>
git ls-files | grep -i accounts | head -30
</function_calls><function_calls>
ls -la tests/e2e/ 2>/dev/null || echo "Directory not found"
</function_calls><function_calls>
find . -type d -name "tests" 2>/dev/null | head -5
</function_calls>Let me check the repository structure:
<function_calls>
ls -la
</function_calls><function_calls>
git ls-files | head -50
</function_calls>Based on the paths in the learnings and review comment, let me search for this specific test file:
<function_calls>
rg "test_account_user_group_post" --type py
</function_calls><function_calls>
rg "created_account_user_group" --type py -l
</function_calls>Let me check all Python test files:
<function_calls>
fd "test.*.py$" | head -30
</function_calls>Let me try a different approach and search for the fixture:
<function_calls>
rg "created_account_user_group" . 2>/dev/null
</function_calls>Let me check if there's a different branch or if the file is in the PR changes:
<function_calls>
git branch -a && git status
</function_calls><function_calls>
git log --oneline -10
</function_calls>Let me check the repository more carefully:
<function_calls>
pwd && find . -maxdepth 3 -type d | sort
</function_calls>Let me check what files were changed in this PR or branch:
<function_calls>
git diff --name-only HEAD~1..HEAD 2>/dev/null || git diff --name-only HEAD 2>/dev/null | head -20
</function_calls>Let me try to check all commits:
<function_calls>
git log --name-only -1 --oneline
</function_calls>Let me check what we have in the current state:
<function_calls>
ls -R . | grep -E "(test_async|accounts_users)" | head -20
</function_calls>Let me search the entire codebase for pytest or test patterns:
<function_calls>
fd ".py$" | xargs grep -l "async def test_" 2>/dev/null | head -10
</function_calls>Let me check if the learning reference gives us clues about the actual file path in the PR:
<function_calls>
git show --stat 2>/dev/null | head -50
</function_calls>Let me check the basic repo info:
<function_calls>
git remote -v && git rev-parse --abbrev-ref HEAD
</function_calls>Since I'm having difficulty locating the file directly, let me try to understand the pytest-asyncio behavior better to validate the review comment's claim:
<function_calls>
rg "pytest" . --type py | head -5
</function_calls><function_calls>
find . -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" | head -5
</function_calls>Let me try to check for pytest configuration:
<function_calls>
cat pyproject.toml 2>/dev/null | head -100
</function_calls>Let me search for actual test files that use async:
<function_calls>
fd "test.*.py$" --exec rg "async def test_" {} ;
</function_calls><function_calls>
rg "async def test_" --type py -A 3 | head -50
</function_calls>Let me try a simpler approach - list all Python files:
<function_calls>
fd ".py$" -t f | wc -l
</function_calls><function_calls>
fd ".py$" -t f | grep -i test | head -30
</function_calls>Let me check if there are multiple repositories or if this is a fresh clone:
<function_calls>
git config --local --list | grep url
</function_calls>Based on the context provided in the review comment, the file should be at
tests/e2e/accounts/accounts_users/test_async_accounts_users.py. Let me verify if this path exists in any form:
<function_calls>
test -f "tests/e2e/accounts/accounts_users/test_async_accounts_users.py" && echo "File exists" || echo "File does not exist"; ls -la "tests/" 2>&1 | head -20
</function_calls>Let me check the entire directory tree depth:
<function_calls>
find . -type f -name "*.py" | grep -E "(test|spec)" | sort
</function_calls>Let me check what's in the root:
<function_calls>
ls -la . | head -40
</function_calls>Let me use git to understand what files are available:
<function_calls>
git ls-tree -r HEAD --name-only | wc -l
</function_calls><function_calls>
git ls-tree -r HEAD --name-only | grep test | head -50
</function_calls>Based on the evidence that I cannot locate the test file in the current repository state, let me document this finding and provide guidance:
I'm unable to locate the test file
tests/e2e/accounts/accounts_users/test_async_accounts_users.pyin the current repository state after exhaustive searching. This prevents me from verifying the review comment's specific claims about:
- Whether the function truly contains no
awaitstatements- The exact fixture and code structure
- The context around lines 171-175
Given the learnings provided confirm that pytest-asyncio automatically resolves async fixtures before passing them to test functions (both sync and async), the general principle in the review comment appears sound. However, without access to the actual code, I cannot definitively verify the specific scenario.
The review comment suggests converting an async test function that doesn't await anything to a sync function to satisfy RUF029. This follows the correct pytest-asyncio pattern documented in the learnings - sync test functions can safely consume async fixtures without additional awaits. However, the file cannot be located in the repository for verification of the actual code state and specific line numbers mentioned (171-175).
| AsyncCreatableResourceMixin[AccountsUserGroup], | ||
| AsyncCollectionMixin[AccountsUserGroup], | ||
| AsyncService[AccountsUserGroup], | ||
| AccountsUserGroupsServiceConfig, | ||
| ): | ||
| """Asynchronous Account User Groups Service.""" | ||
|
|
There was a problem hiding this comment.
This one of a time exclusive setup calls I would not create them as Mixin or if we do, I would presonally move them to the resource file
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
7a4955a to
1d05f5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
mpt_api_client/http/mixins.py (1)
627-634: Clarify docstrings for CreatableResourceMixin classesThe implementations look good and the MRO matches the intended
create/get/deletecomposition. The docstrings still say “Managed resource mixin…”, which can be confusing now that there are both Managed and Creatable resource mixins. Consider updating the text to “Creatable resource mixin...” (and async variant) to keep naming consistent.tests/e2e/accounts/accounts_users/conftest.py (1)
4-36: Account user fixtures look correct and ensure unique emails
invalid_user_idis a simple, readable fixture for 404 scenarios.account_user_factorynow defaultsNoneand derives a unique address fromuuid_strwhen omitted, which avoids cross-test collisions while keeping the payload shape clear.If you ever enable Ruff’s
RUF100in CI, it will flag# noqa: WPS430as an unknown code; in that case you’ll either need to adjust Ruff’s config or drop thenoqaif WPS430 is no longer enforced by flake8.tests/unit/http/test_mixins.py (1)
31-57: Creatable mixin tests are good; consider using or dropping the new fixturesThe new sync/async tests for
CreatableResourceMixinandAsyncCreatableResourceMixinfollow the existing pattern for managed/modifiable mixins and correctly assert the presence/callability ofcreate,get, anddelete.Right now,
DummyCreatableResourceService,AsyncDummyCreatableResourceService, and their fixtures are not used by any test in this module. If you don’t plan to add behavior-level tests that actually callcreate/get/deleteon these services, consider removing the unused fixtures (and possibly the dummy classes) to keep the test module lean.Also applies to: 1080-1114
mpt_api_client/resources/accounts/accounts_user_groups.py (1)
2-7: AccountsUserGroupsService update methods match the new collection-level API
- Switching to
CreatableResourceMixin/AsyncCreatableResourceMixincleanly drops the genericupdate(resource_id, ...)while keepingcreate/get/delete.- The new
update(resource_data: ResourceData)(sync and async) correctly issues a PUT toself.path(i.e.,/accounts/{account_id}/users/{user_id}/groups) with the provided list of group IDs and materializes anAccountsUserGroupvia_model_class.from_response, which agrees with the unit and e2e tests.Only nit: the docstrings say “Account User Group” singular; if the API concept is plural (“user groups” for a user), you might tweak the wording for clarity, but behavior-wise this looks solid.
Also applies to: 24-65
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
171-175: Based on my verification, I've found a critical issue with the original review comment. Let me provide the rewritten version:
Consider making
test_account_user_group_postsynchronous for code clarity
test_account_user_group_postis declaredasyncbut contains noawaitstatements. While this is not currently a linting error (RUF029 requires preview mode, which is not enabled inruff.toml), the async keyword is unnecessary here.Since pytest-asyncio resolves async fixtures before passing them to test functions—whether sync or async—the
created_account_user_groupparameter arrives as a concrete value, not a coroutine. You can safely make this a regular sync test:-async def test_account_user_group_post(created_account_user_group): +def test_account_user_group_post(created_account_user_group): """Test creating an account user group.""" created_account_user_group_data = created_account_user_group assert created_account_user_group_data is not NoneThis improves readability by signaling that no async operations occur in the test body, while maintaining identical behavior.
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (5)
9-37: Tighten fixture teardown for multiple account-user creations
created_account_useronly tracks the last created user inret_account_user. If a test ever callscreated_account_user()more than once, earlier users won’t be deleted in teardown and will accumulate in the environment.Consider tracking all created users and deleting them all in teardown, e.g.:
- ret_account_user = None + ret_account_users: list = [] 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_users account_user_data = account_user_factory( first_name=first_name, last_name=last_name, ) - ret_account_user = mpt_vendor.accounts.accounts.users(account_id=account_id).create( - account_user_data - ) - return ret_account_user + created = mpt_vendor.accounts.accounts.users(account_id=account_id).create( + account_user_data + ) + ret_account_users.append(created) + return created - if ret_account_user: - try: - mpt_vendor.accounts.accounts.users(account_id=account_id).delete(ret_account_user.id) - except MPTAPIError: - print( - f"TEARDOWN - Unable to delete account user {ret_account_user.id}", - ) + for user in ret_account_users: + try: + mpt_vendor.accounts.accounts.users(account_id=account_id).delete(user.id) + except MPTAPIError: + print(f"TEARDOWN - Unable to delete account user {user.id}")
75-122: Account-user CRUD and error tests look correct; consider slightly stronger assertionsThe tests for
get,list,404on invalid ID, filtering, create, and delete exercise the expected client methods and basic behaviours and should give good coverage of the happy path plus a 404.If you want to tighten them further (optional), you could:
- In
test_get_account_user_by_id, assert on a stable field (e.g.account_user.id == user_id).- In
test_filter_account_users, also assert that the single result’sidmatchesuser_id.These would catch subtle regressions in route wiring or RQL handling, not just “non-empty response”.
125-155: Update and resend-invite tests: verify state changes, not just non-None(optional)
test_update_account_userandtest_account_user_resend_invitecurrently only assert that a response object is returned. This confirms “no exception”, but not that:
- The user’s name was actually updated.
- The resend-invite operation behaved as expected (e.g. status, timestamp, or some flag changed).
If the API surface exposes stable fields for these operations, consider asserting on them (e.g.,
updated_account_user.user.firstName == "E2E Updated").
157-196: Account-user-group tests are valid; you might also assert effective membership (optional)The group POST/PUT/DELETE tests exercise the correct chained service (
accounts.accounts.users(...).groups(...)) and basic success via non-Noneor no exception, which is fine for smoke e2e coverage.If the client exposes a way to read back a user’s groups efficiently, you could optionally:
- After
update, verify that the returned object (or a follow-upget) reflects the expected group list.- After
delete, assert that the group is no longer present in the user’s group list.This would turn them from “call doesn’t error” into true behavioural checks.
14-14: Remove or update unused/unknownnoqadirectives flagged by RuffRuff is reporting
RUF100for the# noqa: WPS430/WPS420/WPS421comments here, meaning these codes are unknown under the current lint configuration and the directives are effectively invalid.To keep lint green, either:
- Remove these
noqacomments entirely, or- Replace them with codes that are actually enabled in your config (if you still want to suppress something specific).
Given the current hints, simply dropping them is likely the simplest fix.
Also applies to: 19-19, 35-35, 51-51, 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)setup.cfg(1 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/accounts/accounts_users/test_async_accounts_users.py
🧬 Code graph analysis (7)
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
mpt_api_client/http/mixins.py (6)
AsyncCreatableResourceMixin(631-634)CreatableResourceMixin(627-628)update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/accounts/account.py (2)
update(84-111)update(161-188)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
mpt_api_client/http/mixins.py (4)
update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(32-43)update(54-65)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (5)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)async_mpt_ops(36-39)tests/e2e/accounts/accounts_users/conftest.py (1)
account_user_factory(10-36)tests/e2e/accounts/conftest.py (1)
account_id(22-23)mpt_api_client/models/model.py (1)
id(119-121)
mpt_api_client/http/mixins.py (1)
mpt_api_client/models/model.py (1)
Model(65-125)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
mpt_vendor(19-20)mpt_ops(31-32)tests/e2e/accounts/accounts_users/conftest.py (1)
account_user_factory(10-36)tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_factory(47-62)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (3)
AsyncCreatableResourceMixin(631-634)AsyncCreateWithIconMixin(289-319)CreatableResourceMixin(627-628)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[error] 171-171: docker compose run --service-ports app_test failed. Ruff: RUF029 Function test_account_user_group_post is declared async, but doesn't await or use async features.
🪛 Ruff (0.14.6)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
37-37: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
14-14: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
35-35: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
51-51: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
72-72: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
1089-1089: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
1106-1106: Unused noqa directive (unknown: WPS431)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (4)
setup.cfg (1)
52-56: Per-file ignore for accounts_users e2e tests is consistentThe added
tests/e2e/accounts/accounts_users/*.pyWPS ignores match the existing pattern for other e2e modules; no issues from a tooling or structure standpoint.e2e_config.test.json (1)
10-14: Newaccounts.user.idconfig entry aligns with testsThe added
"accounts.user.id"entry is well-formed JSON and matches the naming scheme used by the existing account-related IDs; suitable for theuser_idfixture.tests/unit/resources/accounts/test_accounts_user_groups.py (1)
47-118: Accounts user-groups tests correctly reflect new update API
- Restricting the mixin presence checks to
["create", "delete"]matches the move toCreatableResourceMixin(noupdate(resource_id, ...)).- The new sync/async update tests assert the correct PUT URL (
.../accounts/ACC-0000-0001/users/{user_id}/groups) and a JSON body consistent withhttpx’s encoding, and validate the model viato_dict().This gives good coverage of the new
update(resource_data)methods.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
40-73: LGTM on user-group and account-user-group fixtures
created_user_groupandcreated_account_user_groupcorrectly encapsulate setup/teardown around the external API, and the teardown paths are defensive against 4xx/5xx viaMPTAPIErrorhandling. The dependency chain (created_account_user_group→created_account_user&created_user_group) is also sound from a teardown-order perspective.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
1ce723a to
10794fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
67-67: Remove unusednoqadirective to fix pipeline.Ruff flags
# noqa: AAA01as unknown since AAA (pytest-aaa) rules aren't recognized by Ruff.Apply this diff:
-def test_account_user_groups_update(accounts_user_groups_service): # noqa: AAA01 +def test_account_user_groups_update(accounts_user_groups_service):tests/e2e/accounts/accounts_users/conftest.py (1)
9-36: Remove unusednoqadirective.Ruff doesn't recognize
WPS430and flags this as RUF100. Since the project uses Ruff for linting, this noqa is ineffective and should be removed.Apply this diff:
- def _account_user( # noqa: WPS430 + def _account_user(Otherwise, the factory fixture logic looks good—it correctly generates unique emails when not provided and builds appropriate account user payloads.
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
96-100: Add blank line before Act block.The pipeline reports AAA03: expected 1 blank line before Act block. Add a blank line before the
with pytest.raisesstatement:def test_get_account_user_by_id_not_found(mpt_vendor, invalid_user_id, account_id): """Test retrieving an account user by invalid ID.""" users = mpt_vendor.accounts.accounts.users(account_id=account_id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): users.get(invalid_user_id)
20-43: Remove WPS/AAAnoqadirectives unknown to Ruff.Ruff flags the WPS and AAA
noqadirectives as unknown (RUF100 violations), contributing to the WPS402 "noqa overuse" failure (15 total). These are wemake-python-styleguide codes not recognized by Ruff.Remove noqa directives on lines 20, 25, 41, 57, 76, 79, 86, 103, 114, 120, 127, 147, 159, 165, 181:
- def _created_account_user( # noqa: WPS430 + 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 ... -def test_get_account_user_by_id(mpt_vendor, user_id, account_id): # noqa: AAA01 +def test_get_account_user_by_id(users, user_id):Using the
usersfixture (as suggested in the previous comment) will eliminate most AAA01 directives naturally.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
13-14: Unusedusersfixture doesn't resolve WPS204 violations.The
usersfixture was created to address the WPS204 "overused expression" violation (line 14 in pipeline failure), but it's never actually used in the tests. All test functions still directly callasync_mpt_vendor.accounts.accounts.users(account_id=account_id).To fix the pipeline failure, replace the repeated expression throughout the file with the
usersfixture parameter. For example:async def test_get_account_user_by_id(async_mpt_vendor, user_id, account_id): """Test retrieving an account user by ID.""" - users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) +async def test_get_account_user_by_id(users, user_id): + """Test retrieving an account user by ID.""" account_user = await users.get(user_id) assert account_user is not NoneApply this pattern to all test functions and fixtures that currently repeat the expression.
22-45: Remove WPS/AAAnoqadirectives unknown to Ruff.Ruff flags the
# noqa: WPS430,# noqa: WPS420, and# noqa: WPS421directives as unknown (RUF100 violations). These are wemake-python-styleguide codes not recognized by Ruff.Remove lines 22, 27, and 43's noqa directives:
- async def _created_account_user( # noqa: WPS430 + 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 ... except MPTAPIError: - print( # noqa: WPS421 + print(Similarly remove WPS421 and AAA01 noqa directives throughout the file (lines 61, 82, 165).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/accounts/accounts_users/test_async_accounts_users.pytests/e2e/accounts/accounts_users/conftest.pytests/e2e/conftest.py
🧬 Code graph analysis (3)
mpt_api_client/resources/accounts/accounts_user_groups.py (1)
mpt_api_client/http/mixins.py (6)
AsyncCreateMixin(187-198)AsyncDeleteMixin(201-211)AsyncGetMixin(379-394)CreateMixin(16-27)DeleteMixin(30-39)GetMixin(360-376)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[error] 14-14: WPS204 Found overused expression: async_mpt_vendor.accounts.accounts.users(account_id=account_id); return async_mpt_vendor.accounts.accounts.users(account_id=account_id)
[error] 39-39: WPS229 Found too long 'try' body length: 2 > 1
[error] 78-78: WPS229 Found too long 'try' body length: 2 > 1
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
[error] WPS402 Found noqa comments overuse: 15
[error] 12-12: WPS204 Found overused expression: mpt_vendor.accounts.accounts.users(account_id=account_id); return mpt_vendor.accounts.accounts.users(account_id=account_id)
[error] 37-37: WPS229 Found too long 'try' body length: 2 > 1
[error] 72-72: WPS229 Found too long 'try' body length: 2 > 1
[error] 99-99: AAA03 expected 1 blank line before Act block, found none
🪛 Ruff (0.14.6)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
22-22: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
27-27: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
43-43: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
61-61: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
82-82: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_accounts_user_groups.py
67-67: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
20-20: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
25-25: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
41-41: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
57-57: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
76-76: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
79-79: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
86-86: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
103-103: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
114-114: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
120-120: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
127-127: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
147-147: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
159-159: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
181-181: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (13)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
1-5: LGTM! HTTP testing dependencies properly added.The imports for
json,httpx, andrespxare appropriately added to support the new HTTP mocking functionality in the update tests.
94-118: LGTM! Async update test properly implemented.The async test correctly mirrors the synchronous version, properly awaiting the async operations and verifying both request content and response data.
mpt_api_client/resources/accounts/accounts_user_groups.py (3)
2-13: LGTM! Mixin decomposition improves modularity.The replacement of composite mixins (ManagedResourceMixin, AsyncManagedResourceMixin) with granular mixins (GetMixin, CreateMixin, DeleteMixin, and their async equivalents) provides better clarity and follows the single responsibility principle. The ResourceData import is correctly added for the new update methods.
28-49: LGTM! Update method correctly implements PUT without resource ID.The synchronous update method properly performs a PUT request to
self.path(without appending a resource ID), which is correct for updating account user groups in bulk. The implementation aligns with the service patterns shown in the relevant code snippets.
52-73: LGTM! Async update method correctly mirrors synchronous version.The asynchronous update method properly awaits the HTTP request and follows the same pattern as the synchronous version, ensuring consistency across the API surface.
e2e_config.test.json (1)
12-12: LGTM! Configuration entry added for e2e tests.The
accounts.user.idconfiguration is appropriately added to support the new account user e2e tests.tests/e2e/conftest.py (2)
105-107: LGTM! UUID fixture provides full unique identifiers.The
uuid_strfixture correctly returns a full UUID string, complementing the existingshort_uuidfixture for cases requiring complete unique identifiers (like email addresses in the account user tests).
116-118: LGTM! User ID fixture properly derives from configuration.The
user_idfixture appropriately extracts the user ID from the e2e configuration, following the established pattern for other ID fixtures in this file.tests/e2e/accounts/accounts_users/conftest.py (1)
4-6: LGTM! Invalid user ID fixture for negative testing.The
invalid_user_idfixture appropriately provides a dummy user ID for testing 404 scenarios.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
85-163: LGTM! Comprehensive async e2e test coverage.The test functions properly cover the account user lifecycle: retrieval, listing, filtering, creation, deletion, updates, and invite resend. The async patterns and assertions are correct.
165-168: Synchronous test with async fixture is correct per pytest-asyncio.Based on learnings, pytest-asyncio automatically resolves async fixtures before passing them to test functions. The synchronous test definition is intentional and correct—no async/await needed here.
Based on learnings, pytest-asyncio handles async fixture resolution.
171-198: LGTM! Account user group tests complete the e2e coverage.The update and delete tests for account user groups properly exercise the new update methods added to the service layer.
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
79-192: LGTM! Comprehensive synchronous e2e test coverage.The test functions properly mirror the async version, covering the complete account user lifecycle with appropriate synchronous calls. Once the pipeline issues are addressed, these tests will provide solid coverage.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
10794fb to
ad87439
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (4)
95-99: Add blank line before Act block.The pipeline reports AAA03 violation: expected 1 blank line before Act block. Add a blank line before the
with pytest.raisesstatement to satisfy the pytest-aaa linter.Apply this diff:
def test_get_account_user_by_id_not_found(mpt_vendor, invalid_user_id, account_id): """Test retrieving an account user by invalid ID.""" users = mpt_vendor.accounts.accounts.users(account_id=account_id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): users.get(invalid_user_id)
35-42: Shorten try-body to fix WPS229 violation.The teardown block has a try body with 2 statements (lines 37-38), exceeding the WPS229 limit of 1. Extract the service instantiation before the try block.
Apply this diff:
if ret_account_user: + users = mpt_vendor.accounts.accounts.users(account_id=account_id) try: - users = mpt_vendor.accounts.accounts.users(account_id=account_id) users.delete(ret_account_user.id) except MPTAPIError:
19-19: Remove unused noqa directive.Ruff doesn't recognize
WPS430and flags this as RUF100. Remove the noqa comment.Apply this diff:
- def _created_account_user( # noqa: WPS430 + def _created_account_user( first_name: str = "E2E Created", last_name: str = "Account User",
71-75: Shorten try-body to fix WPS229 violation.The teardown block has a try body with 2 statements (lines 72-73), exceeding the WPS229 limit of 1. Extract the service instantiation before the try block.
Apply this diff:
yield created_account_user_group + users = mpt_vendor.accounts.accounts.users(account_id=account_id) try: - users = mpt_vendor.accounts.accounts.users(account_id=account_id) users.groups(user_id=created_account_user_data.id).delete(user_group_data.id) except MPTAPIError as error:tests/e2e/accounts/accounts_users/conftest.py (1)
11-11: Remove unused noqa directive.Ruff doesn't recognize
WPS430and flags this as RUF100. Remove the noqa comment to fix the linting violation.Apply this diff:
- 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",tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
78-84: Shorten try-body to fix WPS229 violation.The teardown block has a try body with 2 statements (lines 79-80), exceeding the WPS229 limit of 1. Extract the service instantiation before the try block.
Apply this diff:
yield created_account_user_group + users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) try: - users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) await users.groups(user_id=created_account_user_data.id).delete(user_group_data.id) except MPTAPIError as error:
37-45: Shorten try-body to fix WPS229 violation.The teardown block has a try body with 2 statements (lines 39-40), exceeding the WPS229 limit of 1. Extract the service instantiation before the try block.
Apply this diff:
if ret_account_user: + users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) try: - users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) await users.delete(ret_account_user.id) except MPTAPIError as error:
26-26: Remove unused noqa directive.Ruff doesn't recognize
WPS420and flags this as RUF100. Remove the noqa comment.Apply this diff:
- nonlocal ret_account_user # noqa: WPS420 + nonlocal ret_account_usertests/unit/resources/accounts/test_accounts_user_groups.py (1)
65-118: Remove invalidnoqaand simplify request body assertionsTwo points on the new sync/async update tests:
- The
# noqa: AAA01on the sync test function is still present and Ruff flags it as an unknown/unused directive (RUF100), which will keep CI red. Please drop the directive as previously suggested:-def test_account_user_groups_update(accounts_user_groups_service): # noqa: AAA01 +def test_account_user_groups_update(accounts_user_groups_service):
- The request body assertion currently relies on matching pre‑dumped JSON bytes exactly. To make the tests less brittle with respect to JSON serialization details and avoid an extra
request_expected_contentvariable, you can compare parsed JSON structures instead, e.g.:- input_data = [{"id": group_id}] - request_expected_content = json.dumps(input_data, separators=(",", ":")).encode() - response_expected_data = {"id": group_id, "name": "Test Group"} + input_data = [{"id": group_id}] + response_expected_data = {"id": group_id, "name": "Test Group"} @@ - updated_group = accounts_user_groups_service.update(input_data) - - assert mock_route.call_count == 1 - request = mock_route.calls[0].request - assert request.content == request_expected_content - assert updated_group.to_dict() == response_expected_data + updated_group = accounts_user_groups_service.update(input_data) + + assert mock_route.call_count == 1 + request = mock_route.calls[0].request + assert json.loads(request.content) == input_data + assert updated_group.to_dict() == response_expected_dataand similarly for the async test:
- input_data = [{"id": group_id}] - request_expected_content = json.dumps(input_data, separators=(",", ":")).encode() - response_expected_data = {"id": group_id, "name": "Test Group"} + input_data = [{"id": group_id}] + response_expected_data = {"id": group_id, "name": "Test Group"} @@ - updated_group = await async_accounts_user_groups_service.update(input_data) - - assert mock_route.call_count == 1 - request = mock_route.calls[0].request - assert request.content == request_expected_content - assert updated_group.to_dict() == response_expected_data + updated_group = await async_accounts_user_groups_service.update(input_data) + + assert mock_route.call_count == 1 + request = mock_route.calls[0].request + assert json.loads(request.content) == input_data + assert updated_group.to_dict() == response_expected_dataOptionally, you could also DRY up the nearly identical sync/async tests via parametrization or a small helper, but that’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/conftest.py
- e2e_config.test.json
- mpt_api_client/resources/accounts/accounts_user_groups.py
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/accounts/accounts_users/test_async_accounts_users.pytests/e2e/accounts/accounts_users/test_sync_accounts_users.pytests/e2e/accounts/accounts_users/conftest.py
🧬 Code graph analysis (3)
tests/e2e/accounts/accounts_users/test_async_accounts_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 (3)
async_mpt_vendor(24-27)async_mpt_ops(36-39)user_id(117-118)tests/e2e/accounts/conftest.py (1)
account_id(22-23)tests/e2e/accounts/accounts_users/conftest.py (2)
account_user_factory(10-36)invalid_user_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
tests/unit/resources/accounts/test_accounts_user_groups.py (3)
tests/unit/resources/accounts/test_account_user_groups.py (1)
test_async_mixins_present(55-58)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)tests/unit/http/test_mixins.py (2)
update(74-88)update(114-128)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[warning] 13-13: WPS204 Found overused expression: async_mpt_vendor.accounts.accounts.users(account_id=account_id); used 14 > 7
[warning] 38-38: WPS229 Found too long try body length: 2 > 1
[warning] 42-42: WPS421 Found wrong function call: print
[warning] 61-61: WPS421 Found wrong function call: print
[warning] 78-78: WPS229 Found too long try body length: 2 > 1
[warning] 82-82: WPS421 Found wrong function call: print
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
[warning] 1-1: WPS402 Found noqa comments overuse: 15
[warning] 11-11: WPS204 Found overused expression: mpt_vendor.accounts.accounts.users(account_id=account_id); used 14 > 7
[warning] 36-36: WPS229 Found too long try body length: 2 > 1
[warning] 71-71: WPS229 Found too long try body length: 2 > 1
[warning] 98-98: AAA03 expected 1 blank line before Act block, found none
🪛 Ruff (0.14.6)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
1-1: Unused noqa directive (unknown: WPS204, WPS402, AAA01)
Remove unused noqa directive
(RUF100)
21-21: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
26-26: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
167-167: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
19-19: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
24-24: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
40-40: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
56-56: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
75-75: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
78-78: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
85-85: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
102-102: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
113-113: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
119-119: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
126-126: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
146-146: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
158-158: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
164-164: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
180-180: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_accounts_user_groups.py
67-67: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (2)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
1-5: Imports for JSON and HTTP mocking look appropriate
json,httpx, andrespximports are consistent with how the new sync/async update tests construct payloads, mock responses, and inspect requests; no issues here.
47-60: Mixin presence tests remain validThe parametrized
"create", "delete"checks for sync and async services still correctly validate the expected mixins onAccountsUserGroupsServiceandAsyncAccountsUserGroupsService; no change needed here.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
ad87439 to
713c697
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
67-67: Remove unusednoqadirective to fix pipeline.Ruff flags
# noqa: AAA01as unknown since AAA (pytest-aaa) rules aren't recognized by Ruff. This contributes to the CI failure.Apply this diff:
-def test_account_user_groups_update(accounts_user_groups_service): # noqa: AAA01 +def test_account_user_groups_update(accounts_user_groups_service):tests/e2e/accounts/accounts_users/conftest.py (1)
9-36: Remove unusednoqadirective.Line 11 has
# noqa: WPS430which Ruff doesn't recognize, causing an RUF100 violation.Apply this diff:
- def _account_user( # noqa: WPS430 + def _account_user(Otherwise, the factory correctly generates unique email addresses using
uuid_strand properly structures the account user payload.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (3)
35-42: Shorten try-body to fix WPS229 pipeline failure.The teardown try block contains 2 statements (lines 37-38), exceeding the WPS229 limit of 1.
Apply this diff:
if ret_account_user: + users = mpt_vendor.accounts.accounts.users(account_id=account_id) try: - users = mpt_vendor.accounts.accounts.users(account_id=account_id) users.delete(ret_account_user.id)This same issue occurs in the
created_account_user_groupfixture at lines 71-75.
95-99: Add blank line before Act block to fix AAA03 pipeline failure.The pytest-aaa linter requires a blank line separating the Arrange and Act blocks.
Apply this diff:
def test_get_account_user_by_id_not_found(mpt_vendor, invalid_user_id, account_id): """Test retrieving an account user by invalid ID.""" users = mpt_vendor.accounts.accounts.users(account_id=account_id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): users.get(invalid_user_id)
9-11: Theusersfixture is defined but never used, leaving WPS204 violation unresolved.The fixture was likely created to address the WPS204 "overused expression" violation, but all tests still directly call
mpt_vendor.accounts.accounts.users(account_id=account_id)(14 times) instead of injecting theusersfixture.To fix the WPS204 pipeline failure, inject the
usersfixture into tests. For example:-def test_get_account_user_by_id(mpt_vendor, user_id, account_id): +def test_get_account_user_by_id(users, user_id): """Test retrieving an account user by ID.""" - users = mpt_vendor.accounts.accounts.users(account_id=account_id) account_user = users.get(user_id)Apply this pattern to all test functions and fixtures, which will also eliminate most noqa comments and resolve the WPS402 violation.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
1-1: Remove module-levelnoqadirective to fix pipeline.The module-level
# noqa: WPS204, WPS402, AAA01contains codes Ruff doesn't recognize, triggering RUF100 violations and contributing to the WPS402 "noqa overuse" pipeline failure (11 total noqa comments).Apply this diff:
-# noqa: WPS204, WPS402, AAA01 - import pytestIf these suppressions are needed for another linter (flake8 with wemake-python-styleguide), configure Ruff to recognize external codes via the
externalrule category inpyproject.tomlinstead.
37-45: Shorten try-body to fix WPS229 pipeline failure.The teardown try block contains 2 statements (lines 39-40), exceeding the WPS229 limit of 1. Extract the service instantiation before the try block.
Apply this diff:
if ret_account_user: + users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) try: # noqa: WPS229 - users = async_mpt_vendor.accounts.accounts.users(account_id=account_id) await users.delete(ret_account_user.id)This same pattern appears in the
created_account_user_groupfixture at lines 78-84.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/conftest.pytests/e2e/accounts/accounts_users/conftest.pytests/e2e/accounts/accounts_users/test_async_accounts_users.pytests/e2e/accounts/accounts_users/test_sync_accounts_users.py
📚 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 (3)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (3)
async_mpt_vendor(24-27)async_mpt_ops(36-39)user_id(117-118)tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_factory(47-62)tests/e2e/accounts/accounts_users/conftest.py (2)
account_user_factory(10-36)invalid_user_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
[error] 1: WPS402 Found noqa comments overuse: 11
[error] 13-13: WPS204 Found overused expression: async_mpt_vendor.accounts.accounts.users(account_id=account_id); used 14 > 7
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
[error] 1: WPS402 Found noqa comments overuse: 15
[error] 11-12: WPS204 Found overused expression: mpt_vendor.accounts.accounts.users(account_id=account_id); used 14 > 7
[error] 36-36: WPS229 Found too long try body length: 2 > 1
[error] 71-71: WPS229 Found too long try body length: 2 > 1
[error] 98-98: AAA03 expected 1 blank line before Act block, found none
🪛 Ruff (0.14.6)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
1-1: Unused noqa directive (unknown: WPS204, WPS402, AAA01)
Remove unused noqa directive
(RUF100)
12-12: Unused noqa directive (unknown: WPS204)
Remove unused noqa directive
(RUF100)
21-21: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
26-26: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
38-38: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
42-42: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
58-58: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
61-61: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
78-78: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
82-82: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
167-167: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
19-19: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
24-24: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
40-40: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
56-56: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
75-75: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
78-78: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
85-85: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
102-102: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
113-113: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
119-119: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
126-126: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
146-146: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
158-158: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
164-164: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
180-180: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_accounts_user_groups.py
67-67: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (16)
e2e_config.test.json (1)
12-12: LGTM!The new configuration entry follows the established pattern and properly supports the new
user_idfixture added totests/e2e/conftest.py.tests/unit/resources/accounts/test_accounts_user_groups.py (4)
1-5: LGTM!The new imports (
jsonandrespx) properly support the added update test cases that mock HTTP requests.
67-91: LGTM!The synchronous update test correctly:
- Mocks the PUT request to the expected endpoint
- Verifies the request payload matches the input data
- Validates the returned model data
94-118: LGTM!The asynchronous update test properly mirrors the sync version and correctly handles async/await patterns.
47-64: Based on my verification, I can now provide the rewritten review comment:The parametrize change correctly removes tests for non-existent methods. The CollectionMixin does not provide
list()orsearch()methods—it providesfetch_page(),fetch_one(), anditerate(). The original parametrize testing for "list" and "search" was checking for methods that don't exist on the service. The reduction to["create", "delete"]correctly aligns the test with the actual mixin methods provided by CreateMixin and DeleteMixin. CollectionMixin functionality is adequately tested in generic mixin tests (tests/unit/http/test_mixins.py), not in this service-specific file.tests/e2e/conftest.py (2)
105-107: LGTM!The
uuid_strfixture provides unique identifiers for test data, supporting theaccount_user_factorywhich uses it to generate unique email addresses.
116-118: LGTM!The
user_idfixture properly sources the test user ID from the e2e configuration, enabling tests to reference an existing user.tests/e2e/accounts/accounts_users/conftest.py (1)
4-6: LGTM!The
invalid_user_idfixture provides a consistent invalid ID for negative test cases.tests/e2e/accounts/accounts_users/test_async_accounts_users.py (2)
17-34: LGTM!The
created_account_userasync fixture factory correctly:
- Accepts custom first/last names for test flexibility
- Uses
nonlocalto track the created user for teardown- Handles cleanup with error suppression for idempotent test runs
87-165: LGTM!The test suite comprehensively covers the account user lifecycle:
- GET by ID (valid and invalid)
- List with pagination
- Filter with RQL
- Create, update, delete
- Resend invite
All tests follow appropriate async patterns and error handling.
mpt_api_client/resources/accounts/accounts_user_groups.py (4)
2-11: LGTM!The refactoring from composite
ManagedResourceMixinto granular mixins (GetMixin,CreateMixin,DeleteMixin) improves modularity and makes the service capabilities explicit.
13-13: LGTM!The
ResourceDataimport supports the newupdatemethods' type hints.
28-49: LGTM!The synchronous
updatemethod correctly:
- Accepts
ResourceDatafor flexible payload structure- Performs PUT to
self.path- Returns a properly typed
AccountsUserGroupmodel
52-73: LGTM!The async
updatemethod properly mirrors the sync implementation with correct async/await patterns.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
14-33: LGTM!The
created_account_userfixture factory correctly manages the account user lifecycle with proper teardown and error handling.
78-191: LGTM!The synchronous test suite provides comprehensive coverage of account user operations, properly mirroring the async test suite's functionality.
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
8 times, most recently
from
a5bfccc to
b1cf33f
Compare
albertsola
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
b1cf33f to
201846a
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
2 times, most recently
from
91d3c3d to
ba52dd4
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
2 times, most recently
from
35ef8e2 to
991cfbc
Compare
robcsegal
force-pushed
the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
991cfbc to
92ce69a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
67-67: Remove remaining# noqa: AAA01to satisfy Ruff and keep linting cleanLine 67 still has
# noqa: AAA01, which Ruff treats as an unknown/unused noqa (RUF100). Given pytest-aaa isn’t wired into Ruff here, this directive buys nothing and just adds noise.You can safely drop it:
-def test_account_user_groups_update(accounts_user_groups_service): # noqa: AAA01 +def test_account_user_groups_update(accounts_user_groups_service):tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
1-182: Noqa overuse and unknown codes are breaking CI (WPS402 / RUF100)Functionally these e2e tests and fixtures look good, but the current
# noqausage is causing lint failures from both wemake-python-styleguide and Ruff:
- Line 1:
# noqa: WPS402plus 17 other# noqa:lines triggersWPS402 Found "noqa" comments overuse: 18.- Lines 11, 12, 25, 41, 57, 61, 76, 79, 86, 104, 115, 121, 128, 148, 160, 166, 182: Ruff reports RUF100 because it doesn’t recognize
WPS204,WPS210,WPS420,WPS421, orAAA01.To unblock CI, you’ll need to reduce or eliminate these inline suppressions:
- Prefer fixing the underlying style issues (e.g., factoring the repeated
mpt_vendor.accounts.accounts.users(account_id=account_id)into the existingusersfixture, letting you drop severalWPS204noqas; limiting or acceptingnonlocal/- And remove the
# noqa: AAA01markers from test definitions unless pytest-aaa is integrated via Ruff’sexternalconfiguration; as-is they only produce RUF100.If you truly need WPS suppressions for another linter, an alternative is to move them into configuration (per-file-ignores for this path) and then delete the in-code
noqacomments so Ruff and WPS are no longer fighting over them.tests/e2e/accounts/accounts_users/conftest.py (1)
9-36: Drop# noqa: WPS430on_account_user(unused/unknown for Ruff)The
_account_userfactory looks correct and now generates unique emails, but the# noqa: WPS430on Line 11 is still flagged by Ruff as an unknown/unused directive (RUF100).You can safely remove it:
- def _account_user( # noqa: WPS430 + def _account_user( email: str | None = None, # Must be unique in MarketplaceIf wemake’s WPS430 is still enforced elsewhere, consider handling that via per-file-ignores in config instead of inline in tests.
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (1)
9-76: Clean up WPS-specificnoqacomments that Ruff flags as unused (RUF100)In this async users e2e module, Ruff still reports RUF100 on several WPS-specific
noqacomments:
- Line 11:
# noqa: WPS204- Line 24:
# noqa: WPS420- Lines 40, 56, 75:
# noqa: WPS421- Line 60:
# noqa: WPS210These codes aren’t known to Ruff, so the suppressions don’t actually silence any Ruff rules and just produce RUF100.
Options:
- Preferably, drop these inline
noqacomments and either:
- adjust the code slightly to satisfy the underlying WPS rules where they still run (e.g., accept
nonlocalin tests, or move those exceptions to per-file ignores in your wemake config), or- configure Ruff’s
externallinting if you truly need to keep WPS-style codes inline (sonoqalines map to real Ruff diagnostics instead of being “unused”).Right now they only hurt lint signal and contribute to CI noise.
🧹 Nitpick comments (1)
mpt_api_client/resources/accounts/accounts_user_groups.py (1)
13-73: Broadenupdateparameter type to match actual usage (list payload)Both sync and async
updatemethods currently declare:def update(self, resource_data: ResourceData) -> AccountsUserGroup:but the tests and e2e code pass a list of group dicts (e.g.
[{ "id": group_id }]). Elsewhere (e.g.,Service._resource_do_request/AsyncService._resource_do_request) the JSON payload is typed asResourceData | ResourceList, so the current hint is inconsistent with actual usage.Consider widening the type to reflect the supported payload shape:
-from mpt_api_client.models.model import ResourceData +from mpt_api_client.models.model import ResourceData, ResourceList @@ - def update(self, resource_data: ResourceData) -> AccountsUserGroup: + def update(self, resource_data: ResourceData | ResourceList) -> AccountsUserGroup: @@ - async def update(self, resource_data: ResourceData) -> AccountsUserGroup: + async def update(self, resource_data: ResourceData | ResourceList) -> AccountsUserGroup:This keeps runtime behavior identical while making the public API’s type hints align with how it’s actually used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/accounts_user_groups.py(2 hunks)tests/e2e/accounts/accounts_users/conftest.py(1 hunks)tests/e2e/accounts/accounts_users/test_async_accounts_users.py(1 hunks)tests/e2e/accounts/accounts_users/test_sync_accounts_users.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/unit/resources/accounts/test_accounts_user_groups.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/accounts/accounts_users/conftest.pytests/e2e/accounts/accounts_users/test_async_accounts_users.pytests/e2e/accounts/accounts_users/test_sync_accounts_users.py
📚 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)
mpt_api_client/resources/accounts/accounts_user_groups.py (4)
mpt_api_client/http/mixins.py (7)
CreateMixin(16-27)DeleteMixin(30-39)GetMixin(360-376)update(45-56)update(152-184)update(217-228)update(325-357)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
tests/unit/resources/accounts/test_account_user_groups.py (1)
test_async_mixins_present(55-58)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)
tests/e2e/accounts/accounts_users/conftest.py (2)
tests/e2e/accounts/conftest.py (2)
account_id(22-23)user_group_id(37-38)tests/e2e/conftest.py (1)
uuid_str(106-107)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (4)
tests/e2e/conftest.py (3)
mpt_vendor(19-20)mpt_ops(31-32)user_id(117-118)tests/e2e/accounts/conftest.py (1)
account_id(22-23)tests/e2e/accounts/accounts_users/conftest.py (2)
account_user_factory(10-36)invalid_user_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/accounts_users/test_sync_accounts_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)
account_id(22-23)user_group_factory(47-62)tests/e2e/accounts/accounts_users/conftest.py (1)
account_user_factory(10-36)mpt_api_client/resources/accounts/accounts_user_groups.py (2)
update(38-49)update(62-73)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
[error] 1-1: wemake-python-styleguide WPS402: Found noqa comments overuse: 18. Failing due to CI step: 'docker compose run --service-ports app_test'.
🪛 Ruff (0.14.6)
tests/unit/resources/accounts/test_accounts_user_groups.py
67-67: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/conftest.py
11-11: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py
11-11: Unused noqa directive (unknown: WPS204)
Remove unused noqa directive
(RUF100)
24-24: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
40-40: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
56-56: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
60-60: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
75-75: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
1-1: Unused noqa directive (unknown: WPS402)
Remove unused noqa directive
(RUF100)
11-11: Unused noqa directive (unknown: WPS204)
Remove unused noqa directive
(RUF100)
12-12: Unused noqa directive (unknown: WPS204)
Remove unused noqa directive
(RUF100)
25-25: Unused noqa directive (unknown: WPS420)
Remove unused noqa directive
(RUF100)
41-41: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
57-57: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
61-61: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
76-76: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
79-79: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
86-86: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
104-104: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
115-115: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
121-121: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
128-128: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
148-148: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
160-160: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
166-166: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
182-182: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (1)
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
94-118: Async update test mirrors the service contract correctlyThe async update test validates URL, JSON payload, and model deserialization in line with
AsyncAccountsUserGroupsService.update. This gives good coverage of the async path and looks solid.
|
robcsegal
deleted the
MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
Added Accounts account by id users endpoints e2e tests
https://softwareone.atlassian.net/browse/MPT-14865
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.