MPT-16791 Fix price lists tests and increase timeouts for e2e #183
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
135fb99 to
6581aa2
Compare
📝 WalkthroughWalkthroughThis PR increases default HTTP client timeouts from 10.0 → 20.0 (sync and async), removes contents of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
6581aa2 to
b9c631a
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: 0
🧹 Nitpick comments (1)
tests/e2e/catalog/price_lists/items/conftest.py (1)
26-27: Consider handling empty results gracefully.
next()will raiseStopIterationif no items match the filter, which may produce a confusing error. Consider providing a default or a clearer error message.♻️ Optional: Add a descriptive error for missing items
@pytest.fixture def price_list_item(price_list_items_service, item_id): - return next(price_list_items_service.filter(RQLQuery(item__id=item_id)).iterate()) + try: + return next(price_list_items_service.filter(RQLQuery(item__id=item_id)).iterate()) + except StopIteration: + raise ValueError(f"No price list item found for item_id={item_id}") from None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
mpt_api_client/http/async_client.pympt_api_client/http/client.pyprod.Dockerfiletests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/conftest.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
💤 Files with no reviewable changes (1)
- prod.Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/conftest.pytests/unit/http/test_async_client.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/conftest.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/test_async_price_lists.py
🧬 Code graph analysis (3)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (2)
tests/e2e/catalog/price_lists/items/conftest.py (3)
async_price_list_items_service(12-13)price_list_item_id(31-32)price_list_item_data(17-22)tests/e2e/helper.py (1)
assert_async_service_filter_with_iterate(31-39)
tests/e2e/catalog/price_lists/items/conftest.py (2)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/catalog/conftest.py (1)
item_id(5-6)
tests/e2e/catalog/price_lists/test_async_price_lists.py (3)
tests/e2e/catalog/price_lists/conftest.py (1)
price_list_id(29-30)tests/e2e/conftest.py (2)
price_list_id(152-153)short_uuid(101-102)tests/e2e/helper.py (2)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)
⏰ 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 (11)
tests/e2e/conftest.py (1)
111-113: LGTM!Using
project_root_pathfor file path resolution is more robust than relative path resolution, ensuring consistent behavior regardless of the working directory during test execution.tests/unit/http/test_client.py (1)
25-25: LGTM!Test expectations correctly updated to match the new default timeout of 20.0 seconds in
HTTPClient.Also applies to: 44-44
tests/unit/http/test_async_client.py (1)
35-35: LGTM!Test expectations correctly updated to match the new default timeout of 20.0 seconds in
AsyncHTTPClient.Also applies to: 54-54
mpt_api_client/http/client.py (1)
43-43: Timeout increase looks reasonable for E2E reliability.Doubling the default timeout from 10.0 to 20.0 seconds should help with flaky E2E tests on slower network conditions. This is a public API default change, so consider documenting it in release notes if this library follows semantic versioning.
mpt_api_client/http/async_client.py (1)
30-30: LGTM!Consistent default timeout of 20.0 seconds across both sync and async HTTP clients.
tests/e2e/catalog/price_lists/items/conftest.py (1)
30-32: LGTM!The new
price_list_item_idfixture cleanly extracts the ID for tests that only need the identifier.tests/e2e/catalog/price_lists/conftest.py (1)
29-30: Approved: Good refactor to use configuration-based IDs.Using
e2e_configforprice_list_iddecouples this fixture from needing to create a price list, improving test reliability and speed. Thecreated_price_listfixture remains actively used by other tests (test_sync_price_lists.pyandtest_async_price_lists.py), so no dead code concerns.tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (2)
11-28: LGTM! Clean refactor to ID-based fixtures.The refactor from object-based fixtures (
price_list_item) to ID-based fixtures (price_list_item_id) is consistent and correct across all three test functions. This reduces fixture dependencies and should improve test reliability.
38-44: Good semantic improvement with the function rename.The function was renamed from
test_create_price_list_item_invalid_datatotest_update_price_list_item_invalid_data, which better reflects that it's testing the update operation with invalid data. The refactor to useprice_list_item_idis consistent with other changes.tests/e2e/catalog/price_lists/test_async_price_lists.py (1)
32-50: LGTM! Smart selective refactor pattern.The refactor appropriately moves get, filter, and update tests to use the config-based
price_list_id, while intentionally keeping create and delete tests withasync_created_price_listfixtures. This pattern makes sense: read/update operations can use pre-existing resources from config, while create/delete operations require fresh, ephemeral resources.tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
11-40: LGTM! Consistent with async test refactor.The refactor mirrors the changes in the async test file, maintaining consistency across the test suite. All four test functions have been correctly updated to use
price_list_item_idinstead of theprice_list_itemobject, and the function rename on line 36 appropriately clarifies the test's intent.
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
b9c631a to
34bbe23
Compare
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
34bbe23 to
c92045c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/e2e/catalog/price_lists/conftest.py:
- Around line 29-30: The async test_update_price_list is using the shared
price_list_id fixture causing mutation of a shared resource via
assert_async_update_resource() which calls service.update(); change the async
test to consume the async_created_price_list fixture instead of price_list_id so
it uses an isolated resource (mirror the sync test's created_price_list usage)
and ensure any assertions still reference the created fixture and its id when
calling assert_async_update_resource/service.update.
In @tests/e2e/catalog/price_lists/items/conftest.py:
- Around line 26-27: The helper price_list_item should handle the case where
next(...) raises StopIteration; wrap the call to
next(price_list_items_service.filter(RQLQuery(item__id=item_id)).iterate()) in a
try/except StopIteration and raise a clear error (e.g., RuntimeError or
AssertionError) that includes the missing item_id and that no price list item
was found, or alternatively return None and document the behavior; update
callers/tests accordingly so failures show the clearer message instead of an
unhandled StopIteration.
In @tests/e2e/catalog/price_lists/test_async_price_lists.py:
- Around line 44-50: test_update_price_list is mutating a shared config resource
(price_list_id) causing state leakage; change the test to use the isolated
disposable fixture async_created_price_list (same pattern as
test_delete_price_list) instead of price_list_id, update the test signature to
accept async_created_price_list (and/or rename local variable to price_list_id
from that fixture), and call assert_async_update_resource with
async_created_price_list so the test updates an ephemeral resource and preserves
isolation for concurrent runs.
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/pull-request.ymlmpt_api_client/http/async_client.pympt_api_client/http/client.pyprod.Dockerfiletests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/conftest.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
💤 Files with no reviewable changes (1)
- prod.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (5)
- mpt_api_client/http/client.py
- mpt_api_client/http/async_client.py
- tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
- tests/e2e/conftest.py
- tests/e2e/catalog/price_lists/items/test_async_price_list_items.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/catalog/price_lists/test_async_price_lists.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/unit/http/test_async_client.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/catalog/price_lists/test_async_price_lists.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/unit/http/test_async_client.py
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/catalog/price_lists/test_async_price_lists.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/unit/http/test_async_client.py
🧬 Code graph analysis (3)
tests/e2e/catalog/price_lists/test_async_price_lists.py (5)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (3)
test_get_price_list(18-21)test_filter_price_lists(38-41)test_update_price_list(44-47)tests/e2e/catalog/price_lists/conftest.py (1)
price_list_id(29-30)tests/e2e/conftest.py (2)
price_list_id(152-153)short_uuid(101-102)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/helper.py (2)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)
tests/e2e/catalog/price_lists/conftest.py (1)
tests/e2e/conftest.py (2)
price_list_id(152-153)e2e_config(89-92)
tests/e2e/catalog/price_lists/items/conftest.py (4)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/unit/resources/catalog/test_price_list_items.py (1)
price_list_items_service(10-13)tests/e2e/catalog/conftest.py (1)
item_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
⏰ 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/unit/http/test_client.py (1)
25-25: LGTM! Timeout expectations correctly updated.The test assertions now correctly expect a timeout of 20.0 seconds, aligning with the updated default timeout in the HTTPClient implementation. Both explicit initialization and environment-based initialization tests are updated consistently.
Also applies to: 44-44
tests/unit/http/test_async_client.py (1)
35-35: LGTM! Async client timeout expectations correctly updated.The test assertions now correctly expect a timeout of 20.0 seconds, aligning with the updated default timeout in the AsyncHTTPClient implementation. Both explicit initialization and environment-based initialization tests are updated consistently, matching the changes in the synchronous client tests.
Also applies to: 54-54
tests/e2e/catalog/price_lists/items/conftest.py (1)
30-32: LGTM!The
price_list_item_idfixture correctly extracts the ID from theprice_list_itemfixture for use in tests.tests/e2e/catalog/price_lists/test_async_price_lists.py (2)
32-35: LGTM!Using the config-based
price_list_idfor read-only operations is appropriate and ensures the test can run against a stable, pre-existing resource.
38-41: LGTM!Using the config-based
price_list_idfor filtering operations is appropriate for read-only tests..github/workflows/pull-request.yml (1)
39-44: Verify that the required secrets are configured in the GitHub repository settings.The E2E test step correctly references
RP_ENDPOINTandRP_API_KEYsecrets using proper GitHub Actions syntax. Ensure these secrets are configured in the repository settings for the workflow to execute successfully.
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
c92045c to
f84ebae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (1)
1-1: Address Ruff formatting issue.The pipeline indicates this file needs reformatting with
ruff format.Run the following command to fix the formatting:
#!/bin/bash # Format the file with ruff ruff format tests/e2e/catalog/price_lists/test_sync_price_lists.py
🧹 Nitpick comments (2)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (1)
18-27: Duplicate test detected.Both
test_get_price_listandtest_get_price_list_by_id(lines 18-21 and 24-27) appear to perform identical operations with identical assertions. Consider removing one of these duplicate tests.♻️ Proposed fix
-def test_get_price_list(price_lists_service, price_list_id): - result = price_lists_service.get(price_list_id) - - assert result.id == price_list_id - - def test_get_price_list_by_id(price_lists_service, price_list_id): result = price_lists_service.get(price_list_id) assert result.id == price_list_id.github/workflows/pull-request.yml (1)
39-44: Consider passing ReportPortal credentials via environment variables only.Good addition of E2E tests to the PR workflow! The ReportPortal pytest plugin supports reading
RP_ENDPOINTandRP_API_KEYdirectly from environment variables. Since you're already setting both as environment variables (lines 43-44), you can simplify line 40 to remove the redundant CLI arguments:Suggested change
- run: make e2e args="--reportportal --rp-launch=$RP_LAUNCH --rp-api-key=$RP_API_KEY --rp-endpoint=$RP_ENDPOINT" + run: make e2e args="--reportportal --rp-launch=$RP_LAUNCH"This approach is cleaner and reduces the surface area for accidental secret exposure in logs.
Also, verify that the 45-minute workflow timeout (line 14) remains sufficient with E2E tests now enabled.
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/pull-request.ymlmpt_api_client/http/async_client.pympt_api_client/http/client.pyprod.Dockerfiletests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/conftest.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
💤 Files with no reviewable changes (1)
- prod.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/catalog/price_lists/items/conftest.py
- tests/unit/http/test_async_client.py
- mpt_api_client/http/async_client.py
- mpt_api_client/http/client.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/conftest.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/conftest.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In tests/e2e/catalog/price_lists/items/conftest.py, the price_list_item fixture is seeded to always provide an item. Do not add StopIteration handling in this test fixture since absence of items is not expected; treat StopIteration as an upstream test setup failure rather than a recoverable condition.
Applied to files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/conftest.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/conftest.pytests/unit/http/test_client.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
📚 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/price_lists/conftest.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/catalog/price_lists/conftest.py
🧬 Code graph analysis (3)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (2)
tests/e2e/catalog/price_lists/items/conftest.py (2)
price_list_item_id(31-32)price_list_item_data(17-22)tests/e2e/helper.py (1)
assert_async_service_filter_with_iterate(31-39)
tests/e2e/catalog/price_lists/test_async_price_lists.py (4)
tests/e2e/catalog/price_lists/conftest.py (1)
price_list_id(29-30)tests/e2e/conftest.py (1)
price_list_id(152-153)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/helper.py (2)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)
tests/e2e/catalog/price_lists/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
🪛 GitHub Actions: PR build and merge
tests/e2e/catalog/price_lists/test_sync_price_lists.py
[error] 1-1: Ruff format check failed. File would be reformatted by 'ruff format'.
🔇 Additional comments (8)
tests/unit/http/test_client.py (2)
13-27: LGTM! Test correctly reflects the new default timeout.The test now expects the HTTPClient to initialize with a 20.0 second timeout, matching the updated default in the implementation.
30-46: LGTM! Test correctly reflects the new default timeout.The test now expects the HTTPClient initialized from environment variables to use a 20.0 second timeout, consistent with the updated default and the other initialization test.
tests/e2e/conftest.py (1)
111-113: LGTM! Improved consistency through dependency injection.The refactoring to use the
project_root_pathfixture improves testability and aligns with the pattern established by other fixtures likee2e_config. The path construction is correct and explicit about dependencies.tests/e2e/catalog/price_lists/conftest.py (1)
29-30: LGTM! Fixture refactored to use centralized config.This change aligns with the team's pattern of reusing existing API resources (via
e2e_config) for read-only and safe mutation tests, improving test execution speed.tests/e2e/catalog/price_lists/test_async_price_lists.py (1)
32-50: LGTM! Tests correctly refactored to use ID-based fixtures.The refactoring appropriately uses
price_list_idfor read-only and safe mutation operations (get, filter, update), whiletest_delete_price_listcorrectly retains theasync_created_price_listfixture for destructive operations. This aligns with the team's pattern for balancing test speed and proper isolation.tests/e2e/catalog/price_lists/test_sync_price_lists.py (1)
30-47: LGTM! Tests correctly refactored to use ID-based fixtures.The refactoring appropriately uses
price_list_idfor read-only and safe mutation operations, whiletest_delete_price_listcorrectly retains thecreated_price_listfixture for destructive operations.tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
11-44: LGTM! Tests correctly refactored to use ID-based fixtures.The refactoring appropriately:
- Uses
price_list_item_idfor all operations- Renames the invalid data test from "create" to "update" which better reflects the operation being tested
- Uses appropriate invalid data (
{"unitPP": "NaN"}) for validation testingThe pattern is consistent with the broader E2E test refactoring in this PR.
tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
11-40: LGTM! Tests correctly refactored to use ID-based fixtures.The refactoring is consistent with the async version and appropriately:
- Uses
price_list_item_idfor all operations- Renames the invalid data test to reflect the actual operation (update)
- Uses appropriate invalid data for validation testing
albertsola
force-pushed
the
fix/MPT-16791/Unreliable-e2e-pass-runs
branch
from
f84ebae to
17716e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (1)
18-27: Remove duplicate test.
test_get_price_list(lines 18-21) andtest_get_price_list_by_id(lines 24-27) now contain identical logic after the migration toprice_list_id. Consider removing one of these tests to eliminate duplication.♻️ Proposed fix to remove duplicate test
-def test_get_price_list(price_lists_service, price_list_id): - result = price_lists_service.get(price_list_id) - - assert result.id == price_list_id - - def test_get_price_list_by_id(price_lists_service, price_list_id): result = price_lists_service.get(price_list_id) assert result.id == price_list_id
🧹 Nitpick comments (1)
tests/e2e/conftest.py (1)
73-75: Consider applying the same pattern topdf_fdfor consistency.The
logo_fdfixture now usesproject_root_pathfor path construction, butpdf_fdstill uses the relative path approach. For consistency and maintainability, consider updatingpdf_fdto follow the same pattern.♻️ Suggested refactor
@pytest.fixture -def pdf_fd(): - icon_path = pathlib.Path(__file__).parent / "empty.pdf" +def pdf_fd(project_root_path): + icon_path = project_root_path / "tests/e2e/empty.pdf" return pathlib.Path.open(icon_path, "rb")
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/pull-request.ymlmpt_api_client/http/async_client.pympt_api_client/http/client.pyprod.Dockerfiletests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/conftest.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
💤 Files with no reviewable changes (1)
- prod.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/pull-request.yml
- tests/e2e/catalog/price_lists/conftest.py
- tests/unit/http/test_async_client.py
- mpt_api_client/http/client.py
- mpt_api_client/http/async_client.py
- tests/unit/http/test_client.py
- tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
tests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/conftest.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
tests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/conftest.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/conftest.py
📚 Learning: 2026-01-08T08:34:19.306Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/items/conftest.py:26-27
Timestamp: 2026-01-08T08:34:19.306Z
Learning: In tests/e2e/catalog/price_lists/items/conftest.py, the price_list_item fixture is seeded to always provide an item. Do not add StopIteration handling in this test fixture since absence of items is not expected; treat StopIteration as an upstream test setup failure rather than a recoverable condition.
Applied to files:
tests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/conftest.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/catalog/price_lists/test_sync_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/conftest.py
🧬 Code graph analysis (3)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (3)
tests/e2e/catalog/price_lists/conftest.py (2)
price_lists_service(18-19)price_list_id(29-30)tests/e2e/conftest.py (2)
price_list_id(152-153)short_uuid(101-102)tests/e2e/helper.py (2)
assert_service_filter_with_iterate(42-50)assert_update_resource(53-59)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (4)
tests/e2e/catalog/price_lists/items/conftest.py (3)
async_price_list_items_service(12-13)price_list_item_id(31-32)price_list_item_data(17-22)tests/unit/resources/catalog/test_price_list_items.py (1)
async_price_list_items_service(17-20)tests/e2e/helper.py (1)
assert_async_service_filter_with_iterate(31-39)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)
tests/e2e/catalog/price_lists/test_async_price_lists.py (5)
tests/e2e/catalog/price_lists/test_sync_price_lists.py (3)
test_get_price_list(18-21)test_filter_price_lists(38-39)test_update_price_list(42-45)tests/e2e/catalog/price_lists/conftest.py (1)
price_list_id(29-30)tests/e2e/conftest.py (2)
price_list_id(152-153)short_uuid(101-102)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/helper.py (2)
assert_async_service_filter_with_iterate(31-39)assert_async_update_resource(62-68)
⏰ 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 (7)
tests/e2e/conftest.py (1)
111-112: LGTM! Improved consistency withproject_root_pathinjection.The change to inject
project_root_pathand use it for path construction improves consistency with other fixtures likee2e_configand makes the file path more explicit and maintainable.tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
11-44: LGTM! Clean migration to ID-based fixtures.The migration from
price_list_itemobject toprice_list_item_idis consistent across all test functions. The changes align with the PR's objective to improve test reliability by using config-based IDs.tests/e2e/catalog/price_lists/items/conftest.py (3)
3-3: LGTM! Appropriate import added.The
RQLQueryimport is necessary for the updated filtering logic in theprice_list_itemfixture.
30-32: LGTM! ID extraction fixture is straightforward.The
price_list_item_idfixture correctly extracts the ID from the resolvedprice_list_item.
26-27: No action required—theitem_idfixture is properly defined.The
item_idfixture is already defined intests/e2e/catalog/conftest.pyand retrieves its value from the e2e configuration, making it available to all child test directories includingtests/e2e/catalog/price_lists/items/. The fixture dependency is satisfied.tests/e2e/catalog/price_lists/test_sync_price_lists.py (1)
30-45: LGTM! Tests correctly migrated to use price_list_id.The iterate, filter, and update tests have been properly updated to use the
price_list_idfixture instead of thecreated_price_listobject. The changes are consistent with the PR's migration strategy.tests/e2e/catalog/price_lists/test_async_price_lists.py (1)
32-50: LGTM! Async tests correctly migrated to ID-based fixtures.The async price list tests have been properly updated to use
price_list_idfrom the config instead ofasync_created_price_list. This change:
- Improves test reliability by reusing existing resources
- Maintains consistency with the sync test variant
- Correctly preserves async function signatures
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
Closes MPT-16791