MPT-14895 E2E for catalog/authorizations #147
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughCentralized a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/accounts/buyers/conftest.py(0 hunks)tests/e2e/catalog/authorizations/conftest.py(1 hunks)tests/e2e/catalog/authorizations/test_async_authorizations.py(1 hunks)tests/e2e/catalog/authorizations/test_authorizations.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/buyers/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/conftest.py
🧬 Code graph analysis (2)
tests/e2e/catalog/authorizations/conftest.py (1)
tests/e2e/conftest.py (5)
mpt_ops(31-32)product_id(96-97)seller_id(127-128)account_id(122-123)short_uuid(101-102)
tests/e2e/catalog/pricing_policies/conftest.py (1)
tests/e2e/conftest.py (2)
buyer_account_id(137-138)product_id(96-97)
🪛 GitHub Actions: PR build and merge
tests/e2e/catalog/authorizations/test_async_authorizations.py
[error] 1-6: I001 Import block is un-sorted or un-formatted. Organize imports. These issues are fixable with the '--fix' option.
tests/e2e/catalog/authorizations/test_authorizations.py
[error] 1-7: I001 Import block is un-sorted or un-formatted. Organize imports. These issues are fixable with the '--fix' option.
🪛 Ruff (0.14.7)
tests/e2e/catalog/authorizations/test_async_authorizations.py
23-23: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/authorizations/test_authorizations.py
24-24: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (9)
tests/e2e/conftest.py (1)
136-138: LGTM! Fixture centralization improves maintainability.The
buyer_account_idfixture is now properly centralized at the root level, following the same pattern as other configuration-based fixtures in this file.tests/e2e/catalog/pricing_policies/conftest.py (1)
7-11: LGTM! Parameter rename aligns with centralized fixture.The change from
buyer_idtobuyer_account_idcorrectly aligns with the centralized fixture added to the root conftest.py.tests/e2e/catalog/authorizations/conftest.py (2)
4-6: LGTM! Service fixture follows standard pattern.The
authorizations_servicefixture correctly provides access to the catalog authorizations service.
9-21: LGTM! Authorization data fixture is well-structured.The
authorization_datafixture provides a comprehensive test payload with all required fields for authorization testing.tests/e2e/catalog/authorizations/test_async_authorizations.py (3)
10-20: LGTM! Async fixtures properly structured.The async authorization fixtures correctly use async context managers for resource lifecycle management.
23-55: LGTM! Comprehensive async test coverage.The tests cover all key operations (create, get, filter, update, not found) with proper async/await patterns and error handling.
23-23: Remove unused noqa directive.The
noqa: AAA01directive is not recognized by the linter and should be removed.Apply this diff:
-def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 +def test_create_authorization(created_authorization, authorization_data):⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.tests/e2e/catalog/authorizations/test_authorizations.py (2)
10-15: LGTM! Sync fixtures properly structured.The authorization fixture correctly uses a context manager for resource lifecycle management.
18-48: LGTM! Comprehensive test coverage.The tests cover all key operations (get, create, filter, update, not found) with proper synchronous patterns and error handling.
| assert result.id == authorization_id | ||
|
|
||
|
|
||
| def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 |
There was a problem hiding this comment.
Remove unused noqa directive.
The noqa: AAA01 directive is not recognized by the linter and should be removed.
Apply this diff:
-def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01
+def test_create_authorization(created_authorization, authorization_data):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 | |
| def test_create_authorization(created_authorization, authorization_data): |
🧰 Tools
🪛 Ruff (0.14.7)
24-24: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/e2e/catalog/authorizations/test_authorizations.py around line 24, the
test definition includes an unused and unrecognized noqa directive "noqa:
AAA01"; remove the trailing ", # noqa: AAA01" from the function signature so
the test line reads simply "def test_create_authorization(created_authorization,
authorization_data):" to eliminate the invalid linter directive.
albertsola
force-pushed
the
e2e/catalog/MPT-14895-authorizations
branch
from
96e0dda to
bb6f776
Compare
albertsola
force-pushed
the
e2e/catalog/MPT-14895-authorizations
branch
from
bb6f776 to
4397619
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/catalog/authorizations/test_authorizations.py (1)
24-24: Remove unused noqa directive.The
noqa: AAA01directive remains unused and should be removed, as previously flagged.Apply this diff:
-def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 +def test_create_authorization(created_authorization, authorization_data):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/accounts/buyers/conftest.py(0 hunks)tests/e2e/catalog/authorizations/conftest.py(1 hunks)tests/e2e/catalog/authorizations/test_async_authorizations.py(1 hunks)tests/e2e/catalog/authorizations/test_authorizations.py(1 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/buyers/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/catalog/authorizations/conftest.py
- tests/e2e/catalog/pricing_policies/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/conftest.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/e2e/catalog/authorizations/test_authorizations.py
🪛 Ruff (0.14.7)
tests/e2e/catalog/authorizations/test_async_authorizations.py
23-23: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/authorizations/test_authorizations.py
24-24: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
tests/e2e/conftest.py (1)
136-138: LGTM! Fixture follows established patterns.The
buyer_account_idfixture correctly centralizes access to the buyer account ID configuration value, consistent with other config-based fixtures in this file.tests/e2e/catalog/authorizations/test_async_authorizations.py (1)
10-55: LGTM! Comprehensive async test coverage.The test suite properly covers:
- Authorization creation and cleanup via async context manager
- GET operation with ID verification
- Filtering with RQL queries and field selection
- 404 error handling
- Update operations with assertions
All async patterns are correctly implemented.
tests/e2e/catalog/authorizations/test_authorizations.py (1)
10-48: LGTM! Comprehensive synchronous test coverage.The test suite mirrors the async version with proper synchronous patterns, covering:
- Authorization creation and cleanup
- GET operation
- RQL filtering and field selection
- Error handling for not-found scenarios
- Update operations
The
# actcomment on line 48 appropriately marks the Act phase of the AAA test pattern.
| import pytest | ||
|
|
||
| from mpt_api_client.exceptions import MPTAPIError | ||
| from mpt_api_client.rql.query_builder import RQLQuery | ||
| from tests.e2e.helper import assert_async_update_resource, async_create_fixture_resource_and_delete | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/e2e/catalog/authorizations/test_async_authorizations.py | head -30Repository: softwareone-platform/mpt-api-python-client
Length of output: 1285
🏁 Script executed:
fd -t f -name "pyproject.toml" -o -name "ruff.toml" -o -name ".flake8" | head -10Repository: softwareone-platform/mpt-api-python-client
Length of output: 316
🏁 Script executed:
rg "noqa" tests/e2e/catalog/authorizations/test_async_authorizations.py -nRepository: softwareone-platform/mpt-api-python-client
Length of output: 177
🏁 Script executed:
cat pyproject.toml | grep -A 50 "\[tool.ruff\]" | head -80Repository: softwareone-platform/mpt-api-python-client
Length of output: 1373
🏁 Script executed:
cat pyproject.toml | grep -i "flake8\|aaa01" -B 2 -A 2Repository: softwareone-platform/mpt-api-python-client
Length of output: 1574
🏁 Script executed:
fd -t f "pyproject.toml" -exec grep -l "flake8-aaa\|AAA" {} \;Repository: softwareone-platform/mpt-api-python-client
Length of output: 336
Remove unnecessary noqa: AAA01 directive on line 23.
The imports on lines 1–6 are correctly sorted (stdlib, third-party, local with proper blank line separators). However, line 23 has # noqa: AAA01 which is unnecessary. flake8-aaa is installed as a dev dependency but not enabled in the ruff configuration ([tool.ruff.lint] does not include "AAA" in the select list). Remove this noqa directive.
🤖 Prompt for AI Agents
In tests/e2e/catalog/authorizations/test_async_authorizations.py (lines 1–6),
remove the unnecessary "# noqa: AAA01" directive found on line 23; since the AAA
rule set is not enabled in the project Ruff config, delete that noqa comment,
save the file, and run the linter (ruff/flake8) to verify no remaining noqa for
AAA is required.
| def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 | ||
| assert created_authorization.name == authorization_data["name"] |
There was a problem hiding this comment.
Remove unused noqa directive.
The noqa: AAA01 directive is not recognized by the project's linter (Ruff) and should be removed.
Apply this diff:
-def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01
+def test_create_authorization(created_authorization, authorization_data):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_create_authorization(created_authorization, authorization_data): # noqa: AAA01 | |
| assert created_authorization.name == authorization_data["name"] | |
| def test_create_authorization(created_authorization, authorization_data): | |
| assert created_authorization.name == authorization_data["name"] |
🧰 Tools
🪛 Ruff (0.14.7)
23-23: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/e2e/catalog/authorizations/test_async_authorizations.py around lines 23
to 24, the test function contains an unused and unrecognized noqa directive `#
noqa: AAA01`; remove the `# noqa: AAA01` suffix from the function definition
line so the file no longer contains the invalid linter directive and the test
reads simply `def test_create_authorization(created_authorization,
authorization_data):` with the assertion unchanged.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.