MPT-14935 add E2E tests for notifications messages endpoint #139
Conversation
WalkthroughAdded an e2e configuration key for a message ID and introduced pytest fixtures plus synchronous and asynchronous end-to-end tests for the notifications/messages API (including success, pagination, and not-found cases). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
jentyk
force-pushed
the
feat/MPT-14935
branch
from
28a01e7 to
1fcd75f
Compare
jentyk
requested review from
albertsola,
alephsur,
d3rky,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/notifications/messages/test_async_messages.py (1)
8-27: Align asynctest_get_messagebehavior with sync test whenmessage_idis missingRight now, if
message_idis not configured (fixture returnsNone),test_get_messagewill try to callservice.get(None)and fail rather than skipping like the sync test does. To keep behavior consistent and make tests robust to missing config, consider:async def test_get_message(async_mpt_client, message_id): - service = async_mpt_client.notifications.messages - - result = await service.get(message_id) - - assert result.id == message_id + if message_id is None: + pytest.skip("Message ID not configured in e2e_config") + + service = async_mpt_client.notifications.messages + + result = await service.get(message_id) + + assert result.id == message_idThe other async tests look fine and will benefit from the
invalid_message_idfixture fix suggested in the conftest review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/messages/conftest.py(1 hunks)tests/e2e/notifications/messages/test_async_messages.py(1 hunks)tests/e2e/notifications/messages/test_sync_messages.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/notifications/messages/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/messages/test_sync_messages.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
mpt_client(43-44)mpt_ops(31-32)tests/e2e/notifications/messages/conftest.py (2)
message_id(19-20)invalid_message_id(24-25)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/messages/conftest.py (1)
tests/e2e/conftest.py (3)
short_uuid(101-102)account_id(122-123)e2e_config(89-92)
🪛 GitHub Actions: PR build and merge
tests/e2e/notifications/messages/test_sync_messages.py
[error] 11-11: wemake-python-styleguide: AAA05 blank line in block. Run lint to fix style; associated with test_sync_messages.py. Command step: docker compose run --service-ports app_test
🔇 Additional comments (2)
e2e_config.test.json (1)
23-24: Config entry fornotifications.message.idmatches fixture usageThe new
notifications.message.idkey is consistent with the fixtures and keeps the JSON valid after adding the trailing comma on the preceding line. No issues here.tests/e2e/conftest.py (1)
121-123: Centralizingaccount_idfixture at top-level conftest looks goodThe
account_idfixture mirrors the existinguser_idpattern and correctly pulls frome2e_config["accounts.account.id"], making it reusable across E2E tests.
jentyk
force-pushed
the
feat/MPT-14935
branch
2 times, most recently
from
4fc33c8 to
9b6d2ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/notifications/messages/conftest.py (2)
18-20: Prefer direct dictionary access for consistency and clearer errors.The
message_idfixture uses.get(), which silently returnsNoneif the key is missing, potentially causing confusing test failures downstream. For consistency with theaccount_idfixture pattern (which usese2e_config["accounts.account.id"]) and to get immediate, clear feedback when required config is missing, use direct dictionary access.Apply this diff:
@pytest.fixture def message_id(e2e_config): - return e2e_config.get("notifications.message.id") + return e2e_config["notifications.message.id"]
23-25: Remove unusede2e_configparameter.The
invalid_message_idfixture returns a hard-coded value and doesn't use thee2e_configparameter.Apply this diff:
@pytest.fixture -def invalid_message_id(e2e_config): +def invalid_message_id(): return "MSG-000-000-000-000"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/messages/conftest.py(1 hunks)tests/e2e/notifications/messages/test_async_messages.py(1 hunks)tests/e2e/notifications/messages/test_sync_messages.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e_config.test.json
- tests/e2e/notifications/messages/test_async_messages.py
- tests/e2e/conftest.py
- tests/e2e/notifications/messages/test_sync_messages.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/notifications/messages/conftest.py
🧬 Code graph analysis (1)
tests/e2e/notifications/messages/conftest.py (1)
tests/e2e/conftest.py (3)
short_uuid(101-102)account_id(122-123)e2e_config(89-92)
🪛 Ruff (0.14.6)
tests/e2e/notifications/messages/conftest.py
24-24: Unused function argument: e2e_config
(ARG001)
⏰ 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 (1)
tests/e2e/notifications/messages/conftest.py (1)
4-15: LGTM!The
message_datafixture is well-structured with appropriate test data and correct fixture dependencies.
albertsola
left a comment
albertsola
left a comment
There was a problem hiding this comment.
Not 100% sure about moving account_id a level up.
jentyk
force-pushed
the
feat/MPT-14935
branch
2 times, most recently
from
b591c76 to
ed3b678
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/notifications/messages/conftest.py (1)
23-25: Remove unusede2e_configargument frominvalid_message_idfixture
e2e_configisn’t used here and Ruff flags it as ARG001, so this fixture needn’t depend on that fixture at all.-@pytest.fixture -def invalid_message_id(e2e_config): - return "MSG-000-000-000-000" +@pytest.fixture +def invalid_message_id(): + return "MSG-000-000-000-000"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e_config.test.json(1 hunks)tests/e2e/notifications/messages/conftest.py(1 hunks)tests/e2e/notifications/messages/test_async_messages.py(1 hunks)tests/e2e/notifications/messages/test_sync_messages.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/notifications/messages/test_sync_messages.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/notifications/messages/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/messages/test_async_messages.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/messages/test_sync_messages.py (3)
test_get_message(8-15)test_list_messages(18-23)test_not_found(26-30)tests/e2e/conftest.py (2)
async_mpt_client(48-51)async_mpt_ops(36-39)tests/e2e/notifications/messages/conftest.py (2)
message_id(19-20)invalid_message_id(24-25)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/messages/conftest.py (2)
tests/e2e/conftest.py (2)
short_uuid(101-102)e2e_config(89-92)tests/e2e/accounts/conftest.py (1)
account_id(22-23)
🪛 Ruff (0.14.7)
tests/e2e/notifications/messages/conftest.py
24-24: Unused function argument: e2e_config
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
tests/e2e/notifications/messages/test_async_messages.py (1)
16-28: Async list/not-found coverage looks solid and mirrors sync testsThe
test_list_messagesandtest_not_foundcases correctly exercise page fetching and 404 handling viaMPTAPIError, matching the sync behaviors with the async client.tests/e2e/notifications/messages/conftest.py (2)
4-15:message_datafixture is clear and reusablePayload structure (title/content/category/priority/account/externalIds) looks good for E2E creation and later cleanup via the short UUID tag.
18-20:message_idfixture matches optional-config patternUsing
.get("notifications.message.id")is consistent with tests that skip when the ID is not configured, keeping the fixture flexible across environments.
reverted |
jentyk
force-pushed
the
feat/MPT-14935
branch
from
ed3b678 to
2f32d3f
Compare
jentyk
force-pushed
the
feat/MPT-14935
branch
from
2f32d3f to
33d5413
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/notifications/messages/conftest.py (1)
18-20: Consider failing fast ifnotifications.message.idis missingElsewhere (e.g.,
account_idintests/e2e/accounts/conftest.py) the config is accessed with[], which will fail loudly if the key is missing. For consistency and clearer failures, you might prefer:@pytest.fixture def message_id(e2e_config): - return e2e_config.get("notifications.message.id") + return e2e_config["notifications.message.id"]This is optional, but it keeps behavior consistent across fixtures and surfaces misconfigured test environments earlier.
Please confirm that all your test configs define
"notifications.message.id"before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e_config.test.json(1 hunks)tests/e2e/notifications/messages/conftest.py(1 hunks)tests/e2e/notifications/messages/test_async_messages.py(1 hunks)tests/e2e/notifications/messages/test_sync_messages.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/notifications/messages/test_sync_messages.py
- tests/e2e/notifications/messages/test_async_messages.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.332Z
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.332Z
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/notifications/messages/conftest.py
🧬 Code graph analysis (1)
tests/e2e/notifications/messages/conftest.py (2)
tests/e2e/conftest.py (2)
short_uuid(101-102)e2e_config(89-92)tests/e2e/accounts/conftest.py (1)
account_id(22-23)
🪛 Ruff (0.14.7)
tests/e2e/notifications/messages/conftest.py
24-24: Unused function argument: e2e_config
(ARG001)
⏰ 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 (1)
tests/e2e/notifications/messages/conftest.py (1)
4-15: message_data fixture looks good and reusableThe payload shape and use of
short_uuidandaccount_idare clear and appropriate for E2E tests; no changes needed.
| @pytest.fixture | ||
| def invalid_message_id(e2e_config): | ||
| return "MSG-000-000-000-000" |
There was a problem hiding this comment.
Remove unused e2e_config parameter from invalid_message_id
Ruff correctly flags e2e_config as unused here; the fixture always returns a constant invalid ID and no longer depends on config. You can simplify and quiet the warning:
@pytest.fixture
-def invalid_message_id(e2e_config):
- return "MSG-000-000-000-000"
+def invalid_message_id():
+ return "MSG-000-000-000-000"This keeps the intent explicit and avoids an unnecessary fixture dependency.
🧰 Tools
🪛 Ruff (0.14.7)
24-24: Unused function argument: e2e_config
(ARG001)
🤖 Prompt for AI Agents
In tests/e2e/notifications/messages/conftest.py around lines 23–25, the pytest
fixture invalid_message_id includes an unused e2e_config parameter; remove
e2e_config from the fixture signature so it simply returns the constant
"MSG-000-000-000-000" and run tests/linters to confirm the Ruff warning is
resolved.
|
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.