MPT-19486 refactor fixtures and unskip chat attachment tests#268
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoved helpdesk case fixtures from per-directory to parent conftest and reintroduced case lifecycle fixtures there; changed chat attachment and message fixtures to accept Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py (1)
47-50:⚠️ Potential issue | 🟡 MinorPipeline failure: Missing
# noqa: AAA01ontest_delete_chat_attachment.The pipeline is failing with flake8 AAA01 violation on line 47. This test has the same pattern as
test_create_chat_attachment(no explicit Act block), but is missing the# noqa: AAA01comment. Removing the skip decorator exposed this pre-existing lint issue.Proposed fix
-def test_delete_chat_attachment(chat_attachments_service, chat_attachment_data, pdf_fd): +def test_delete_chat_attachment(chat_attachments_service, chat_attachment_data, pdf_fd): # noqa: AAA01🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py` around lines 47 - 50, Add the flake8 exception to the test definition for test_delete_chat_attachment by appending "# noqa: AAA01" to the test function line so it matches the pattern used in test_create_chat_attachment; this suppresses the AAA01 lint for the missing explicit Act block while keeping the current usages of chat_attachments_service, chat_attachment_data, and pdf_fd and the delete call on created.id unchanged.
🧹 Nitpick comments (2)
tests/e2e/helpdesk/conftest.py (2)
58-59: Unusedshort_uuiddependency incase_factory.The
case_factoryfixture declaresshort_uuidas a dependency but doesn't use it anywhere in the factory function.Proposed fix
Either remove the unused dependency:
`@pytest.fixture` -def case_factory(short_uuid, created_queue): +def case_factory(created_queue):Or use it to make case data unique (if intended):
def factory( title: str = "E2E Created Helpdesk Case", description: str = "E2E Created Helpdesk Case Description", ): return { + "title": f"{title} - {short_uuid}", + "description": f"{description} - {short_uuid}", "queue": {"id": created_queue.id}, "chat": {"lastMessage": {"content": "E2E testing!!!"}}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/conftest.py` around lines 58 - 59, The case_factory fixture declares an unused dependency short_uuid; either remove short_uuid from the case_factory parameter list to eliminate the unused fixture, or actually use it inside case_factory (e.g., incorporate short_uuid into generated case data/IDs or the payload created by the factory) so the dependency is meaningful; locate the fixture named case_factory in conftest.py and update its signature and/or its case creation logic accordingly (references: case_factory, short_uuid, created_queue).
58-69: Unused parameterstitleanddescriptionincase_factory.The factory function accepts
titleanddescriptionparameters but never uses them in the returned dictionary. This appears to be dead code or an incomplete implementation.Proposed fix: Either use the parameters or remove them
Option 1 - Use the parameters:
`@pytest.fixture` def case_factory(short_uuid, created_queue): def factory( title: str = "E2E Created Helpdesk Case", description: str = "E2E Created Helpdesk Case Description", ): return { + "title": title, + "description": description, "queue": {"id": created_queue.id}, "chat": {"lastMessage": {"content": "E2E testing!!!"}}, } return factoryOption 2 - Remove unused parameters:
`@pytest.fixture` def case_factory(short_uuid, created_queue): - def factory( - title: str = "E2E Created Helpdesk Case", - description: str = "E2E Created Helpdesk Case Description", - ): + def factory(): return { "queue": {"id": created_queue.id}, "chat": {"lastMessage": {"content": "E2E testing!!!"}}, } return factory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/conftest.py` around lines 58 - 69, The factory function case_factory -> inner factory currently accepts title and description but doesn't use them; either remove those unused parameters from factory(title, description) to avoid dead code, or update the returned dict to include them (e.g., include "title": title and "description": description in the dict returned by factory) so the parameters are applied when tests call case_factory; refer to the case_factory and its inner factory and the returned structure that currently uses created_queue and chat to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py`:
- Around line 47-50: Add the flake8 exception to the test definition for
test_delete_chat_attachment by appending "# noqa: AAA01" to the test function
line so it matches the pattern used in test_create_chat_attachment; this
suppresses the AAA01 lint for the missing explicit Act block while keeping the
current usages of chat_attachments_service, chat_attachment_data, and pdf_fd and
the delete call on created.id unchanged.
---
Nitpick comments:
In `@tests/e2e/helpdesk/conftest.py`:
- Around line 58-59: The case_factory fixture declares an unused dependency
short_uuid; either remove short_uuid from the case_factory parameter list to
eliminate the unused fixture, or actually use it inside case_factory (e.g.,
incorporate short_uuid into generated case data/IDs or the payload created by
the factory) so the dependency is meaningful; locate the fixture named
case_factory in conftest.py and update its signature and/or its case creation
logic accordingly (references: case_factory, short_uuid, created_queue).
- Around line 58-69: The factory function case_factory -> inner factory
currently accepts title and description but doesn't use them; either remove
those unused parameters from factory(title, description) to avoid dead code, or
update the returned dict to include them (e.g., include "title": title and
"description": description in the dict returned by factory) so the parameters
are applied when tests call case_factory; refer to the case_factory and its
inner factory and the returned structure that currently uses created_queue and
chat to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 715f4330-7dc5-4cbc-be94-d7dfa468d363
📒 Files selected for processing (5)
tests/e2e/helpdesk/cases/conftest.pytests/e2e/helpdesk/chats/attachment/conftest.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/e2e/helpdesk/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/helpdesk/cases/conftest.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/helpdesk/chats/attachment/test_async_attachment.py (1)
18-20: Consider addingAAA01to Ruff's external rules configuration.The
# noqa: AAA01comment suppresses a pytest-aaa linter rule that Ruff doesn't recognize, causing an RUF102 warning. To silence this, addAAA01to thelint.externallist inpyproject.toml.Add to
pyproject.tomlunder[tool.ruff.lint]:external = ["AAA01"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_async_attachment.py` around lines 18 - 20, The test function test_create_chat_attachment currently uses a pytest-aaa suppression comment "# noqa: AAA01" which Ruff flags as unknown (RUF102); to fix, add the rule ID "AAA01" to Ruff's external rules list by updating the project's pyproject.toml under the [tool.ruff.lint] section to include AAA01 in the external array so Ruff recognizes the suppression and the RUF102 warning disappears.tests/e2e/helpdesk/conftest.py (1)
58-69:case_factoryhas unused parameters and fixture dependency.The
titleanddescriptionparameters are accepted but never used in the returned dictionary. Additionally, theshort_uuidfixture dependency is unused. Either incorporate these into the payload or remove them.♻️ Proposed fix to use or remove unused elements
Option 1 - Use the parameters:
`@pytest.fixture` -def case_factory(short_uuid, created_queue): +def case_factory(created_queue): def factory( title: str = "E2E Created Helpdesk Case", description: str = "E2E Created Helpdesk Case Description", ): return { + "title": title, + "description": description, "queue": {"id": created_queue.id}, "chat": {"lastMessage": {"content": "E2E testing!!!"}}, } return factoryOption 2 - Remove unused parameters if not needed:
`@pytest.fixture` -def case_factory(short_uuid, created_queue): - def factory( - title: str = "E2E Created Helpdesk Case", - description: str = "E2E Created Helpdesk Case Description", - ): +def case_factory(created_queue): + def factory(): return { "queue": {"id": created_queue.id}, "chat": {"lastMessage": {"content": "E2E testing!!!"}}, } return factory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/conftest.py` around lines 58 - 69, The case_factory fixture currently accepts title, description and depends on short_uuid but neither title/description nor short_uuid are used; either remove the unused parameters and the short_uuid fixture dependency from case_factory, or (preferred) incorporate them into the returned payload by using title and description in the case/chat payload and using short_uuid to generate a unique title/identifier (e.g., append short_uuid to title or include it as a field) so the returned dict uses "title", "description" and/or a unique id along with the existing "queue" and "chat" keys; update the fixture signature and returned dict in the case_factory function accordingly.
🤖 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/attachment/test_async_attachment.py`:
- Around line 18-20: The test function test_create_chat_attachment currently
uses a pytest-aaa suppression comment "# noqa: AAA01" which Ruff flags as
unknown (RUF102); to fix, add the rule ID "AAA01" to Ruff's external rules list
by updating the project's pyproject.toml under the [tool.ruff.lint] section to
include AAA01 in the external array so Ruff recognizes the suppression and the
RUF102 warning disappears.
In `@tests/e2e/helpdesk/conftest.py`:
- Around line 58-69: The case_factory fixture currently accepts title,
description and depends on short_uuid but neither title/description nor
short_uuid are used; either remove the unused parameters and the short_uuid
fixture dependency from case_factory, or (preferred) incorporate them into the
returned payload by using title and description in the case/chat payload and
using short_uuid to generate a unique title/identifier (e.g., append short_uuid
to title or include it as a field) so the returned dict uses "title",
"description" and/or a unique id along with the existing "queue" and "chat"
keys; update the fixture signature and returned dict in the case_factory
function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: de4ae8c1-432a-4c6c-b336-9eae29edd167
📒 Files selected for processing (5)
tests/e2e/helpdesk/cases/conftest.pytests/e2e/helpdesk/chats/attachment/conftest.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/e2e/helpdesk/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/helpdesk/cases/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/helpdesk/chats/attachment/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py (1)
18-21: Invalid# noqarule code for Ruff.
AAA01is a flake8-aaa rule code, not recognized by Ruff. Either add"AAA01"tolint.externalinpyproject.toml, or remove the comment if flake8-aaa isn't part of the linting pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py` around lines 18 - 21, The inline noqa comment using rule code "AAA01" in the test function test_create_chat_attachment is not recognized by Ruff; either remove the invalid "# noqa: AAA01" from the test declaration (function test_create_chat_attachment) or register the flake8-aaa rule by adding "AAA01" under lint.external in pyproject.toml so Ruff knows to ignore it; locate the test in tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py and update either the test header or the project lint config accordingly, keeping references to created_chat_attachment and chat_attachment_data intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpdesk/conftest.py`:
- Around line 58-69: The fixture case_factory currently injects short_uuid but
never uses it and defines factory parameters title and description that are not
included in the returned dict; either remove the unused short_uuid parameter
from the case_factory signature or reference it inside factory, and if title and
description are intended, add them into the returned dictionary (e.g., include
"title": title and "description": description) so factory uses its parameters;
update the case_factory and nested factory definitions accordingly (look for the
case_factory fixture and the inner factory function).
---
Nitpick comments:
In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py`:
- Around line 18-21: The inline noqa comment using rule code "AAA01" in the test
function test_create_chat_attachment is not recognized by Ruff; either remove
the invalid "# noqa: AAA01" from the test declaration (function
test_create_chat_attachment) or register the flake8-aaa rule by adding "AAA01"
under lint.external in pyproject.toml so Ruff knows to ignore it; locate the
test in tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py and update
either the test header or the project lint config accordingly, keeping
references to created_chat_attachment and chat_attachment_data intact.
🪄 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: abbe6227-38ae-4677-b683-f2b590f87254
📒 Files selected for processing (6)
tests/e2e/helpdesk/cases/conftest.pytests/e2e/helpdesk/chats/attachment/conftest.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/e2e/helpdesk/chats/messages/conftest.pytests/e2e/helpdesk/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/helpdesk/cases/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/helpdesk/chats/attachment/conftest.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/helpdesk/chats/attachment/test_async_attachment.py (1)
18-18: Ruff flagsAAA01as an unrecognized rule code.The
AAA01rule is from thepytest-aaaplugin (Arrange-Act-Assert pattern checker), not a native Ruff rule. To suppress the RUF102 warning, add"AAA01"to thelint.externallist inpyproject.toml.This is a minor configuration issue and doesn't affect test execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_async_attachment.py` at line 18, Ruff is flagging the AAA01 ignore in test_create_chat_attachment because AAA01 is from pytest-aaa (not a built-in Ruff rule); open pyproject.toml and add "AAA01" to the lint.external list so Ruff recognizes external rule codes (e.g., ensure lint.external includes "AAA01"), then keep the noqa on the test function def test_create_chat_attachment(async_created_chat_attachment, chat_attachment_data) to suppress RUF102 warnings.tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py (1)
18-18: SameAAA01external rule code issue.Same as the async test file—consider adding
"AAA01"tolint.externalinpyproject.tomlto suppress RUF102 warnings across both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py` at line 18, The test function test_create_chat_attachment in tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py triggers the external rule AAA01; to suppress the RUF102 warnings consistently with the async counterpart, add "AAA01" to the lint.external list in pyproject.toml (so the linter treats that external rule as allowed across both sync and async test files).
🤖 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/attachment/test_async_attachment.py`:
- Line 18: Ruff is flagging the AAA01 ignore in test_create_chat_attachment
because AAA01 is from pytest-aaa (not a built-in Ruff rule); open pyproject.toml
and add "AAA01" to the lint.external list so Ruff recognizes external rule codes
(e.g., ensure lint.external includes "AAA01"), then keep the noqa on the test
function def test_create_chat_attachment(async_created_chat_attachment,
chat_attachment_data) to suppress RUF102 warnings.
In `@tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py`:
- Line 18: The test function test_create_chat_attachment in
tests/e2e/helpdesk/chats/attachment/test_sync_attachment.py triggers the
external rule AAA01; to suppress the RUF102 warnings consistently with the async
counterpart, add "AAA01" to the lint.external list in pyproject.toml (so the
linter treats that external rule as allowed across both sync and async test
files).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 752303a0-bf67-46bb-a791-d8693500f8ff
📒 Files selected for processing (6)
tests/e2e/helpdesk/cases/conftest.pytests/e2e/helpdesk/chats/attachment/conftest.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/e2e/helpdesk/chats/messages/conftest.pytests/e2e/helpdesk/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/helpdesk/cases/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/helpdesk/chats/messages/conftest.py
- tests/e2e/helpdesk/chats/attachment/conftest.py
- tests/e2e/helpdesk/conftest.py
|
robcsegal
approved these changes
Apr 1, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/helpdesk/chats/attachment/test_async_attachment.py (1)
18-18: Address the invalid# noqa: AAA01directive.Ruff reports
AAA01is not a recognized rule code. This appears to be from thepytest-aaaplugin. Per the static analysis hint, addAAA01to thelint.externalconfiguration inpyproject.tomlto suppress this warning, or remove the directive if pytest-aaa is not in use.Option 1: Add to pyproject.toml
[tool.ruff.lint] external = ["AAA01"]Option 2: Remove the directive if pytest-aaa is not used
-def test_create_chat_attachment(async_created_chat_attachment, chat_attachment_data): # noqa: AAA01 +def test_create_chat_attachment(async_created_chat_attachment, chat_attachment_data):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/attachment/test_async_attachment.py` at line 18, The test function test_create_chat_attachment contains an invalid Ruff noqa directive "# noqa: AAA01" (AAA01 is not a Ruff rule); either remove the directive from the test function definition (test_create_chat_attachment) if pytest-aaa is not used, or add "AAA01" to Ruff's external rules by updating the project config (add AAA01 to the tool.ruff.lint.external list in pyproject.toml) so Ruff recognizes the pytest-aaa code.
🤖 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/attachment/test_async_attachment.py`:
- Line 18: The test function test_create_chat_attachment contains an invalid
Ruff noqa directive "# noqa: AAA01" (AAA01 is not a Ruff rule); either remove
the directive from the test function definition (test_create_chat_attachment) if
pytest-aaa is not used, or add "AAA01" to Ruff's external rules by updating the
project config (add AAA01 to the tool.ruff.lint.external list in pyproject.toml)
so Ruff recognizes the pytest-aaa code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c76b8b21-7f82-449f-ab30-eeba7f21f811
📒 Files selected for processing (6)
tests/e2e/helpdesk/cases/conftest.pytests/e2e/helpdesk/chats/attachment/conftest.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/e2e/helpdesk/chats/messages/conftest.pytests/e2e/helpdesk/conftest.py
💤 Files with no reviewable changes (1)
- tests/e2e/helpdesk/cases/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/helpdesk/chats/messages/conftest.py
- tests/e2e/helpdesk/chats/attachment/conftest.py
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-19486