Use JSON Schema defaults in synthetic test get_diff - #1670
Conversation
Update get_diff() to accept an optional JSON Schema parameter via the new get_diff_with_schema() function. When a property exists in the expected (desired) state but is missing from the actual state, the function now checks the schema for a 'default' value for that property. If the expected value matches the schema default, it is not reported as differing. This improves synthetic test accuracy for resources that don't return properties whose values match the schema-defined defaults. - Add get_diff_with_schema() with optional schema parameter - Keep get_diff() as a convenience wrapper (no schema) - Update invoke_synthetic_test to retrieve and pass the resource schema - Update DscResource synthetic test path for adapted resources - Add get_schema_default() helper to extract defaults from JSON Schema - Add Test/SchemaDefault test resource and dsctest subcommand - Add Rust unit tests for schema default comparison logic - Add Pester integration tests for end-to-end validation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds schema-aware diffing for synthetic dsc resource test (when a resource lacks a native test operation), so missing properties in get output don’t produce false diffs when the desired value matches the JSON Schema default.
Changes:
- Introduces
get_diff_with_schema(expected, actual, schema)and a helper to read per-property schema defaults whenactualomits a key. - Updates synthetic test paths (command + adapted resources) to retrieve the resource schema and pass it into diffing.
- Adds new Rust + Pester tests, including a
Test/SchemaDefaultresource that omits defaulted fields fromgetoutput.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/dsctest/src/schema_default.rs | Adds a dsctest input struct for the new schema-default test resource. |
| tools/dsctest/src/main.rs | Adds schema-default subcommand and exposes its JSON schema via dsctest schema. |
| tools/dsctest/src/args.rs | Wires new clap subcommand and schema enum variant. |
| tools/dsctest/dsctest.dsc.manifests.json | Adds Test/SchemaDefault manifest with embedded schema defaults. |
| lib/dsc-lib/src/dscresources/dscresource.rs | Adds schema-aware diffing and unit tests for default handling. |
| lib/dsc-lib/src/dscresources/command_resource.rs | Passes resource schema into synthetic test diffing. |
| dsc/tests/dsc_schema_default.tests.ps1 | Adds end-to-end tests verifying schema defaults are honored in synthetic tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ation - Change get_diff_with_schema from pub to pub(crate) since it is only used within the dsc-lib crate - Read schema from RESOURCE_SCHEMAS cache directly (returns Value) instead of round-tripping through get_schema -> String -> from_str. Only calls get_schema to populate the cache on a miss. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add tests verifying that unspecifiedRulesAction set to the schema default
value 'ignore' is no longer reported as drift in synthetic test. Non-default
values ('disable', 'remove') are still correctly flagged.
Tests require elevation to create/remove firewall rules and are skipped
when not running as Administrator.
Fixes #1666
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
😁 Code Coverage ReportChanged Code Coverage90% (90%+ coverage)
🔵 Full Codebase Coverage82% (good)
|
…available Move -Skip to Describe block and check for Get-NetFirewallRule cmdlet availability in BeforeDiscovery. This prevents BeforeAll/AfterAll from running on CI runners without the NetSecurity module. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mikey Lombardi (He/Him) (michaeltlombardi)
left a comment
There was a problem hiding this comment.
Overall, this PR looks good to me.
Out of scope but related, we should probably leverage this new comparison with schemas in a follow up PR to handle write-only and read-only properties that differ between actual/expected - write-only properties can never be compared and there's an argument for read-only properties only being compared when they're in both states (because the user is specifically saying "I expect output to be foo").
| /// Looks up the default value for a property from a JSON Schema. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `schema` - Optional JSON Schema value | ||
| /// * `property_name` - The property name to look up | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// The default value if found in the schema's properties definition, otherwise None | ||
| fn get_schema_default(schema: Option<&Value>, property_name: &str) -> Option<Value> { | ||
| let schema = schema?; | ||
| let properties = schema.get("properties")?.as_object()?; | ||
| let property_schema = properties.get(property_name)?.as_object()?; | ||
| property_schema.get("default").cloned() | ||
| } |
There was a problem hiding this comment.
Nit: Out of scope for this PR, but I think this fits better as an extension method in dsc-lib-jsonschema - probably something like the following signature:
pub get_default_property_value(
&Self,
property_name: &str,
) -> Option<&Value> {}There was a problem hiding this comment.
Agree we can improve this outside of this PR
Co-authored-by: Mikey Lombardi (He/Him) <michael.t.lombardi@gmail.com>
* Use JSON Schema defaults in synthetic test get_diff (#1670) * Use JSON Schema defaults in synthetic test get_diff Update get_diff() to accept an optional JSON Schema parameter via the new get_diff_with_schema() function. When a property exists in the expected (desired) state but is missing from the actual state, the function now checks the schema for a 'default' value for that property. If the expected value matches the schema default, it is not reported as differing. This improves synthetic test accuracy for resources that don't return properties whose values match the schema-defined defaults. - Add get_diff_with_schema() with optional schema parameter - Keep get_diff() as a convenience wrapper (no schema) - Update invoke_synthetic_test to retrieve and pass the resource schema - Update DscResource synthetic test path for adapted resources - Add get_schema_default() helper to extract defaults from JSON Schema - Add Test/SchemaDefault test resource and dsctest subcommand - Add Rust unit tests for schema default comparison logic - Add Pester integration tests for end-to-end validation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address PR feedback: restrict visibility and avoid redundant serialization - Change get_diff_with_schema from pub to pub(crate) since it is only used within the dsc-lib crate - Read schema from RESOURCE_SCHEMAS cache directly (returns Value) instead of round-tripping through get_schema -> String -> from_str. Only calls get_schema to populate the cache on a miss. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Add FirewallRuleList Pester tests for schema default fix (#1666) Add tests verifying that unspecifiedRulesAction set to the schema default value 'ignore' is no longer reported as drift in synthetic test. Non-default values ('disable', 'remove') are still correctly flagged. Tests require elevation to create/remove firewall rules and are skipped when not running as Administrator. Fixes #1666 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix CI: skip firewall schema default tests when NetSecurity module unavailable Move -Skip to Describe block and check for Get-NetFirewallRule cmdlet availability in BeforeDiscovery. This prevents BeforeAll/AfterAll from running on CI runners without the NetSecurity module. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Mikey Lombardi (He/Him) <michael.t.lombardi@gmail.com> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) <slee@ntdev.microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Mikey Lombardi (He/Him) <michael.t.lombardi@gmail.com> * Add scoped unspecified firewall rule handling and allow empty rules (#1671) * Add scoped unspecified firewall rules Replace unspecifiedRulesAction with the scoped unspecifiedRules object and allow empty rule lists for authoritative reconciliation. Add Rust and Pester coverage for direction and profile filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address firewall scope review feedback Reject empty unspecified rule profile filters in the schema and runtime, and localize the VariantClear warning. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix platform-specific changed coverage Merge coverage from every platform when measuring changed Rust code while retaining Linux-only data for the full-codebase metric. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix cross-platform coverage reporting Correct the PowerShell coverage artifact predicate and initialize firewall Pester skip conditions before Describe discovery so elevated Windows CI executes the suites. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Guard firewall tests on NetSecurity Skip firewall set and what-if suites when any cmdlet required for setup or cleanup is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reduce firewall formatting churn Keep the scoped unspecified-rule implementation focused on semantic changes so changed-line coverage measures the feature rather than unrelated rustfmt reflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Minimize changed firewall coverage lines Keep changed expressions in the existing compact style so line coverage is not diluted by formatting-only line splits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) <slee@ntdev.microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Ignore write-only properties in schema-aware diffs (#1674) * Ignore write-only properties in schema diffs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Resolve local refs for write-only properties Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Omit write-only firewall instructions from set output Co-authored-by: SteveL-MSFT <11859881+SteveL-MSFT@users.noreply.github.com> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) <slee@ntdev.microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SteveL-MSFT <11859881+SteveL-MSFT@users.noreply.github.com> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) <slee@ntdev.microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Mikey Lombardi (He/Him) <michael.t.lombardi@gmail.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SteveL-MSFT <11859881+SteveL-MSFT@users.noreply.github.com>
Motivation
When a DSC resource does not implement the
testoperation, the engine performs a synthetic test by callinggetand comparing expected vs actual state usingget_diff(). Previously, if a property existed in the expected (desired) state but was missing from the actual (get) output, it was always reported as differing -- even if the resource simply omits properties whose values match the schema-defined default.This causes false positives in synthetic test results. The most impactful case is
Microsoft.Windows/FirewallRuleListwhereunspecifiedRulesActionis a write-only instruction thatgetnever returns. Setting it to"ignore"(the documented default) permanently reports drift, making the property unusable for compliance reporting.Approach
get_diff_with_schema(expected, actual, schema)which accepts an optional JSON Schema. When a property is inexpectedbut absent fromactual, the function looks up the schema'sdefaultvalue for that property. If the expected value matches the default, it is not flagged as a diff.get_diff(expected, actual)remains as a convenience wrapper (passesNonefor the schema), so all non-synthetic-test call sites are unaffected.RESOURCE_SCHEMAScache (avoiding redundant serialization), with a fallback toget_schema()to populate the cache on a miss.Test coverage
Test/SchemaDefaultresource.unspecifiedRulesAction: ignoreno longer causes false drift, while non-default values (disable,remove) are still correctly reported.dsc_resource_test.tests.ps1tests still pass with no regressions.Fixes #1666