MPT-14897 E2E Catalog listings #149
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds catalog-specific config keys and new pytest fixtures, plus synchronous and asynchronous end-to-end tests for catalog listings (create, get, filter, not-found, update, delete), wiring fixtures for authorization, listing, and price-list identifiers. 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
🧹 Nitpick comments (1)
tests/e2e/catalog/listings/conftest.py (1)
14-34: Use price_list_id fixture instead of hardcoded value.The
listing_datafixture hardcodes the price list ID"PRC-7255-3950-0245"on line 30. Since aprice_list_idfixture was added intests/e2e/conftest.py(lines 146-148) that retrieves this value from the configuration, use that fixture for consistency and maintainability. Also, the inline comment on line 25 is unnecessary since the value is already provided via fixture.Apply this diff:
@pytest.fixture -def listing_data(authorization_id, product_id, account_id, seller_id): +def listing_data(authorization_id, product_id, account_id, seller_id, price_list_id): return { "name": "e2e - please delete", "authorization": { "id": authorization_id, }, "product": { "id": product_id, }, "vendor": { - "id": account_id, # "ACC-9042-0088", + "id": account_id, }, "seller": { "id": seller_id, }, - "priceList": {"id": "PRC-7255-3950-0245"}, + "priceList": {"id": price_list_id}, "primary": False, "notes": "", "eligibility": {"client": True, "partner": False}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/catalog/conftest.py (1)
tests/e2e/conftest.py (2)
authorization_id(142-143)e2e_config(89-92)
tests/e2e/catalog/listings/conftest.py (3)
tests/e2e/catalog/listings/test_sync_listings.py (1)
listings_service(11-12)tests/e2e/conftest.py (6)
mpt_ops(31-32)e2e_config(89-92)authorization_id(142-143)product_id(96-97)account_id(122-123)seller_id(127-128)tests/e2e/catalog/conftest.py (1)
authorization_id(20-21)
tests/e2e/catalog/listings/test_async_listings.py (5)
tests/e2e/helper.py (2)
assert_async_update_resource(62-68)async_create_fixture_resource_and_delete(8-16)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)product_id(96-97)tests/e2e/catalog/listings/conftest.py (2)
listing_data(15-34)listing_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
🪛 GitHub Actions: PR build and merge
tests/e2e/catalog/listings/test_async_listings.py
[error] 2-2: ModuleNotFoundError: No module named 'e2e' while importing tests/e2e/catalog/listings/test_async_listings.py
tests/e2e/catalog/listings/test_sync_listings.py
[error] 2-2: ModuleNotFoundError: No module named 'e2e' while importing tests/e2e/catalog/listings/test_sync_listings.py
🪛 Ruff (0.14.7)
tests/e2e/catalog/listings/test_async_listings.py
23-23: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/listings/test_sync_listings.py
21-21: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (6)
tests/e2e/conftest.py (1)
146-148: LGTM!The fixture follows the established pattern for retrieving configuration values and will be useful for catalog listings tests.
e2e_config.test.json (1)
14-33: LGTM!The configuration properly introduces catalog-specific identifiers while maintaining existing commerce identifiers, supporting both namespaces for the E2E test suite.
tests/e2e/catalog/listings/test_sync_listings.py (2)
21-21: Remove unused noqa directive.The
noqa: AAA01directive is not recognized by the configured linters and should be removed.Apply this diff:
-def test_create_listing(created_listing, product_id): # noqa: AAA01 +def test_create_listing(created_listing, product_id): assert created_listing.product.id == product_id⛔ 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.
10-12: Remove duplicate fixture definition.The
listings_servicefixture is already defined intests/e2e/catalog/listings/conftest.py(lines 4-6) with the exact same implementation. Remove this duplicate definition to avoid confusion and potential fixture resolution issues.Apply this diff:
-@pytest.fixture -def listings_service(mpt_ops): - return mpt_ops.catalog.listings - -⛔ Skipped due to learnings
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.tests/e2e/catalog/listings/test_async_listings.py (2)
23-24: Sync test function cannot use async fixture.The
test_create_listingfunction is not declared asasync, but it depends onasync_created_listing, which is an async fixture. Pytest requires test functions that consume async fixtures to be declared as async.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +async def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_id⛔ 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.701Z Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
23-23: Remove unused noqa directive.The
noqa: AAA01directive is not recognized by the configured linters and should be removed.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +async def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_id⛔ 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.
| @pytest.fixture | ||
| def authorization_id(e2e_config): | ||
| return e2e_config["catalog.authorization.id"] |
There was a problem hiding this comment.
Duplicate fixture name creates shadowing and confusion.
This authorization_id fixture shadows an existing fixture in tests/e2e/conftest.py (lines 141-142) that retrieves commerce.authorization.id. While pytest's scoping means this catalog-specific version will take precedence within the tests/e2e/catalog/ directory, having two fixtures with the same name retrieving different configuration keys is confusing and error-prone.
Consider renaming one of these fixtures (e.g., catalog_authorization_id vs commerce_authorization_id) to make the distinction explicit and prevent unintended fixture resolution.
🤖 Prompt for AI Agents
In tests/e2e/catalog/conftest.py around lines 19-21, the fixture
authorization_id duplicates a fixture name defined in tests/e2e/conftest.py and
causes shadowing; rename this fixture to something explicit (e.g.,
catalog_authorization_id), update its return to still use
e2e_config["catalog.authorization.id"], and then update all tests in
tests/e2e/catalog/ that currently depend on authorization_id to use
catalog_authorization_id so resolution is unambiguous and self-documenting.
af9b083 to
8dd1a7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/catalog/listings/test_async_listings.py (1)
26-27: Consider making this test async for consistency.This test function is synchronous while using an async fixture (
async_created_listing). Although pytest-asyncio can handle this, all other test functions in this module (lines 30, 36, 40, 47) are async, making this inconsistent.Apply this diff to make the test async:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +async def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/catalog/listings/conftest.py
- tests/e2e/catalog/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 (1)
tests/e2e/catalog/listings/test_async_listings.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (3)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)async_create_fixture_resource_and_delete(8-16)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)product_id(96-97)tests/e2e/catalog/listings/conftest.py (1)
listing_data(15-34)tests/e2e/catalog/listings/test_sync_listings.py (1)
test_create_listing(24-25)tests/seed/catalog/test_product.py (1)
product(18-19)
🪛 Ruff (0.14.7)
tests/e2e/catalog/listings/test_sync_listings.py
24-24: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/listings/test_async_listings.py
26-26: 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 (16)
tests/e2e/conftest.py (1)
146-148: LGTM!The new
price_list_idfixture follows the established pattern for e2e config fixtures and correctly references the catalog price list identifier added to the configuration.tests/e2e/catalog/listings/test_async_listings.py (7)
13-15: LGTM!The fixture correctly provides the async listings service from the operations client.
18-23: LGTM!The async fixture properly manages the listing lifecycle using the async context manager helper for resource creation and cleanup.
30-33: LGTM!The async test correctly retrieves a listing by ID and verifies the result.
36-37: LGTM!The async filter test correctly uses the helper to verify filtering functionality.
40-44: LGTM!The async test properly validates not-found error handling with the expected 404 response.
47-50: LGTM!The async update test correctly uses the helper to verify listing updates.
26-26: Remove unused noqa directive.The
noqa: AAA01directive is not recognized and should be removed.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_id⛔ 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.e2e_config.test.json (1)
14-33: LGTM!The configuration correctly introduces catalog-specific identifiers while maintaining existing commerce identifiers, providing comprehensive test data for both catalog and commerce e2e tests.
tests/e2e/catalog/listings/test_sync_listings.py (7)
13-15: LGTM!The fixture correctly provides the synchronous listings service from the operations client.
18-21: LGTM!The sync fixture properly manages the listing lifecycle using the sync context manager helper for resource creation and cleanup.
28-31: LGTM!The test correctly retrieves a listing by ID and verifies the result.
34-39: LGTM!The filter test correctly uses the helper to verify filtering functionality.
42-46: LGTM!The test properly validates not-found error handling with the expected 404 response.
49-52: LGTM!The update test correctly uses the helper to verify listing updates.
24-25: Remove unused noqa directive.The
noqa: AAA01directive is not recognized and should be removed.Apply this diff:
-def test_create_listing(created_listing, product_id): # noqa: AAA01 +def test_create_listing(created_listing, product_id): assert created_listing.product.id == product_id⛔ 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.
albertsola
force-pushed
the
e2e/catalog/MPT-14897-catalog-listings
branch
2 times, most recently
from
653327c to
931b9d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/e2e/catalog/listings/test_async_listings.py (1)
26-27: Remove unused noqa directive.The
noqa: AAA01comment is unnecessary and should be removed for cleaner code.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_idtests/e2e/catalog/listings/test_sync_listings.py (1)
24-25: Remove unused noqa directive.The
noqa: AAA01comment is unnecessary and should be removed for cleaner code.Apply this diff:
-def test_create_listing(created_listing, product_id): # noqa: AAA01 +def test_create_listing(created_listing, product_id): assert created_listing.product.id == product_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/catalog/conftest.py
- tests/e2e/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/catalog/listings/test_async_listings.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (3)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)async_create_fixture_resource_and_delete(8-16)tests/e2e/conftest.py (3)
async_mpt_ops(36-39)product_id(96-97)short_uuid(101-102)tests/seed/catalog/test_product.py (1)
product(15-16)
tests/e2e/catalog/listings/conftest.py (2)
tests/e2e/conftest.py (6)
mpt_ops(31-32)e2e_config(89-92)authorization_id(147-148)product_id(96-97)account_id(122-123)seller_id(127-128)tests/e2e/catalog/conftest.py (1)
authorization_id(20-21)
🪛 Ruff (0.14.7)
tests/e2e/catalog/listings/test_async_listings.py
26-26: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/listings/test_sync_listings.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 (6)
tests/e2e/catalog/listings/test_async_listings.py (2)
13-23: LGTM! Fixtures are well-structured.The fixtures correctly provide the async listings service and handle the create/delete lifecycle for test listings using the helper context manager.
30-54: LGTM! Async test coverage is comprehensive.The tests correctly cover retrieval, filtering, error handling, updates, and deletion using appropriate async patterns and helper functions.
e2e_config.test.json (1)
14-33: LGTM! Configuration entries are properly structured.The catalog and commerce identifiers are correctly added to support the new end-to-end listing tests.
tests/e2e/catalog/listings/conftest.py (1)
4-11: LGTM! Service and ID fixtures are correctly implemented.These fixtures properly extract the listings service and listing ID from the appropriate sources.
tests/e2e/catalog/listings/test_sync_listings.py (2)
18-21: LGTM! Fixture handles lifecycle correctly.The
created_listingfixture properly manages the create/delete lifecycle using the helper context manager.
28-56: LGTM! Sync test coverage is comprehensive.The tests correctly cover all CRUD operations, filtering, and error handling using appropriate helper functions and assertions.
| @pytest.fixture | ||
| def listings_service(mpt_ops): | ||
| return mpt_ops.catalog.listings |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate fixture definition.
The listings_service fixture is already defined in tests/e2e/catalog/listings/conftest.py (lines 4-6). This duplicate definition should be removed.
Apply this diff:
-@pytest.fixture
-def listings_service(mpt_ops):
- return mpt_ops.catalog.listings
-
-📝 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.
| @pytest.fixture | |
| def listings_service(mpt_ops): | |
| return mpt_ops.catalog.listings |
🤖 Prompt for AI Agents
In tests/e2e/catalog/listings/test_sync_listings.py around lines 13 to 15,
remove the duplicate listings_service fixture definition (it is already defined
in tests/e2e/catalog/listings/conftest.py lines 4-6); delete these three lines
so the test uses the shared fixture from conftest.py instead of redefining it
locally.
931b9d5 to
4ef140e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/catalog/listings/test_sync_listings.py (1)
13-15: Remove duplicate fixture definition.The
listings_servicefixture is already defined intests/e2e/catalog/listings/conftest.py. This duplicate definition should be removed to use the shared fixture.Apply this diff:
-@pytest.fixture -def listings_service(mpt_ops): - return mpt_ops.catalog.listings - -
🧹 Nitpick comments (2)
tests/e2e/catalog/listings/test_async_listings.py (1)
26-27: Remove unused noqa directive.The
noqa: AAA01comment references a flake8-aaa code that is not recognized by ruff (the static analysis tool in use). Remove the unused directive.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +def test_create_listing(async_created_listing, product_id): assert async_created_listing.product.id == product_idtests/e2e/catalog/listings/test_sync_listings.py (1)
24-25: Remove unused noqa directive.The
noqa: AAA01comment references a flake8-aaa code that is not recognized by ruff (the static analysis tool in use). Remove the unused directive.Apply this diff:
-def test_create_listing(created_listing, product_id): # noqa: AAA01 +def test_create_listing(created_listing, product_id): assert created_listing.product.id == product_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/catalog/conftest.py
- e2e_config.test.json
- tests/e2e/catalog/listings/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/catalog/listings/test_sync_listings.py
🧬 Code graph analysis (3)
tests/e2e/conftest.py (1)
tests/e2e/catalog/conftest.py (1)
price_list_id(25-26)
tests/e2e/catalog/listings/test_sync_listings.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (3)
assert_service_filter_with_iterate(42-50)assert_update_resource(53-59)create_fixture_resource_and_delete(20-28)tests/e2e/catalog/listings/conftest.py (2)
listing_data(15-34)listing_id(10-11)tests/e2e/conftest.py (3)
mpt_ops(31-32)product_id(96-97)short_uuid(101-102)tests/seed/catalog/test_product.py (1)
product(15-16)
tests/e2e/catalog/listings/test_async_listings.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (3)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)async_create_fixture_resource_and_delete(8-16)tests/e2e/conftest.py (3)
async_mpt_ops(36-39)product_id(96-97)short_uuid(101-102)tests/e2e/catalog/listings/conftest.py (1)
listing_data(15-34)tests/seed/catalog/test_product.py (1)
product(15-16)
🪛 Ruff (0.14.7)
tests/e2e/catalog/listings/test_sync_listings.py
24-24: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/listings/test_async_listings.py
26-26: 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 (8)
tests/e2e/conftest.py (1)
151-153: LGTM! Fixture addition follows established pattern.The
price_list_idfixture correctly retrieves the catalog price list ID from the e2e configuration. While a fixture with the same name exists intests/e2e/catalog/conftest.py, this parent-level fixture provides a fallback for tests outside the catalog subdirectory. Based on learnings, this duplication pattern is intentional and allows child scopes to override when needed.tests/e2e/catalog/listings/test_async_listings.py (4)
1-10: LGTM! Imports and test markers are correct.The imports use the correct
tests.e2e.helperpath, and the flaky marker appropriately handles potential transient failures in end-to-end tests.
13-15: LGTM! Fixture provides async listings service.The fixture correctly exposes the async catalog listings service for use in async tests.
18-23: LGTM! Async fixture with proper lifecycle management.The fixture correctly uses an async context manager to create a listing resource and ensure cleanup after the test completes.
30-54: LGTM! Comprehensive async test coverage.The async tests properly cover the full listing lifecycle:
- Retrieval by ID
- Filtering with iteration
- Error handling for not-found scenarios
- Updates with field modifications
- Deletion
All tests correctly use
awaitfor async operations and follow established testing patterns.tests/e2e/catalog/listings/test_sync_listings.py (3)
1-10: LGTM! Imports and test markers are correct.The imports use the correct
tests.e2e.helperpath, and the flaky marker appropriately handles potential transient failures in end-to-end tests.
18-21: LGTM! Fixture provides proper lifecycle management.The fixture correctly uses a context manager to create a listing resource and ensure cleanup after the test completes.
28-56: LGTM! Comprehensive sync test coverage.The sync tests properly cover the full listing lifecycle with synchronous operations:
- Retrieval by ID
- Filtering with iteration
- Error handling for not-found scenarios
- Updates with field modifications
- Deletion
All tests follow established patterns and provide good coverage parallel to the async tests.
albertsola
force-pushed
the
e2e/catalog/MPT-14897-catalog-listings
branch
from
4ef140e to
01c1445
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/catalog/listings/test_async_listings.py (1)
26-26: Remove unused noqa directive.The
noqa: AAA01directive references an unknown error code. If this is meant for a different linter, please verify; otherwise, remove it.Apply this diff:
-def test_create_listing(async_created_listing, product_id): # noqa: AAA01 +def test_create_listing(async_created_listing, product_id):tests/e2e/catalog/listings/test_sync_listings.py (1)
24-24: Remove unused noqa directive.The
noqa: AAA01directive references an unknown error code. If this is meant for a different linter, please verify; otherwise, remove it.Apply this diff:
-def test_create_listing(created_listing, product_id): # noqa: AAA01 +def test_create_listing(created_listing, product_id):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/catalog/conftest.py
- tests/e2e/catalog/listings/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/catalog/listings/test_sync_listings.py
🧬 Code graph analysis (2)
tests/e2e/conftest.py (1)
tests/e2e/catalog/conftest.py (1)
price_list_id(25-26)
tests/e2e/catalog/listings/test_sync_listings.py (3)
tests/e2e/helper.py (2)
assert_service_filter_with_iterate(42-50)assert_update_resource(53-59)tests/e2e/conftest.py (3)
mpt_ops(31-32)product_id(96-97)short_uuid(101-102)tests/e2e/catalog/listings/conftest.py (2)
listing_data(10-29)listing_id(5-6)
🪛 Ruff (0.14.7)
tests/e2e/catalog/listings/test_async_listings.py
26-26: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/catalog/listings/test_sync_listings.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 (4)
tests/e2e/catalog/listings/test_async_listings.py (1)
1-54: LGTM! Comprehensive async test coverage.The async test suite properly covers the full lifecycle of catalog listings including creation, retrieval, filtering, error handling, updates, and deletion. The use of async context managers and helper utilities follows established patterns.
e2e_config.test.json (1)
14-33: LGTM! Configuration properly supports catalog and commerce namespaces.The addition of catalog-specific identifiers alongside existing commerce identifiers enables proper E2E test configuration for the catalog listings functionality.
tests/e2e/catalog/listings/test_sync_listings.py (1)
1-56: LGTM! Comprehensive sync test coverage.The sync test suite provides thorough coverage of catalog listings operations, properly using synchronous patterns and helper utilities. The test structure mirrors the async implementation effectively.
tests/e2e/conftest.py (1)
151-153: No action required. Theprice_list_idfixture intests/e2e/catalog/conftest.pyis intentionally scoped to its directory hierarchy. Childconftest.pyfiles in pytest commonly define their own fixtures to control scope and precedence, which is standard behavior—not a defect.Likely an incorrect or invalid review comment.
|
|
||
|
|
||
| def test_create_listing(async_created_listing, product_id): # noqa: AAA01 | ||
| assert async_created_listing.product.id == product_id |
There was a problem hiding this comment.
| assert async_created_listing.product.id == product_id | |
| def test_create_listing(async_created_listing, product_id): | |
| result = async_created_listing.product.id | |
| assert result == product_id |
| def test_create_listing(created_listing, product_id): # noqa: AAA01 | ||
| assert created_listing.product.id == product_id |
There was a problem hiding this comment.
| def test_create_listing(created_listing, product_id): # noqa: AAA01 | |
| assert created_listing.product.id == product_id | |
| def test_create_listing(created_listing, product_id): | |
| result = created_listing.product.id | |
| assert result == product_id |
albertsola
force-pushed
the
e2e/catalog/MPT-14897-catalog-listings
branch
from
01c1445 to
c9a22d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/catalog/listings/test_sync_listings.py (1)
24-58: E2E coverage for listing lifecycle is solid, with one optional refinement around delete.
test_create_listing,test_get_listing_by_id,test_filter_listings, andtest_update_listingexercise the happy-path lifecycle well and correctly reuse shared helpers and fixtures (product_id,listing_id,short_uuid).test_get_listing_not_foundassertingMPTAPIErrorwithmatch=r"404 Not Found"aligns with the currentMPTAPIError.__str__format and is a clear contract for 404s.One minor optional refinement:
- In
test_delete_listing, you deletecreated_listing.idexplicitly whilecreated_listing’s fixture teardown will also attempt to delete the same resource viacreate_fixture_resource_and_delete. Because that helper swallowsMPTAPIError, this is safe, but it may cause extra API traffic and noisy teardown logs if deletes are not idempotent.- If this becomes noisy, consider:
- Creating a dedicated “delete-only” fixture (or inline create) that does not auto-delete in teardown for this test, or
- Extending
create_fixture_resource_and_deleteto detect/skip deletion when the resource has already been deleted.Right now it’s harmless, so this is purely an optional cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/catalog/conftest.py(1 hunks)tests/e2e/catalog/listings/conftest.py(1 hunks)tests/e2e/catalog/listings/test_async_listings.py(1 hunks)tests/e2e/catalog/listings/test_sync_listings.py(1 hunks)tests/e2e/conftest.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/catalog/conftest.py
- tests/e2e/conftest.py
- tests/e2e/catalog/listings/test_async_listings.py
- tests/e2e/catalog/listings/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/catalog/listings/test_sync_listings.py
🧬 Code graph analysis (1)
tests/e2e/catalog/listings/test_sync_listings.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/helper.py (3)
assert_service_filter_with_iterate(42-50)assert_update_resource(53-59)create_fixture_resource_and_delete(20-28)tests/e2e/conftest.py (3)
mpt_ops(31-32)product_id(96-97)short_uuid(101-102)tests/e2e/catalog/listings/conftest.py (1)
listing_data(10-29)
⏰ 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)
e2e_config.test.json (1)
14-33: Config additions for catalog/commerce IDs look consistent with new E2E fixtures.The new
catalog.*entries (authorization, listing, price_list, product/item) and the retainedcommerce.*IDs match the fixtures and helper usage in the E2E tests (e.g.,authorization_id,price_list_id,listing_id,product_id). The JSON remains valid and the naming is coherent, so this config change looks good.tests/e2e/catalog/listings/test_sync_listings.py (2)
1-8: Imports and helper usage are wired correctly.Using
tests.e2e.helperforassert_service_filter_with_iterate,assert_update_resource, andcreate_fixture_resource_and_deleteis consistent with the existing E2E helpers and resolves the earlier import-path issue; no changes needed here.
13-21: Fixture setup forlistings_serviceandcreated_listingis appropriate.Resolving
mpt_ops.catalog.listingsvia thelistings_servicefixture and usingcreate_fixture_resource_and_deleteforcreated_listinggives clear, reusable wiring and automatic cleanup for created listings; this matches the patterns used elsewhere in the e2e suite and looks good as-is. Based on learnings, I’m intentionally not suggesting moving or deduplicating these fixtures across conftest layers in this PR.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.