Skip to content

fix: Improve the code quality#848

Merged
Roopan-Microsoft merged 2 commits intodev-v4from
psl-code-quality
Mar 12, 2026
Merged

fix: Improve the code quality#848
Roopan-Microsoft merged 2 commits intodev-v4from
psl-code-quality

Conversation

@Prekshith-Microsoft
Copy link
Contributor

@Prekshith-Microsoft Prekshith-Microsoft commented Mar 12, 2026

Purpose

  • ...

Does this introduce a breaking change?

This pull request primarily improves the reliability and structure of the test suite for the backend and agent modules. The main changes include enhancing test robustness, updating test method signatures for async compatibility, and improving module import handling in tests.

Test robustness and reliability improvements:

  • Added a best-effort cleanup in tearDown for locale restoration to prevent test failures due to environment locale issues.
  • Improved event tracking test in test_plan_service.py to use async test methods and verify event tracking is called as expected.

Test structure and import handling:

  • Ensured the proxy_agent module is imported and referenced correctly in tests, adding an explicit import check.
  • Updated the main block in test_orchestration_manager.py to use unittest.main() directly for test execution.
  • Cleaned up unused imports and adjusted import statements for better clarity and compatibility in several test files. [1] [2] [3]

These changes collectively enhance test maintainability and reduce the likelihood of false negatives due to environment or import issues.
This pull request primarily improves the robustness and clarity of the test suite for the backend by refining test logic, fixing minor issues, and improving code quality. The changes include enhancing event tracking tests, improving error handling in locale restoration, clarifying test imports and usage, and making minor cleanups for better readability and reliability.

Test logic improvements:

  • Enhanced the event tracking test in test_plan_service.py to fully mock dependencies, ensure proper asynchronous test execution, and verify that event tracking is called as expected. The test now seeds required mocks and asserts the tracking call, improving coverage and reliability.
  • Improved error handling in test_utils_date.py by making locale restoration failures non-fatal, ensuring that tests don't fail due to environment-specific locale issues.

Code quality and cleanup:

  • Updated test imports and usage in several files:
    • Removed unused imports and ensured all imported modules are used, such as importing proxy_agent_module with a usage assertion in test_proxy_agent.py.
    • Simplified test runner invocation in test_orchestration_manager.py by using main() directly and adjusting imports accordingly. [1] [2]
    • Removed unnecessary imports, such as asyncio in test_lifecycle.py.
    • Cleaned up unused variables and redundant lines in tests, such as removing an unused retry flag in test_MACAE_Smoke_test.py.

Minor improvements:

  • Adjusted assignment of async results to variables instead of discarding them, improving clarity in test_settings.py. [1] [2]
  • Added whitespace for readability in test_settings.py.

Dependency management:

  • Removed unused AsyncMock import in test_app_config.py, keeping only necessary mocks.
  • Yes
  • No

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

This pull request contains several improvements and fixes across multiple test modules in the backend and E2E test suites. The main focus is on enhancing test reliability, improving mocking and patching strategies, and cleaning up unused imports and redundant code. Below are the most important changes grouped by theme:

Test Reliability and Coverage Improvements:

  • Converted the test_event_tracking_calls method in test_plan_service.py to an async test, improved the setup by seeding mock data, and added an assertion to verify that event tracking is actually invoked. This strengthens the test's reliability and coverage.
  • Added a test_module_imports function in test_proxy_agent.py to ensure the proxy_agent module imports correctly and is referenced in tests, helping catch import errors early.

Code and Import Cleanups:

  • Removed unused imports such as asyncio from test_lifecycle.py and AsyncMock from test_app_config.py to clean up the codebase. [1] [2]
  • Removed an unnecessary variable step5_retry_attempted in the E2E smoke test, simplifying the test logic.

Test Runner and Structure Improvements:

  • Simplified the test runner invocation in test_orchestration_manager.py by importing main directly from unittest and replacing the previous if __name__ == '__main__' block with a single call to main(). [1] [2]

Test Robustness:

  • Improved the tearDown method in test_utils_date.py to avoid test failures if restoring the locale fails due to environment configuration, making tests more robust across different setups.

Minor Enhancements:

  • Added a blank line for readability in test_settings.py.

Total number of code quality issue - 16 findings

Copy link
Contributor

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 focuses on improving the backend test suite’s robustness and code quality by refining test logic, cleaning up imports/unused variables, and making tests less sensitive to environment-specific issues (e.g., locale restoration).

Changes:

  • Improved test correctness/robustness in several backend unit tests (notably event tracking coverage and locale cleanup behavior).
  • Cleaned up test code by removing unused imports/variables and simplifying test runners.
  • Minor formatting/readability adjustments in tests.

Reviewed changes

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

Show a summary per file
File Description
tests/e2e-test/tests/test_MACAE_Smoke_test.py Removes an unused retry flag variable in the e2e smoke test.
src/tests/backend/v4/orchestration/test_orchestration_manager.py Simplifies the script entrypoint to call unittest.main() via a direct import.
src/tests/backend/v4/magentic_agents/test_proxy_agent.py Renames the imported module and adds a top-level assertion to “use” the import.
src/tests/backend/v4/magentic_agents/common/test_lifecycle.py Removes an unused asyncio import.
src/tests/backend/v4/config/test_settings.py Adds whitespace and changes how an awaited task handle is consumed.
src/tests/backend/v4/common/services/test_plan_service.py Makes the event tracking test async and seeds mocks so approval flow reaches tracking; asserts tracking call.
src/tests/backend/common/utils/test_utils_date.py Makes locale restoration in tearDown best-effort to avoid env-specific failures.
src/tests/backend/common/database/test_database_base.py Adjusts indentation for an exception-handling test (but introduces a logic issue—see comments).
src/tests/backend/common/config/test_app_config.py Removes unused mock imports.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

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 8 out of 9 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

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 8 out of 9 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 108 to 111
except Exception:
# Best-effort cleanup: if restoring the locale fails (e.g., unsupported locale),
# do not fail tests because of the environment configuration.
pass
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

In tearDown, swallowing all exceptions during locale restoration can leave the process locale in a modified state, which can cascade into unrelated test failures later in the suite. Consider a safer best-effort fallback (e.g., attempt to restore to a known-safe locale like "C" / system default) and/or emit a log message so failures are visible without hard-failing the test run.

Copilot uses AI. Check for mistakes.
@Roopan-Microsoft Roopan-Microsoft merged commit fb00119 into dev-v4 Mar 12, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants