Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds Helpdesk API surface: Chat model, synchronous and asynchronous Chats services, Helpdesk/AsyncHelpdesk wrappers, client properties exposing helpdesk, resource exports, e2e fixtures/tests, and unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
jentyk
force-pushed
the
feat/MPT-18369
branch
2 times, most recently
from
196539a to
32f8572
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/unit/resources/helpdesk/test_helpdesk.py (1)
27-33: Strengthen property tests by asserting client wiring.You already validate service type; also assert that the created service keeps the same client instance.
💡 Proposed improvement
def test_helpdesk_properties(http_client, attr_name, expected): helpdesk = Helpdesk(http_client=http_client) result = getattr(helpdesk, attr_name) assert isinstance(result, expected) + assert result.http_client is http_client @@ def test_async_helpdesk_properties(async_http_client, attr_name, expected): helpdesk = AsyncHelpdesk(http_client=async_http_client) result = getattr(helpdesk, attr_name) assert isinstance(result, expected) + assert result.http_client is async_http_clientAlso applies to: 41-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/helpdesk/test_helpdesk.py` around lines 27 - 33, The test_helpdesk_properties test creates a Helpdesk via Helpdesk(http_client=...), but only asserts the service type; update the test to also assert the returned service instance retains the same client instance (e.g., check the service's client attribute or internal _client) by adding an assertion that result.client (or result._client) is http_client; apply the same additional assertion to the other similar test (the one at lines ~41-46) so both verify client wiring.tests/e2e/helpdesk/chats/test_sync_chats.py (1)
32-36: Makenot_foundassertion stricter.Assert expected status code so the test fails on unrelated
MPTAPIErrorcases.💡 Proposed assertion tightening
def test_not_found(mpt_ops, invalid_chat_id): service = mpt_ops.helpdesk.chats - with pytest.raises(MPTAPIError): + with pytest.raises(MPTAPIError) as exc_info: service.get(invalid_chat_id) + assert exc_info.value.status_code == 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/test_sync_chats.py` around lines 32 - 36, The test_not_found in tests/e2e/helpdesk/chats/test_sync_chats.py currently only asserts that service.get(invalid_chat_id) raises MPTAPIError; tighten it by capturing the exception (using pytest.raises as excinfo) and then assert the raised MPTAPIError's status_code equals the expected not-found code (e.g., 404) to ensure you fail on unrelated MPTAPIError cases; locate the test_not_found function and modify the pytest.raises usage around service.get to perform the additional excinfo.value.status_code == 404 assertion.tests/unit/resources/helpdesk/test_chats.py (1)
16-33: Prefer callable checks overhasattrfor API method tests.
hasattronly checks attribute existence. For method-contract tests,callable(getattr(...))is stricter.💡 Proposed improvement
def test_mixins_present(chats_service, method): - result = hasattr(chats_service, method) - - assert result is True + result = getattr(chats_service, method, None) + assert callable(result) @@ def test_async_mixins_present(async_chats_service, method): - result = hasattr(async_chats_service, method) - - assert result is True + result = getattr(async_chats_service, method, None) + assert callable(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/helpdesk/test_chats.py` around lines 16 - 33, Replace the fragile hasattr checks in tests test_mixins_present and test_async_mixins_present with callable assertions: use callable(getattr(chats_service, method, None)) and callable(getattr(async_chats_service, method, None)) respectively so the tests verify the attribute is an actual callable method (use getattr default None to avoid AttributeError).tests/e2e/helpdesk/chats/test_async_chats.py (2)
35-40: Assert the 404 contract, not only exception type.
pytest.raises(MPTAPIError)alone can pass on unrelated API failures; check status details too.💡 Proposed assertion tightening
async def test_not_found(async_mpt_ops, invalid_chat_id): service = async_mpt_ops.helpdesk.chats - with pytest.raises(MPTAPIError): + with pytest.raises(MPTAPIError) as exc_info: await service.get(invalid_chat_id) + assert exc_info.value.status_code == 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/test_async_chats.py` around lines 35 - 40, The test_not_found currently only checks that an MPTAPIError is raised; tighten it to assert the 404 contract by using pytest.raises(MPTAPIError) as excinfo around await async_mpt_ops.helpdesk.chats.get(invalid_chat_id) and then assert the exception contains the HTTP 404 info (e.g., excinfo.value.status == 404 or excinfo.value.status_code == 404 or excinfo.value.response.status_code == 404) and optionally assert the error message contains "Not Found" to ensure the failure is the expected 404 not some other API error.
6-36: Remove redundant@pytest.mark.asynciodecorators.The project uses
asyncio_mode = "auto"in pyproject.toml, which automatically detects and runs async test functions without requiring explicit markers.Proposed cleanup
-@pytest.mark.asyncio async def test_get_chat(async_mpt_ops, chat_id): -@pytest.mark.asyncio async def test_list_chats(async_mpt_ops): -@pytest.mark.asyncio async def test_update_chat(async_mpt_ops, chat_id, short_uuid): -@pytest.mark.asyncio async def test_not_found(async_mpt_ops, invalid_chat_id):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/test_async_chats.py` around lines 6 - 36, Tests in tests/e2e/helpdesk/chats/test_async_chats.py are redundantly decorated with `@pytest.mark.asyncio`; remove the decorator lines from the async test functions test_get_chat, test_list_chats, test_update_chat, and test_not_found so they rely on pytest's asyncio_mode="auto" detection; keep the async function signatures and awaits unchanged (locate the decorator above each function name to delete).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/helpdesk/chats/test_async_chats.py`:
- Around line 35-40: The test_not_found currently only checks that an
MPTAPIError is raised; tighten it to assert the 404 contract by using
pytest.raises(MPTAPIError) as excinfo around await
async_mpt_ops.helpdesk.chats.get(invalid_chat_id) and then assert the exception
contains the HTTP 404 info (e.g., excinfo.value.status == 404 or
excinfo.value.status_code == 404 or excinfo.value.response.status_code == 404)
and optionally assert the error message contains "Not Found" to ensure the
failure is the expected 404 not some other API error.
- Around line 6-36: Tests in tests/e2e/helpdesk/chats/test_async_chats.py are
redundantly decorated with `@pytest.mark.asyncio`; remove the decorator lines from
the async test functions test_get_chat, test_list_chats, test_update_chat, and
test_not_found so they rely on pytest's asyncio_mode="auto" detection; keep the
async function signatures and awaits unchanged (locate the decorator above each
function name to delete).
In `@tests/e2e/helpdesk/chats/test_sync_chats.py`:
- Around line 32-36: The test_not_found in
tests/e2e/helpdesk/chats/test_sync_chats.py currently only asserts that
service.get(invalid_chat_id) raises MPTAPIError; tighten it by capturing the
exception (using pytest.raises as excinfo) and then assert the raised
MPTAPIError's status_code equals the expected not-found code (e.g., 404) to
ensure you fail on unrelated MPTAPIError cases; locate the test_not_found
function and modify the pytest.raises usage around service.get to perform the
additional excinfo.value.status_code == 404 assertion.
In `@tests/unit/resources/helpdesk/test_chats.py`:
- Around line 16-33: Replace the fragile hasattr checks in tests
test_mixins_present and test_async_mixins_present with callable assertions: use
callable(getattr(chats_service, method, None)) and
callable(getattr(async_chats_service, method, None)) respectively so the tests
verify the attribute is an actual callable method (use getattr default None to
avoid AttributeError).
In `@tests/unit/resources/helpdesk/test_helpdesk.py`:
- Around line 27-33: The test_helpdesk_properties test creates a Helpdesk via
Helpdesk(http_client=...), but only asserts the service type; update the test to
also assert the returned service instance retains the same client instance
(e.g., check the service's client attribute or internal _client) by adding an
assertion that result.client (or result._client) is http_client; apply the same
additional assertion to the other similar test (the one at lines ~41-46) so both
verify client wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bd2d0f2c-aa7d-43f0-b9fa-fb0503ae79a4
📒 Files selected for processing (15)
e2e_config.test.jsonmpt_api_client/mpt_client.pympt_api_client/resources/__init__.pympt_api_client/resources/helpdesk/__init__.pympt_api_client/resources/helpdesk/chats.pympt_api_client/resources/helpdesk/helpdesk.pytests/e2e/helpdesk/__init__.pytests/e2e/helpdesk/chats/__init__.pytests/e2e/helpdesk/chats/conftest.pytests/e2e/helpdesk/chats/test_async_chats.pytests/e2e/helpdesk/chats/test_sync_chats.pytests/unit/resources/helpdesk/__init__.pytests/unit/resources/helpdesk/test_chats.pytests/unit/resources/helpdesk/test_helpdesk.pytests/unit/test_mpt_client.py
albertsola
approved these changes
Mar 13, 2026
| from mpt_api_client.exceptions import MPTAPIError | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
Contributor
There was a problem hiding this comment.
we don't mark those as asyncio
albertsola
approved these changes
Mar 13, 2026
…rt, including E2E and unit tests
|
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-18371
Release Notes
HelpdeskandAsyncHelpdeskentry-point classes and exported them from the resources package; integrated intoMPTClientandAsyncMPTClientvia newhelpdeskproperties.ChatsServiceandAsyncChatsServiceproviding create, get, update and list (collection) operations backed by the/public/v1/helpdesk/chatsendpoint.Chatmodel representing helpdesk chat entities.mpt_api_client/resources/helpdeskand submodules).e2e_config.test.jsonentryhelpdesk.chat.idand pytest fixtures (chat_id,invalid_chat_id) for e2e tests.