MPT-14865 add E2E tests for accounts users endpoints (rebased) #135
Conversation
WalkthroughThis PR adds account user management capabilities by implementing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Found Jira issue key in the title: MPT-14865 |
albertsola
force-pushed
the
asola/MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
3 times, most recently
from
48a507c to
1f818ed
Compare
albertsola
changed the title
albertsola
force-pushed
the
asola/MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
from
1f818ed to
5e7e3c2
Compare
|
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
albertsola
changed the title
albertsola
changed the title
|
Based on this PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (2)
11-11: Remove unused noqa directives.Static analysis indicates these
noqadirectives are unused. The linter codes (WPS204, WPS420, WPS421, WPS210) are not recognized by the current tooling.Apply this diff to clean up the unused directives:
- return mpt_vendor.accounts.accounts.users(account_id=account_id) # noqa: WPS204 + return mpt_vendor.accounts.accounts.users(account_id=account_id) 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, ) users_obj = mpt_vendor.accounts.accounts.users(account_id=account_id) ret_account_user = users_obj.create(account_user_data) return ret_account_user yield _created_account_user if ret_account_user: users_obj = mpt_vendor.accounts.accounts.users(account_id=account_id) try: users_obj.delete(ret_account_user.id) except MPTAPIError: - print( # noqa: WPS421 + print( f"TEARDOWN - Unable to delete account user {ret_account_user.id}", )Apply similar changes to lines 56, 60, and 75.
Also applies to: 24-24, 40-40, 56-56, 60-60, 75-75
35-42: Improve teardown error logging for debugging.The teardown logic catches
MPTAPIErrorbut doesn't log error details, making it harder to diagnose cleanup failures.Apply this diff to include error details:
if ret_account_user: users_obj = mpt_vendor.accounts.accounts.users(account_id=account_id) try: users_obj.delete(ret_account_user.id) - except MPTAPIError: - print( # noqa: WPS421 - f"TEARDOWN - Unable to delete account user {ret_account_user.id}", + except MPTAPIError as error: + print( + f"TEARDOWN - Unable to delete account user {ret_account_user.id}: " + f"{getattr(error, 'title', str(error))}" )Apply similar pattern to other teardown blocks at lines 53-56 and 72-75.
tests/unit/resources/accounts/test_accounts_user_groups.py (1)
67-67: Remove unused noqa directive.The
noqa: AAA01directive is not recognized by the current linting tooling.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): """Test updating account user groups."""tests/e2e/accounts/accounts_users/conftest.py (1)
11-11: Remove unused noqa directive.The
noqa: WPS430directive is not recognized by the current linting tooling.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 (1)
11-11: Remove unused noqa directives.Static analysis indicates these
noqadirectives are unused. The linter codes (WPS204, WPS420, WPS421, WPS210) are not recognized by the current tooling.Apply similar cleanup as suggested for the sync test file.
Also applies to: 24-24, 40-40, 59-59, 63-63, 80-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/pr-build-merge.yml(1 hunks)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
🧬 Code graph analysis (4)
mpt_api_client/resources/accounts/accounts_user_groups.py (4)
mpt_api_client/resources/accounts/licensees.py (2)
update(73-99)update(141-167)mpt_api_client/resources/accounts/buyers.py (2)
update(82-108)update(170-196)mpt_api_client/resources/accounts/account.py (2)
update(84-111)update(161-188)mpt_api_client/resources/catalog/products.py (2)
update(103-129)update(215-241)
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)
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (7)
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 (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)
🪛 GitHub Actions: PR build and merge
tests/e2e/accounts/accounts_users/test_sync_accounts_users.py
[error] 1-1: HTTP Timeout during POST to create account user in e2e tests. The request to '/public/v1/accounts/ACC-9042-0088/users' timed out (ReadTimeout). This caused test 'test_account_user_group_post' to fail.
🪛 Ruff (0.14.6)
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
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_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)
59-59: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
63-63: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
80-80: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
166-166: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (13)
.github/workflows/pr-build-merge.yml (1)
44-49: Verify if disabling ReportPortal is intentional.The ReportPortal integration has been disabled by commenting out the original pytest command. However, the environment variables (RP_LAUNCH, RP_ENDPOINT, RP_API_KEY) are still defined but no longer used.
Is this a temporary change, or should the unused environment variables also be removed?
e2e_config.test.json (1)
12-12: LGTM!The new configuration key for
accounts.user.idis correctly formatted and aligns with the test requirements.tests/e2e/conftest.py (1)
105-107: LGTM!The new fixtures follow established patterns and provide necessary test infrastructure for account user tests.
Also applies to: 116-118
tests/unit/resources/accounts/test_accounts_user_groups.py (2)
47-64: LGTM!Removing "update" from the parametrized mixin tests is correct since the
update()method is now explicitly implemented rather than inherited from a mixin.
67-91: LGTM!Both sync and async update tests are well-structured with:
- Proper request mocking
- Validation of request content
- Assertion of response mapping
The tests provide good coverage of the new update functionality.
Also applies to: 94-118
mpt_api_client/resources/accounts/accounts_user_groups.py (2)
28-34: LGTM!The refactoring from
ManagedResourceMixinto explicitGetMixin,CreateMixin, andDeleteMixinimproves clarity and aligns with the customupdate()implementation.
38-49: Perfect. I now have comprehensive evidence. The review comment is accurate and raises valid concerns. Let me generate the rewritten review comment:Verify the update() method signature and API design — the endpoint uses a collection path with list payload instead of the standard resource_id pattern.
The
update()method has an unusual signature compared to other update methods in the codebase. This implementation operates on the entire user's group association list (PUT to/accounts/{account_id}/users/{user_id}/groupswith a list payload[{"id": ...}]), whereas standard update methods take a resource_id and operate on individual items (PUT to{self.path}/{resource_id}).Verify that:
- This bulk-replace API design is intentional and documented in the MPT API specification
- The response handling correctly processes the API's response format (currently
from_response()expects{"data": {...}}dict)- The return type annotation
AccountsUserGroup(singular) accurately reflects what the API returns when given a list of groupsApplies also to: Line 62 (async variant)
tests/e2e/accounts/accounts_users/conftest.py (2)
4-6: LGTM!The
invalid_user_idfixture provides a consistent invalid ID for negative test cases.
9-36: LGTM!The
account_user_factoryfixture is well-designed with:
- Unique email generation using
uuid_strto avoid conflicts- Sensible defaults for names
- Proper structure matching the API requirements
tests/e2e/accounts/accounts_users/test_async_accounts_users.py (3)
35-43: LGTM!The error handling in teardown fixtures properly logs error details using
getattr(error, 'title', str(error)), which aids debugging. This pattern is better than the sync version.Also applies to: 56-59, 76-82
85-199: LGTM!The async E2E tests provide comprehensive coverage of account user operations:
- CRUD operations (create, get, update, delete)
- Filtering and pagination
- Error handling (not found cases)
- User group associations
- Invite resend functionality
The tests are well-structured with appropriate assertions and follow async/await patterns correctly (aside from the issue noted in
test_account_user_group_post).
166-169: Add missing async keyword to test function.The test function
test_account_user_group_postuses the async fixturecreated_account_user_groupbut is not declared asasync. This will cause the test to fail because it receives a coroutine instead of the yielded value.Apply this diff:
-def test_account_user_group_post(created_account_user_group): # noqa: AAA01 +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 + created_account_user_group_data = await created_account_user_group assert created_account_user_group_data is not NoneNote: Also remove the unused
noqa: AAA01directive.⛔ Skipped due to learnings
Learnt from: robcsegal Repo: softwareone-platform/mpt-api-python-client PR: 131 File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0 Timestamp: 2025-11-27T00:05:54.691Z Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.tests/e2e/accounts/accounts_users/test_sync_accounts_users.py (1)
168-173: The review comment lacks concrete evidence and makes speculative claims about a timeout issue.The comment references a "pipeline failure" with a specific endpoint and account ID (
ACC-9042-0088), but provides no link to CI logs or concrete evidence. Analysis reveals:
No timeout configuration in code: The codebase has no pytest timeout decorators, retry mechanisms, or per-test timeout settings. The CI workflow has a 20-minute job-level timeout (
.github/workflows/cron-main-e2e.ymlandpr-build-merge.yml), which is standard.Test is trivial: The test at lines 168–173 simply asserts the fixture result is not None. Any timeout would occur in the
created_account_user_groupfixture's API call, not in the test body itself.Runtime data, not code issue: The specific account ID in the comment (
ACC-9042-0088) is test runtime data, not something that exists in the codebase and cannot be verified through code analysis.Speculative diagnosis: The suggested causes (slow API, network issues, rate limits) are conjecture without supporting evidence from logs or metrics.
If a timeout occurs, it is likely an environmental or infrastructure issue rather than a code problem. The test and fixture are correctly implemented.
d3rky
deleted the
asola/MPT-14865-add-e-2-e-tests-for-accounts-users-endpoints
branch
(rebased from Rob code)