OCPERT-340: Fix object has no attribute error in Jira Notificator#958
OCPERT-340: Fix object has no attribute error in Jira Notificator#958tomasdavidorg wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (2)
tests/test_jira_notificator.py (2)
75-81:⚠️ Potential issue | 🔴 CriticalTest expects old mention format, but production now uses
accountId.The production code changed to
[~accountId:{u.accountId}]format, but this test still expects[~tdavid](the.nameformat). The test mock also doesn't setaccountId. This test will fail.🐛 Fix test to match new format
def test_create_jira_comment_mentions(self): second_user = Mock() - second_user.name = "gjospin" + second_user.accountId = "account-id-2" self.assertEqual(self.ns.create_jira_comment_mentions([]), "") - self.assertEqual(self.ns.create_jira_comment_mentions([self.test_user]), "[~tdavid] ") - self.assertEqual(self.ns.create_jira_comment_mentions([self.test_user, second_user]), "[~tdavid] [~gjospin] ") + self.assertEqual(self.ns.create_jira_comment_mentions([self.test_user]), "[~accountId:test-account-id] ") + self.assertEqual(self.ns.create_jira_comment_mentions([self.test_user, second_user]), "[~accountId:test-account-id] [~accountId:account-id-2] ")Also add
accountIdto the test_user setup insetUp:self.test_user.name = "tdavid" self.test_user.displayName = "Tomas David" self.test_user.emailAddress = "tdavid@redhat.com" + self.test_user.accountId = "test-account-id"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_jira_notificator.py` around lines 75 - 81, The test test_create_jira_comment_mentions expects the old name-based mention format but production uses accountId; update the test to set accountId on the mocks (e.g., set self.test_user.accountId and second_user.accountId in setUp or within the test) and change the asserted strings to the new format produced by create_jira_comment_mentions (e.g., "[~accountId:{...}] " for each user) so the expectations match the current create_jira_comment_mentions output.
140-163:⚠️ Potential issue | 🔴 CriticalMultiple tests still expect the old
[~name]mention format.
test_create_assignee_notification_textand several other tests expect strings like[~tdavid] [~gjospin], but production now outputs[~accountId:...]. These mocks needaccountIdattributes and assertions need updating to match the new format.Affected tests:
test_create_assignee_notification_text,test_notify_assignees,test_create_qa_notification_text,test_notify_qa_contact,test_create_team_lead_notification_text,test_notify_team_lead,test_create_manager_notification_text,test_notify_manager,test_check_issue_and_notify_responsible_people.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_jira_notificator.py` around lines 140 - 163, The tests (e.g., test_create_assignee_notification_text) still expect the old Jira mention format "[~name]" while the production code now emits "[~accountId:...]", so update the mocks and assertions: add an accountId attribute to the mock assignee/QA/team lead/manager objects used in tests and change expected strings in assertions to the new "[~accountId:...]" format (or build expected mentions dynamically from the mock.accountId values) for the tested functions create_assignee_notification_text, create_qa_notification_text, create_team_lead_notification_text, create_manager_notification_text and the notification tests notify_assignees, notify_qa_contact, notify_team_lead, notify_manager, test_check_issue_and_notify_responsible_people.
🤖 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/test_jira_notificator.py`:
- Around line 75-81: The test test_create_jira_comment_mentions expects the old
name-based mention format but production uses accountId; update the test to set
accountId on the mocks (e.g., set self.test_user.accountId and
second_user.accountId in setUp or within the test) and change the asserted
strings to the new format produced by create_jira_comment_mentions (e.g.,
"[~accountId:{...}] " for each user) so the expectations match the current
create_jira_comment_mentions output.
- Around line 140-163: The tests (e.g., test_create_assignee_notification_text)
still expect the old Jira mention format "[~name]" while the production code now
emits "[~accountId:...]", so update the mocks and assertions: add an accountId
attribute to the mock assignee/QA/team lead/manager objects used in tests and
change expected strings in assertions to the new "[~accountId:...]" format (or
build expected mentions dynamically from the mock.accountId values) for the
tested functions create_assignee_notification_text, create_qa_notification_text,
create_team_lead_notification_text, create_manager_notification_text and the
notification tests notify_assignees, notify_qa_contact, notify_team_lead,
notify_manager, test_check_issue_and_notify_responsible_people.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a66225b0-2622-47eb-b8c7-ddae54d4cea4
📒 Files selected for processing (2)
oar/notificator/jira_notificator.pytests/test_jira_notificator.py
|
🤔 need to fix the test cases. I will take a look tomorrow. |
tomasdavidorg
changed the title
|
@tomasdavidorg: This pull request references OCPERT-340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
@coderabbitai review again |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_jira_notificator.py (1)
401-408: Pragmatic relaxation for integration tests, but consider adding a unit test.The relaxed assertions are understandable since
test_issue_on_qais a live Jira issue whose state can change over time. However, this reduces coverage of the notification type logic.Consider adding a unit test with mocked issue data to verify that
check_issue_and_notify_responsible_peoplereturns the correctNotificationTypebased on the issue's notification history (e.g., returnsQA_CONTACTtype when no prior notifications exist,TEAM_LEADafter QA contact notification, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_jira_notificator.py` around lines 401 - 408, Add a focused unit test that mocks Jira issue data and notification history to verify check_issue_and_notify_responsible_people returns specific NotificationType values (e.g., QA_CONTACT, TEAM_LEAD) for given histories instead of relying on the live test_issue_on_qa; use the same identifiers used in the file (check_issue_and_notify_responsible_people, NotificationType, ERT_NOTIFICATION_PREFIX, test_issue_on_qa) to locate logic and assert the returned notification.type and notification.text contents deterministically, simulating prior notifications to exercise transitions (no prior -> QA_CONTACT, after QA_CONTACT -> TEAM_LEAD, etc.).
🤖 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/test_jira_notificator.py`:
- Around line 401-408: Add a focused unit test that mocks Jira issue data and
notification history to verify check_issue_and_notify_responsible_people returns
specific NotificationType values (e.g., QA_CONTACT, TEAM_LEAD) for given
histories instead of relying on the live test_issue_on_qa; use the same
identifiers used in the file (check_issue_and_notify_responsible_people,
NotificationType, ERT_NOTIFICATION_PREFIX, test_issue_on_qa) to locate logic and
assert the returned notification.type and notification.text contents
deterministically, simulating prior notifications to exercise transitions (no
prior -> QA_CONTACT, after QA_CONTACT -> TEAM_LEAD, etc.).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d2c0764-73f2-4672-b89a-80227c17ef7a
📒 Files selected for processing (2)
oar/notificator/jira_notificator.pytests/test_jira_notificator.py
|
@tomasdavidorg: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
https://redhat.atlassian.net/browse/OCPERT-340