-
Notifications
You must be signed in to change notification settings - Fork 73
Use JSON Schema defaults in synthetic test get_diff #1670
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.
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
Changes from 4 commits
b6e5b53
cf3c11c
0dad5dd
1d20bf7
642e0e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| Describe 'Synthetic test uses schema defaults' { | ||
| It 'Property matching schema default is not reported as differing' { | ||
| $out = '{"name":"test","enabled":true}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $true | ||
| $out.differingProperties | Should -BeNullOrEmpty | ||
| } | ||
|
|
||
| It 'Property differing from schema default is reported as differing' { | ||
| $out = '{"name":"test","enabled":false}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $false | ||
| $out.differingProperties | Should -Contain 'enabled' | ||
| } | ||
|
|
||
| It 'Integer property matching schema default is not reported as differing' { | ||
| $out = '{"name":"test","count":5}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $true | ||
| $out.differingProperties | Should -BeNullOrEmpty | ||
| } | ||
|
|
||
| It 'Integer property differing from schema default is reported as differing' { | ||
| $out = '{"name":"test","count":10}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $false | ||
| $out.differingProperties | Should -Contain 'count' | ||
| } | ||
|
|
||
| It 'Multiple properties matching schema defaults are not reported as differing' { | ||
| $out = '{"name":"test","enabled":true,"count":5}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $true | ||
| $out.differingProperties | Should -BeNullOrEmpty | ||
| } | ||
|
|
||
| It 'Mix of matching and non-matching defaults reports only non-matching' { | ||
| $out = '{"name":"test","enabled":true,"count":10}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $false | ||
| $out.differingProperties | Should -Contain 'count' | ||
| $out.differingProperties | Should -Not -Contain 'enabled' | ||
| } | ||
|
|
||
| It 'Property present in both expected and actual is compared normally' { | ||
| $out = '{"name":"test"}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json | ||
| $LASTEXITCODE | Should -Be 0 | ||
| $out.inDesiredState | Should -Be $true | ||
| $out.differingProperties | Should -BeNullOrEmpty | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -470,7 +470,12 @@ impl Invoke for DscResource { | |
| response.actual_state | ||
| } | ||
| }; | ||
| let diff_properties = get_diff( &desired_state, &actual_state); | ||
| let schema: Option<Value> = if let Some(s) = &self.schema { | ||
| serde_json::to_value(s).ok() | ||
| } else { | ||
| self.schema().ok().and_then(|s| serde_json::from_str(&s).ok()) | ||
| }; | ||
| let diff_properties = get_diff_with_schema( &desired_state, &actual_state, schema.as_ref()); | ||
| desired_state = redact(&desired_state); | ||
| let test_result = TestResult::Resource(ResourceTestResponse { | ||
| desired_state, | ||
|
|
@@ -647,6 +652,24 @@ pub fn get_adapter_input_kind(adapter: &DscResource) -> Result<AdapterInputKind, | |
| /// | ||
| /// An array of top level properties that differ, if any | ||
| pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> { | ||
| get_diff_with_schema(expected, actual, None) | ||
| } | ||
|
|
||
| #[must_use] | ||
| /// Performs a comparison of two JSON Values using an optional JSON Schema. | ||
| /// If a property exists in `expected` but not in `actual`, the schema's `default` value | ||
| /// for that property is used for comparison when available. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `expected` - The expected value | ||
| /// * `actual` - The actual value | ||
| /// * `schema` - Optional JSON Schema to look up default values for missing properties | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// An array of top level properties that differ, if any | ||
| pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Option<&Value>) -> Vec<String> { | ||
| let mut diff_properties: Vec<String> = Vec::new(); | ||
| if expected.is_null() { | ||
| return diff_properties; | ||
|
|
@@ -702,8 +725,17 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> { | |
| diff_properties.push(key.to_string()); | ||
| } | ||
| } else { | ||
| info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); | ||
| diff_properties.push(key.to_string()); | ||
| // Property not in actual - check schema for a default value | ||
| let schema_default = get_schema_default(schema, key); | ||
| if let Some(default_value) = schema_default { | ||
| if value != &default_value { | ||
| info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); | ||
| diff_properties.push(key.to_string()); | ||
| } | ||
| } else { | ||
| info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); | ||
| diff_properties.push(key.to_string()); | ||
| } | ||
| } | ||
| } else { | ||
| info!("{}", t!("dscresources.dscresource.diffKeyNotObject", key = key)); | ||
|
|
@@ -716,6 +748,23 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> { | |
| diff_properties | ||
| } | ||
|
|
||
| /// 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() | ||
| } | ||
|
Comment on lines
+750
to
+765
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Out of scope for this PR, but I think this fits better as an extension method in pub get_default_property_value(
&Self,
property_name: &str,
) -> Option<&Value> {}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree we can improve this outside of this PR |
||
|
|
||
| /// Validates the properties of a resource against its schema. | ||
| /// | ||
| /// # Arguments | ||
|
|
@@ -926,3 +975,76 @@ fn different_array_with_nested_array() { | |
| let array_two = vec![json!("a"), json!(1), json!({"a":"b"}), json!(vec![json!("a"), json!(2)])]; | ||
| assert_eq!(is_same_array(&array_one, &array_two), false); | ||
| } | ||
|
|
||
| #[test] | ||
| fn diff_with_schema_default_matches_expected() { | ||
| use serde_json::json; | ||
| let expected = json!({"name": "test", "enabled": true}); | ||
| let actual = json!({"name": "test"}); | ||
| let schema = json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "enabled": { "type": "boolean", "default": true } | ||
| } | ||
| }); | ||
| let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); | ||
| assert!(diff.is_empty(), "Expected no diff when expected matches schema default, got: {diff:?}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn diff_with_schema_default_differs_from_expected() { | ||
| use serde_json::json; | ||
| let expected = json!({"name": "test", "enabled": false}); | ||
| let actual = json!({"name": "test"}); | ||
| let schema = json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "enabled": { "type": "boolean", "default": true } | ||
| } | ||
| }); | ||
| let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); | ||
| assert_eq!(diff, vec!["enabled".to_string()]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn diff_with_schema_no_default_reports_missing_property() { | ||
| use serde_json::json; | ||
| let expected = json!({"name": "test", "enabled": true}); | ||
| let actual = json!({"name": "test"}); | ||
| let schema = json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "enabled": { "type": "boolean" } | ||
| } | ||
| }); | ||
| let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); | ||
| assert_eq!(diff, vec!["enabled".to_string()]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn diff_without_schema_reports_missing_property() { | ||
| use serde_json::json; | ||
| let expected = json!({"name": "test", "enabled": true}); | ||
| let actual = json!({"name": "test"}); | ||
| let diff = get_diff_with_schema(&expected, &actual, None); | ||
| assert_eq!(diff, vec!["enabled".to_string()]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn diff_with_schema_default_integer() { | ||
| use serde_json::json; | ||
| let expected = json!({"name": "test", "count": 5}); | ||
| let actual = json!({"name": "test"}); | ||
| let schema = json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "count": { "type": "integer", "default": 5 } | ||
| } | ||
| }); | ||
| let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); | ||
| assert!(diff.is_empty(), "Expected no diff when expected matches schema default integer, got: {diff:?}"); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaults' -Skip:(!$canRunFirewallTests) { | ||
| BeforeDiscovery { | ||
| $canRunFirewallTests = $IsWindows -and | ||
| (Get-Command Get-NetFirewallRule -ErrorAction Ignore) -and | ||
| ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( | ||
| [Security.Principal.WindowsBuiltInRole]::Administrator) | ||
| } | ||
|
|
||
| BeforeAll { | ||
| $resourceType = 'Microsoft.Windows/FirewallRuleList' | ||
| $testRuleName = 'DSC-WindowsFirewall-SchemaDefault-Test' | ||
|
|
||
| # Ensure a known rule exists for testing | ||
| $existing = Get-NetFirewallRule -Name $testRuleName -ErrorAction Ignore | ||
| if (-not $existing) { | ||
| New-NetFirewallRule -Name $testRuleName -DisplayName $testRuleName ` | ||
| -Direction Inbound -Action Allow -Protocol TCP -LocalPort 32921 ` | ||
| -Enabled True -PolicyStore PersistentStore | Out-Null | ||
| } | ||
| } | ||
|
|
||
| AfterAll { | ||
| Remove-NetFirewallRule -Name $testRuleName -ErrorAction Ignore | ||
| } | ||
|
|
||
| It 'unspecifiedRulesAction set to default "ignore" does not report as differing' { | ||
| $json = @{ | ||
| unspecifiedRulesAction = 'ignore' | ||
| rules = @(@{ | ||
| name = $testRuleName | ||
| direction = 'Inbound' | ||
| action = 'Allow' | ||
| protocol = 6 | ||
| localPorts = '32921' | ||
| enabled = $true | ||
| }) | ||
| } | ConvertTo-Json -Compress -Depth 5 | ||
| $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | ||
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | ||
|
|
||
| $result = $out | ConvertFrom-Json | ||
| $result.inDesiredState | Should -Be $true | ||
| $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' | ||
| } | ||
|
|
||
| It 'unspecifiedRulesAction omitted does not report as differing' { | ||
| $json = @{ | ||
| rules = @(@{ | ||
| name = $testRuleName | ||
| direction = 'Inbound' | ||
| action = 'Allow' | ||
| protocol = 6 | ||
| localPorts = '32921' | ||
| enabled = $true | ||
| }) | ||
| } | ConvertTo-Json -Compress -Depth 5 | ||
| $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | ||
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | ||
|
|
||
| $result = $out | ConvertFrom-Json | ||
| $result.inDesiredState | Should -Be $true | ||
| $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' | ||
| } | ||
|
|
||
| It 'non-default unspecifiedRulesAction "disable" is reported as differing' { | ||
| $json = @{ | ||
| unspecifiedRulesAction = 'disable' | ||
| rules = @(@{ | ||
| name = $testRuleName | ||
| direction = 'Inbound' | ||
| action = 'Allow' | ||
| protocol = 6 | ||
| localPorts = '32921' | ||
| enabled = $true | ||
| }) | ||
| } | ConvertTo-Json -Compress -Depth 5 | ||
| $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | ||
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | ||
|
|
||
| $result = $out | ConvertFrom-Json | ||
| $result.differingProperties | Should -Contain 'unspecifiedRulesAction' | ||
| } | ||
|
|
||
| It 'non-default unspecifiedRulesAction "remove" is reported as differing' { | ||
| $json = @{ | ||
| unspecifiedRulesAction = 'remove' | ||
| rules = @(@{ | ||
| name = $testRuleName | ||
| direction = 'Inbound' | ||
| action = 'Allow' | ||
| protocol = 6 | ||
| localPorts = '32921' | ||
| enabled = $true | ||
| }) | ||
| } | ConvertTo-Json -Compress -Depth 5 | ||
| $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | ||
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | ||
|
|
||
| $result = $out | ConvertFrom-Json | ||
| $result.differingProperties | Should -Contain 'unspecifiedRulesAction' | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.