MPT-21922 Queue and Case Status update upon test completions#341
Merged
Conversation
📝 WalkthroughWalkthroughAdds best-effort finalize helpers for E2E fixtures and updates helpdesk fixtures to finalize queues (disable) and cases (complete) on teardown; refactors BaseModel._process_value to delegate dict and list handling to new helper functions. ChangesE2E Fixture Finalize Infrastructure
Model refactor: BaseModel value processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🔕 Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 error)
✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (1)
mpt_api_client/models/model.py (1)
124-131: Update_resolve_list_model_classto handlelist[T] | None(union/optional list typing)
_resolve_list_model_classonly recognizesget_origin(target_class) is list; forlist[T] | None/Optional[list[T]]it will fall back toBaseModel. In this repo, the optional list fields inmpt_api_client/resources/**are typed aslist[BaseModel] | None(e.g.,agreements.py,pricing_policies.py), so this won’t currently degrade element coercion for concrete model subclasses—but it will for futurelist[ConcreteModel] | Noneannotations unless unions are unwrapped before checking the list origin. [Also impacts the_process_valuepath inmpt_api_client/models/model.py.]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mpt_api_client/models/model.py` around lines 124 - 131, The function _resolve_list_model_class currently only detects list-typed hints when get_origin(target_class) is list and thus misses union/optional forms like list[T] | None; update _resolve_list_model_class to first unwrap Union/Optional by checking if get_origin(target_class) is typing.Union (or types.UnionType) and then iterating its get_args() to find a member whose origin is list, then proceed to extract the list element type and return it if it is a subclass of BaseModel; also apply the same union-unwrapping approach where _process_value inspects list types so Optional[list[ConcreteModel]] is handled correctly by locating the list origin inside the union before resolving element model classes (referencing _resolve_list_model_class and _process_value to find the spots to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Line 129: Keep the existing "# noqa: WPS221" on the conditional in model.py
(the line with "if args and isinstance(args[0], type) and issubclass(args[0],
BaseModel):") and update the Ruff configuration so it no longer flags unknown
WPS ignores (RUF102); specifically add RUF102 to Ruff's ignore list (or enable
accepting external/unknown noqa codes) in your pyproject.toml/.ruff.toml so WPS*
noqa directives are treated as intentional and both linters coexist cleanly.
---
Nitpick comments:
In `@mpt_api_client/models/model.py`:
- Around line 124-131: The function _resolve_list_model_class currently only
detects list-typed hints when get_origin(target_class) is list and thus misses
union/optional forms like list[T] | None; update _resolve_list_model_class to
first unwrap Union/Optional by checking if get_origin(target_class) is
typing.Union (or types.UnionType) and then iterating its get_args() to find a
member whose origin is list, then proceed to extract the list element type and
return it if it is a subclass of BaseModel; also apply the same union-unwrapping
approach where _process_value inspects list types so
Optional[list[ConcreteModel]] is handled correctly by locating the list origin
inside the union before resolving element model classes (referencing
_resolve_list_model_class and _process_value to find the spots to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e4106495-70d6-4821-9850-ee234a0bccca
📒 Files selected for processing (1)
mpt_api_client/models/model.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
mpt_api_client/models/model.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrationmpt_api_client/models/— response model layer and collection wrappersmpt_api_client/rql/— fluent RQL query ...
Files:
mpt_api_client/models/model.py
🧠 Learnings (2)
📚 Learning: 2026-02-17T10:04:00.873Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 210
File: mpt_api_client/rql/query_builder.py:18-18
Timestamp: 2026-02-17T10:04:00.873Z
Learning: In this repository, Ruff and flake8 with wemake-python-styleguide are used together. Do not remove WPS* noqa directives (e.g., WPS231) even if Ruff flags them as unknown in RUF102. Keep the directives to satisfy flake8 rules; ensure tooling configuration accounts for both linters to avoid false positives.
Applied to files:
mpt_api_client/models/model.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
mpt_api_client/models/model.py
🪛 Ruff (0.15.15)
mpt_api_client/models/model.py
[warning] 129-129: Invalid rule code in # noqa: WPS221
Add non-Ruff rule codes to the lint.external configuration option
(RUF102)
jentyk
force-pushed
the
feat/MPT-21922
branch
2 times, most recently
from
3e85e7d to
9c13fa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/helper.py`:
- Line 25: Remove the invalid "# noqa: WPS421" from the teardown print
statements (the lines printing "TEARDOWN - Unable to finalize resource
{resource}: {error.title}" and the similar one at the other occurrence) and
either leave the line without an inline noqa or replace it with a repo-valid
Ruff suppression (e.g., the rule your pyproject.toml uses such as "RUF102") or
move the suppression into the project config; modify the two print lines
referencing resource and error accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 58ba1001-fdd6-411a-a2a9-7c2018f9bc68
📒 Files selected for processing (2)
tests/e2e/helpdesk/conftest.pytests/e2e/helper.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/helpdesk/conftest.py
📜 Review details
⏰ 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 context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/helper.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrationmpt_api_client/models/— response model layer and collection wrappersmpt_api_client/rql/— fluent RQL query ...
Files:
tests/e2e/helper.py
🧠 Learnings (6)
📚 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/helper.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.
Applied to files:
tests/e2e/helper.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/helper.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
tests/e2e/helper.py
🪛 Ruff (0.15.15)
tests/e2e/helper.py
[warning] 25-25: Invalid rule code in # noqa: WPS421
Add non-Ruff rule codes to the lint.external configuration option
(RUF102)
[warning] 32-32: Invalid rule code in # noqa: WPS421
Add non-Ruff rule codes to the lint.external configuration option
(RUF102)
🔇 Additional comments (1)
tests/e2e/helper.py (1)
21-24: LGTM!Also applies to: 28-31, 55-83
Member
Author
|
@coderabbitai repeat Pre-merge checks |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/helper.py`:
- Around line 21-25: The _finalize_async_resource helper only catches
MPTAPIError so non-MPTAPIError exceptions escape and can fail tests; change the
exception handling in _finalize_async_resource (and the analogous synchronous
finalize wrapper around lines 28-32) to catch Exception (or a broad base error)
instead of only MPTAPIError and log the failure (include the resource identifier
and full error info/traceback or error message) at warning level so finalize
remains best-effort and cannot fail the test run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9b2bd32e-7454-4dad-8713-6f3b7ec4a672
📒 Files selected for processing (2)
tests/e2e/helpdesk/conftest.pytests/e2e/helper.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/helpdesk/conftest.py
📜 Review details
⏰ 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 context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/helper.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrationmpt_api_client/models/— response model layer and collection wrappersmpt_api_client/rql/— fluent RQL query ...
Files:
tests/e2e/helper.py
🧠 Learnings (6)
📚 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/helper.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.
Applied to files:
tests/e2e/helper.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/helper.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
tests/e2e/helper.py
Member
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Member
Author
|
@coderabbitai it seems I addressed all your comments and requests. Is anything left? If not, approve or re-review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch open review comments on PR `#341`
gh pr view 341 --repo softwareone-platform/mpt-api-python-client --json reviews,reviewRequests,state
# List review comments (not resolved)
gh api repos/softwareone-platform/mpt-api-python-client/pulls/341/comments --jq '.[] | {id: .id, path: .path, body: .body, line: .line, resolved: .resolved}'Length of output: 38650 🏁 Script executed: #!/bin/bash
# Check current branch
git branch --show-current
# Check the latest commits
git log --oneline -10
# Check the current state of helper.py
cat tests/e2e/helper.py
# Check the relevant section of model.py
sed -n '120,135p' mpt_api_client/models/model.pyLength of output: 4856
Everything looks good — approving the PR. [approve] |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/helper.py (1)
7-18: ⚖️ Poor tradeoffConsider aligning delete wrappers with the finalize pattern.
For consistency, the existing delete wrappers could adopt the same pattern as the new finalize helpers: use
logger.warning()withexc_info=Trueand catch broadExceptionfor best-effort teardown. However, this is outside the current PR scope and can be deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/helper.py` around lines 7 - 18, Update the teardown helpers _delete_async_resource and _delete_resource to follow the finalize pattern: replace the print(...) calls with logger.warning(...) and pass exc_info=True, and broaden the except clause from MPTAPIError to Exception so the deletions are best-effort and log stack traces; locate these changes inside the functions named _delete_async_resource and _delete_resource and ensure the log message still includes the resource representation for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/e2e/helper.py`:
- Around line 7-18: Update the teardown helpers _delete_async_resource and
_delete_resource to follow the finalize pattern: replace the print(...) calls
with logger.warning(...) and pass exc_info=True, and broaden the except clause
from MPTAPIError to Exception so the deletions are best-effort and log stack
traces; locate these changes inside the functions named _delete_async_resource
and _delete_resource and ensure the log message still includes the resource
representation for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 96c8ad4b-4ca9-45ea-b72d-2eb61e0f3d6b
📒 Files selected for processing (3)
docs/e2e_tests.mdtests/e2e/helpdesk/conftest.pytests/e2e/helper.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
✅ Files skipped from review due to trivial changes (1)
- docs/e2e_tests.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/helpdesk/conftest.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/helper.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrationmpt_api_client/models/— response model layer and collection wrappersmpt_api_client/rql/— fluent RQL query ...
Files:
tests/e2e/helper.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-api-python-client
Timestamp: 2026-06-11T14:06:28.839Z
Learning: End-to-end tests must exercise the running MPT API and cover the full request/response lifecycle
Learnt from: CR
Repo: softwareone-platform/mpt-api-python-client
Timestamp: 2026-06-11T14:06:28.839Z
Learning: Required environment variables must be set before invoking the E2E test suite: `MPT_API_BASE_URL`, `MPT_API_TOKEN_VENDOR`, `MPT_API_TOKEN_CLIENT`, `MPT_API_TOKEN_OPERATIONS`
Learnt from: CR
Repo: softwareone-platform/mpt-api-python-client
Timestamp: 2026-06-11T14:06:28.839Z
Learning: Optional ReportPortal integration can be configured via environment variables: `RP_API_KEY`, `RP_ENDPOINT`, `RP_LAUNCH`
📚 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/helper.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.
Applied to files:
tests/e2e/helper.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/helper.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.
Applied to files:
tests/e2e/helper.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
tests/e2e/helper.py
🪛 Ruff (0.15.15)
tests/e2e/helper.py
[warning] 24-24: Do not catch blind exception: Exception
(BLE001)
[warning] 31-31: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
tests/e2e/helper.py (3)
21-32: LGTM!Both previous review comments have been fully addressed:
- ✅ Replaced
print()withlogger.warning()(no invalid# noqa: WPS421)- ✅ Now catch broad
Exceptioninstead of onlyMPTAPIErrorThe finalize wrappers correctly implement best-effort teardown with full traceback logging (
exc_info=True) and match the documented contract.Note on Ruff BLE001: The broad
Exceptioncatch is intentional per PR objectives ("Failures during finalize are logged at warning level with traceback and do not cause test failures"). If desired, you may add# noqa: BLE001with an explanatory comment, but it's optional since the design is documented.
56-67: LGTM!
71-82: LGTM!
E2E helpdesk fixtures left resources in non-terminal states once a test finished: cases were created with no teardown at all (cases cannot be deleted), and a queue carrying an active case could not be deleted, leaving both lingering active in the shared test environment. Add best-effort terminal-state teardown helpers (create_fixture_resource_and_finalize and the async variant) that mirror the existing delete-teardown convention, and wire the helpdesk fixtures to use them: queues are disabled and cases are completed on test completion. Failures are logged, never raised, so teardown cannot fail a test. Forms and parameters keep their existing delete teardown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
robcsegal
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-21922
tests/e2e/helper.py
tests/e2e/helpdesk/conftest.py
mpt_api_client/models/model.py
Purpose: ensure E2E tests finalize stateful helpdesk resources (queues/cases) to terminal states on teardown while keeping teardown failures non-fatal; internal model refactor improves clarity of nested-dict/list processing.