Skip to content

notification unit test #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

notification unit test #3

wants to merge 17 commits into from

Conversation

@abdulanu0
Copy link
Contributor

@abdulanu0 abdulanu0 commented Oct 28, 2025 •
edited
Loading

Adding unit tests to tooling, tooling extensions as well as notifications

Copilot AI review requested due to automatic review settings October 28, 2025 18:31
Copilot AI reviewed Oct 28, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive unit test coverage for the notification models and agent notification functionality in the microsoft-agents-a365-notifications library. The tests cover model initialization, validation, property access, and routing behavior.

Key Changes:

  • Added unit tests for NotificationTypes enum covering all enum operations and string comparisons
  • Added unit tests for EmailReference and WpxComment models testing initialization, validation, and serialization
  • Added unit tests for AgentNotificationActivity testing entity parsing and model conversion
  • Added unit tests for AgentNotification class testing route matching and decorator registration
  • Updated import path from microsoft_agents.activity to microsoft_agents.activity.channel_id for ChannelId

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/notifications/models/test_wpx_comment.py Comprehensive tests for WpxComment model initialization, validation, and property access
tests/notifications/models/test_notification_types.py Tests for NotificationTypes enum values, string operations, and enum behavior
tests/notifications/models/test_email_reference.py Tests for EmailReference model covering initialization, validation, and HTML content handling
tests/notifications/models/test_agent_notification_activity.py Tests for AgentNotificationActivity including entity parsing and model conversion
tests/notifications/models/test_agent_notification.py Tests for AgentNotification class covering route matching, decorators, and subchannel handling
tests/notifications/models/init.py Added copyright header and module docstring for test package
tests/notifications/init.py Added copyright header and module docstring for test package
libraries/microsoft-agents-a365-notifications/microsoft_agents_a365/notifications/agent_notification.py Updated ChannelId import path to be more specific

joratz
joratz previously approved these changes Oct 28, 2025
View reviewed changes
Copilot AI review requested due to automatic review settings November 2, 2025 04:01
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 27 comments.

Copilot AI review requested due to automatic review settings November 2, 2025 04:13
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings November 2, 2025 04:51
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.

Copilot AI review requested due to automatic review settings November 2, 2025 05:35
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 10 comments.

@abdulanu0
Update copyright headers to Microsoft Corporation format
3147656 
- Standardized all test files to use 'Copyright (c) Microsoft Corporation.'
- Added 'Licensed under the MIT License.' to comply with Microsoft coding standards
- Updated 9 test files across notification and tooling unittest modules
- Ensures consistency with Microsoft open source project requirements
@abdulanu0
Fix CI import issue by replacing mock MCPServerConfig with real model
ac6311d
Copilot AI review requested due to automatic review settings November 2, 2025 06:02
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.

juliomenendez
juliomenendez requested changes Nov 2, 2025
View reviewed changes
Copy link
Contributor

@juliomenendez juliomenendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the .coverage file in the repo?
The license header should say Copyright (c) Microsoft Corporation. All rights reserved., nothing related to MIT license.

@juliomenendez
Copy link
Contributor

juliomenendez commented Nov 2, 2025

Please provide a description for the PR.

@abdulanu0
Resolving PR comments: Remove fallback imports, fix copyright headers…
59b71f6 
…, remove .NET env vars, rename test directories, remove .coverage file
Copilot AI review requested due to automatic review settings November 7, 2025 21:30
@abdulanu0 abdulanu0 requested a review from a team as a code owner November 7, 2025 21:30
Copilot AI reviewed Nov 7, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 17 comments.

libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/utils/utility.py
str: The current environment name.
"""
return os.getenv("ASPNETCORE_ENVIRONMENT") or os.getenv("DOTNET_ENVIRONMENT") or "Development"
return os.getenv("ENVIRONMENT") or "Development"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed environment variables ASPNETCORE_ENVIRONMENT and DOTNET_ENVIRONMENT are referenced by "Kairo" in existing code files. According to custom guideline 1000000, the keyword "Kairo" should be removed or replaced with appropriate terminology. Multiple files in the codebase contain "Kairo" references including comments like "Kairo Python SDK", "Kairo tracer", "KairoInstrumentorOpenAIAgents" class names, and module descriptions mentioning "Kairo SDK". These should be updated to use Microsoft-appropriate terminology.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-tooling-unittest/utils/test_constants.py
@@ -0,0 +1,177 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-tooling-unittest/utils/__init__.py
@@ -0,0 +1,5 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
...icrosoft-agents-a365-tooling-unittest/services/test_mcp_tool_server_configuration_service.py
@@ -0,0 +1,746 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-tooling-unittest/services/__init__.py
@@ -0,0 +1,5 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-notification/models/test_email_reference.py
@@ -0,0 +1,255 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-notification/models/test_agent_notification_activity.py
@@ -0,0 +1,386 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-notification/models/test_agent_notification.py
@@ -0,0 +1,385 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-notification/models/__init__.py
@@ -0,0 +1,5 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
tests/microsoft-agents-a365-notification/__init__.py
@@ -0,0 +1,5 @@
# Copyright (c) Microsoft. All rights reserved.
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header format should be "# Copyright (c) Microsoft Corporation." with "Corporation" included, and "# Licensed under the MIT License." on the second line.

Copilot generated this review using guidance from repository custom instructions.
rahuldevikar761
rahuldevikar761 reviewed Nov 11, 2025
View reviewed changes
tests/microsoft-agents-a365-tooling-unittest/utils/test_utility.py Outdated
result = get_tools_mode()

# Assert
assert result == ToolsMode.MOCK_MCP_SERVER
Copy link
Collaborator

@rahuldevikar761 rahuldevikar761 Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought toolsMode was removed from the code. Can you confirm? If not, then we should think if we need to remove it.

Copilot AI review requested due to automatic review settings November 11, 2025 19:49
Copilot finished reviewing on behalf of abdulanu0 November 11, 2025 19:50
Copilot AI reviewed Nov 11, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

libraries/microsoft-agents-a365-tooling/microsoft_agents_a365/tooling/utils/utility.py
Comment on lines +79 to 81
def get_ppapi_token_scope():
"""
Gets the MCP platform authentication scope based on the current environment.
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name get_ppapi_token_scope doesn't match the docstring which refers to 'MCP platform authentication scope'. The docstring should be updated to reflect that this returns the Power Platform API token scope.

Copilot uses AI. Check for mistakes.
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Comment on lines +55 to +57
assert email_string == "Type: NotificationTypes.EMAIL_NOTIFICATION"
assert wpx_string == "Type: NotificationTypes.WPX_COMMENT"
assert lifecycle_string == "Type: NotificationTypes.AGENT_LIFECYCLE"
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expects enum string formatting to include the enum class name (e.g., 'NotificationTypes.EMAIL_NOTIFICATION'), but Python's f-string formatting with enum values typically just returns the value itself (e.g., 'emailNotification'). Unless the enum has a custom __str__ method, this test will likely fail.

Copilot uses AI. Check for mistakes.
rahuldevikar761
rahuldevikar761 requested changes Nov 13, 2025
View reviewed changes
Copy link
Collaborator

@rahuldevikar761 rahuldevikar761 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested feedback on the chat. Please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@rahuldevikar761 rahuldevikar761 rahuldevikar761 requested changes

@pontemonti pontemonti Awaiting requested review from pontemonti

@nikhilNava nikhilNava Awaiting requested review from nikhilNava

@juliomenendez juliomenendez Awaiting requested review from juliomenendez

@joratz joratz Awaiting requested review from joratz

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@abdulanu0 @juliomenendez @rahuldevikar761 @joratz