Engg. Hygiene: Introduce Extension Status Asserter and use in example Test class#352
Engg. Hygiene: Introduce Extension Status Asserter and use in example Test class#352kjohn-msft wants to merge 8 commits into
Conversation
kjohn-msft
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable ExtStatusAsserter helper for unit tests to validate extension status/substatus output by substatus name (not array position), and updates the ConfigurePatching processor tests to use these deterministic assertions instead of brittle index-based checks.
Changes:
- Added
ExtStatusAssertertest utility to load the status file once and provide higher-level assertion helpers (substatus, errors, patch presence, metadata). - Refactored portions of
Test_ConfigurePatchingProcessorto use the new asserter rather than hard-coded substatus ordering. - Minor logging/documentation tweaks in core logic (
ConfigurePatchingProcessor,ServiceManager).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/core/tests/Test_ConfigurePatchingProcessor.py | Replaces index-based status-file assertions with ExtStatusAsserter calls in several tests. |
| src/core/tests/library/ExtStatusAsserter.py | New helper implementing status/substatus parsing and assertion APIs for tests. |
| src/core/src/core_logic/ServiceManager.py | Adds a short docstring to remove_service(). |
| src/core/src/core_logic/ConfigurePatchingProcessor.py | Adds log prefixes/verbose logs and introduces a cleanup helper method (currently unused). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #352 +/- ##
==========================================
+ Coverage 94.00% 94.05% +0.04%
==========================================
Files 107 109 +2
Lines 19686 19843 +157
==========================================
+ Hits 18506 18663 +157
Misses 1180 1180
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| """ Start configure patching """ | ||
| try: | ||
| self.composite_logger.log("\nStarting configure patching... [MachineId: " + self.env_layer.platform.vm_name() + "][ActivityId: " + self.execution_config.activity_id + "][StartTime: " + self.execution_config.start_time + "]") | ||
| self.composite_logger.log("[CPP] Starting configure patching... [MachineId: " + self.env_layer.platform.vm_name() + "][ActivityId: " + self.execution_config.activity_id + "][StartTime: " + self.execution_config.start_time + "]") |
There was a problem hiding this comment.
Not for this PR but an idea for future logging improvement: it seems like every piece of the code base wants the logging to start with [abbreviation of where the logging comes from], but sometimes it gets missed. Would it be at all feasible to have a custom logger that enforces this, either by taking two strings as parameters, the first being the abbreviation (best practice a const used everywhere in the class) and the second the actual logging message, and then the logger combines them, or having some sort of templated logger that automatically derives the abbreviation? I know this is python so I don't know if that's something we can do, but having a code-based enforcement of the logging standards would be much easier/dev and reviewer friendly than combing over every logging statement and something having to make changes like those in this file.
Do we have a work item for improving the noted test? |
| @@ -0,0 +1,179 @@ | |||
| # Copyright 2025 Microsoft Corporation | |||
| if operation not in self.__substatus_high_level_summary: | ||
| raise KeyError("Unknown operation: {0}".format(operation)) | ||
|
|
||
| substatus_index = self.__substatus_high_level_summary[operation]["index"] |
There was a problem hiding this comment.
What will "index" be? We don't have this in the structure for status
There was a problem hiding this comment.
I think I see how 'index' works from __get_high_level_summary_template()
| self.assertEqual(len(substatus_file_data), 2) | ||
| self.assertTrue(substatus_file_data[0]["name"] == Constants.PATCH_ASSESSMENT_SUMMARY) | ||
| message = json.loads(substatus_file_data[0]["formattedMessage"]["message"]) | ||
| self.assertTrue(message["startedBy"], Constants.PatchAssessmentSummaryStartedBy.PLATFORM) |
There was a problem hiding this comment.
This assert is missing
| runtime.configure_patching_processor.start_configure_patching() | ||
|
|
||
| # restore sdt.out ouptput | ||
| # restore sdt.out output |
There was a problem hiding this comment.
nit: sys.stdout or just stdout
|
|
||
| self.__substatus_file_data = self.__read_status_file(self.__status_file_path) | ||
| self.__substatus_high_level_summary = None | ||
| self.__load_substatus_high_level_summary(self.__substatus_file_data) |
There was a problem hiding this comment.
Wouldn't self.__substatus_file_data be available to __load_substatus_high_level_summary()? Need not pass in as an argument
| self.__substatus_high_level_summary = self.__get_high_level_summary_template() | ||
|
|
||
| for index, substatus in enumerate(substatus_file_data): | ||
| summary_name = substatus["name"] |
There was a problem hiding this comment.
This should be summary_name = substatus[index]["name"], that would contain 'ConfigurePatching' or 'PatchAssessmentSummary', etc
| """Check if the healthstore patch version is as expected""" | ||
| healthstore_summary = self.__get_substatus_message_as_dict(Constants.PATCH_METADATA_FOR_HEALTHSTORE) | ||
|
|
||
| if should_report and healthstore_summary["shouldReportToHealthStore"] != True: |
There was a problem hiding this comment.
nit or maybe another PR: All of these keys should be defined in Constants and referred from there. We will also need to modify StatusHandler.py for this
Test harness improvement to allow unit tests to deterministically check extension status.
In this PR:
- Extensible beyond this
Future changes:
All code authors can rely on this for new tests.
Existing tests may be rewritten as a new-hire exercise.
(Note: This was previously bundled into the commit history of https://github.com/Azure/LinuxPatchExtension/pull/313/changes - moved it out separately as good practice)