Pester 5 Migration - Rename resources from MSFT to DSC#66
Merged
dan-hughes merged 34 commits intodsccommunity:mainfrom Jan 28, 2026
Merged
Pester 5 Migration - Rename resources from MSFT to DSC#66dan-hughes merged 34 commits intodsccommunity:mainfrom
dan-hughes merged 34 commits intodsccommunity:mainfrom
Conversation
Contributor
WalkthroughRenames resource classes from MSFT_* to DSC_*, centralizes localization, refactors resource Get/Test/Set flows to use Test-DscParameterState and Set-DscMachineRebootRequired, expands analyzer/IDE rules and build/CI configuration, removes several public common helpers, and replaces legacy MSFT Pester tests with new DSC Pester v5 suites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Resource as DSC Resource
participant Registry as Registry / SChannel
participant Reboot as Reboot Manager
Client->>Resource: Invoke Get-TargetResource / Test-TargetResource / Set-TargetResource
Resource->>Registry: Read current values (Get-SChannelItem / Get-SChannelRegKeyValue)
Registry-->>Resource: Return current state
Resource->>Resource: Compare via Test-DscParameterState (CurrentValues vs DesiredValues)
alt changes require reboot
Resource->>Reboot: Call Set-DscMachineRebootRequired (signal reboot)
Reboot-->>Resource: Acknowledge
end
Resource-->>Client: Return result / compliance boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
source/DSCResources/MSFT_SChannelSettings/MSFT_SChannelSettings.psm1 (1)
1-7: Resource directory and file renaming from MSFT to DSC not completed.The resource and all associated files must be renamed as part of this PR:
- Rename directory
source/DSCResources/MSFT_SChannelSettings/tosource/DSCResources/DSC_SChannelSettings/- Rename
MSFT_SChannelSettings.psm1toDSC_SChannelSettings.psm1- Rename
MSFT_SChannelSettings.schema.moftoDSC_SChannelSettings.schema.mof- Rename
MSFT_SChannelSettings.strings.psd1toDSC_SChannelSettings.strings.psd1source/Examples/Resources/Protocol/2-DisableProtocol.ps1 (1)
16-16: Broken URL in LICENSEURI metadata.The URL appears malformed:
/blob/mainICENSEshould be/blob/main/LICENSE. This looks like a pre-existing typo but should be corrected.Proposed fix
-.LICENSEURI https://github.com/dsccommunity/SChannelDsc/blob/mainICENSE +.LICENSEURI https://github.com/dsccommunity/SChannelDsc/blob/main/LICENSEsource/DSCResources/MSFT_KeyExchangeAlgorithm/MSFT_KeyExchangeAlgorithm.psm1 (1)
1-7: Rename folder and .psm1 file to match the DSC prefix in schema.mof.The schema.mof file defines the class as
DSC_KeyExchangeAlgorithm, but the folder and .psm1 file still use theMSFT_KeyExchangeAlgorithmnaming convention. This naming mismatch will cause DSC resource discovery and loading failures. Rename the folder fromsource/DSCResources/MSFT_KeyExchangeAlgorithmtosource/DSCResources/DSC_KeyExchangeAlgorithmand the file fromMSFT_KeyExchangeAlgorithm.psm1toDSC_KeyExchangeAlgorithm.psm1to maintain consistency with the schema class definition.source/DSCResources/DSC_Cipher/DSC_Cipher.psm1 (1)
30-88: Use absoluteHKLM:\paths to avoid drive-relative resolution.
HKLM:SYSTEM...is relative to the current HKLM drive location; usingHKLM:\SYSTEM...avoids accidental mis-resolution.🛠️ Proposed fix
-$key = 'HKLM:SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Ciphers\' + $Cipher +$key = 'HKLM:\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Ciphers\' + $Cipher ... - ItemKey = 'HKLM:SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Ciphers' + ItemKey = 'HKLM:\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Ciphers'source/DSCResources/MSFT_CipherSuites/MSFT_CipherSuites.schema.mof (1)
2-8: Remove unnecessary parameters fromGet-TargetResourceand include all properties in its return value.
Get-TargetResourceshould only accept theIsSingleInstanceparameter (the Key property) to retrieve the current state. Remove the optional parametersCipherSuitesOrder,Ensure, andRebootWhenRequiredfromGet-TargetResourcesince they are not needed to query the current state.Additionally, the returned hashtable must include all resource properties: add
RebootWhenRequiredto the return value (it currently only returnsCipherSuitesOrderandEnsure).tests/Unit/MSFT_CipherSuites.Tests.ps1 (1)
35-174: ReplaceAssert-MockCalledwithShould -Invokeand moveMockstatements outsideInModuleScope.Mocks are declared inside
InModuleScopeblocks (e.g., lines 48-49, 63-65, 81-82, 100, 114), which violates the style guideline. Additionally,Assert-MockCalled(lines 74, 95, 106) is deprecated; useShould -Invokeinstead.Move all mocks to a
BeforeAllblock outsideInModuleScopeand set up$PSDefaultParameterValues['Mock:ModuleName'] = $script:dscResourceNameto automatically bind mocks to the resource module. ReplaceAssert-MockCalledwithShould -Invokeassertions.
🤖 Fix all issues with AI agents
In @.vscode/settings.json:
- Around line 13-16: The settings file uses an absolute path for the PowerShell
ScriptAnalyzer settings; update the value of the
"powershell.scriptAnalysis.settingsPath" key in .vscode/settings.json to a
workspace-relative path (remove the leading slash so it points to
.vscode/analyzersettings.psd1 or use ./ .vscode/analyzersettings.psd1) so VS
Code can find and load the ScriptAnalyzer settings correctly.
In `@source/DSCResources/DSC_Cipher/en-US/MSFT_Cipher.strings.psd1`:
- Around line 1-9: The localization file name is inconsistent with the resource
rename: rename the file MSFT_Cipher.strings.psd1 to DSC_Cipher.strings.psd1 so
it matches the resource folder and DSC naming convention; update any references
to MSFT_Cipher.strings.psd1 (e.g., in build scripts or module manifests) to the
new DSC_Cipher.strings.psd1 name to ensure the localized strings are loaded
correctly.
In `@source/DSCResources/MSFT_Protocol/MSFT_Protocol.schema.mof`:
- Around line 2-3: The MOF class name DSC_Protocol does not match the schema
base name MSFT_Protocol.schema.mof; update the CIM class to match the schema by
renaming the class declaration from DSC_Protocol to MSFT_Protocol (or
alternatively rename the schema file and containing folder to DSC_Protocol if
you intentionally want the class named DSC_Protocol) so the class name and
schema base filename are identical; locate the class declaration line containing
"class DSC_Protocol : OMI_BaseResource" and change the identifier accordingly,
and update any references to the class elsewhere in the module to keep them
consistent.
In `@source/DSCResources/MSFT_SChannelSettings/MSFT_SChannelSettings.psm1`:
- Around line 599-601: Replace the direct throw(...) with a
New-InvalidOperationException that uses the localized message key
HotfixKB3140245NotInstalled from the module localization data (instead of the
hardcoded English string), and remove the backtick line continuation; e.g.,
retrieve the localized text (from the localization file entry
HotfixKB3140245NotInstalled) and pass it to New-InvalidOperationException before
throwing so the code in MSFT_SChannelSettings.psm1 no longer uses throw or
backticks and uses the localized message.
In `@source/DSCResources/MSFT_SChannelSettings/MSFT_SChannelSettings.schema.mof`:
- Around line 2-4: The resource class name and the folder/schema naming are
inconsistent: the class is declared as DSC_SChannelSettings but the
folder/schema use MSFT_SChannelSettings, and the module manifest's
DscResourcesToExport is empty, preventing discovery; fix by either renaming the
class in MSFT_SChannelSettings.schema.mof to match the folder/schema name
(MSFT_SChannelSettings) or renaming the folder/schema to DSC_SChannelSettings so
it matches the class name (DSC_SChannelSettings), then update the module
manifest (SChannelDsc.psd1) to include the resource name in the
DscResourcesToExport array (add "DSC_SChannelSettings" or
"MSFT_SChannelSettings" to match whichever name you choose) so DSC can discover
the resource.
In `@source/Examples/Resources/CipherSuites/1-ConfigureCipherSuitesOrder.ps1`:
- Around line 49-54: The CipherSuitesOrder array in the ConfigureCipherSuites
DSC resource has a missing comma that concatenates two strings and the entire
list is duplicated; open the CipherSuites block (CipherSuites
ConfigureCipherSuites) and fix CipherSuitesOrder by removing the duplicate
second half, inserting the missing comma between
'TLS_PSK_WITH_AES_128_CBC_SHA256' and 'TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384',
and refactor the array into a multi-line @(... ) with one cipher-suite string
per line for readability and to avoid future concatenation errors.
In `@source/Modules/SChannelDsc.Common/Public/Set-SChannelRegKeyValue.ps1`:
- Around line 59-66: The snippet in Set-SChannelRegKeyValue.ps1 contains
commented-out registry path parsing logic (the lines starting with "# if ($Key
-match..." through "# $path = ($Key -replace $root, "").TrimStart('\')") which
is dead code; remove that entire commented block so the function
Set-SChannelRegKeyValue contains only active code and complies with the "no
commented-out code" guideline, leaving any necessary registry handling
implemented elsewhere or documented properly.
In `@tests/Unit/MSFT_CipherSuites.Tests.ps1`:
- Around line 1-7: Remove the top-level "return" that causes the test file to
exit before registering Describe blocks so Pester can discover tests; keep the
existing [CmdletBinding()] and param() declarations and the script-level
variables ($script:DSCModuleName, $script:DSCResourceName) but delete the
standalone "return" statement at the top of the file so the Describe/It blocks
are executed and tests are discoverable.
- Around line 90-94: Tests use deprecated Pester v4 Assert-MockCalled; update
occurrences to Pester v5 by replacing calls like Assert-MockCalled
New-ItemProperty and Assert-MockCalled Remove-ItemProperty with the Should
-Invoke form (e.g., after calling Set-TargetResource use New-ItemProperty |
Should -Invoke -Times 1 or the appropriate invocation count), updating each test
block that references New-ItemProperty and Remove-ItemProperty so they assert
the mock invocation via Should -Invoke instead of Assert-MockCalled.
In `@tests/Unit/MSFT_Hash.Tests.ps1`:
- Around line 1-3: Remove the top-level return statement so Pester can discover
and run the tests; specifically, in the MSFT_Hash.Tests.ps1 file remove the bare
"return" after the param()/[CmdletBinding()] block (do not remove the param() or
CmdletBinding attributes), ensuring the subsequent Describe/Context/It blocks
are reachable and executed.
In `@tests/Unit/MSFT_KeyExchangeAlgorithm.Tests.ps1`:
- Around line 1-3: Remove the top-level "return" statement so the test file can
execute; locate the top of the script where the param() block and the immediate
"return" appear and delete that "return" line (leave the [CmdletBinding()] and
param() intact) so Pester will discover and run the test setup, functions, and
cases defined later in the file.
- Around line 41-42: Remove the deprecated -Fixture parameter from every Context
invocation in the test file: locate the six Context blocks (e.g., the Context
that currently reads Context -Name 'When the key exchange algorithm is enabled
and should be' -Fixture { ...) and change them to the Pester v5 form using the
scriptblock as the positional parameter (Context -Name '...' { ... }) so each
Context only supplies the -Name parameter and the following scriptblock,
ensuring consistent syntax across all Context blocks in
MSFT_KeyExchangeAlgorithm.Tests.ps1.
In `@tests/Unit/MSFT_Protocol.Tests.ps1`:
- Around line 41-42: Remove the obsolete Pester v4 "-Fixture" parameter from all
Context blocks in this test file (there are 11 occurrences) and update the
Context whose name is 'TLS 1.3 is specified with a pre-Windows Server 2022 OS'
to include the "When" prefix (i.e., rename the Context to start with "When TLS
1.3 is specified..."); locate Context declarations by the Context keyword and
the context names to make these edits (remove "-Fixture" and prepend "When " to
that specific context name).
- Around line 313-316: Replace all uses of Assert-MockCalled with the Pester v5
form Should -Invoke -CommandName; specifically, for every Assert-MockCalled
targeting Set-SChannelItem (and other mocked commands) change to Should -Invoke
-CommandName <CommandName> and add -Times 1 for single expected invocations, and
preserve any parameter filters by adding -ParameterFilter { ... } (e.g., convert
the Assert-MockCalled that checked $ItemSubKey -like '*\Client' -and $State -eq
'Enabled' to Should -Invoke -CommandName Set-SChannelItem -Times 1
-ParameterFilter { $ItemSubKey -like '*\Client' -and $State -eq 'Enabled' }).
Ensure you update all six occurrences of Assert-MockCalled in the file
(including the one inside the Set-TargetResource spec).
- Around line 1-3: The file begins with a top-level return after the
CmdletBinding() and param() declarations which prevents Pester from running any
tests; remove the standalone "return" so the script can continue executing tests
(i.e., delete the top-level return following CmdletBinding()/param() in
MSFT_Protocol.Tests.ps1).
In `@tests/Unit/MSFT_SChannelSettings.Tests.ps1`:
- Around line 72-75: Replace deprecated Assert-MockCalled usages with Pester 5's
Should -Invoke in each test that asserts mock invocations; specifically, in the
block calling Set-TargetResource replace "Assert-MockCalled
Set-SChannelRegKeyValue -Times 8" with an assertion using Should -Invoke against
the Set-SChannelRegKeyValue mock (and do the same for the other occurrences
listed: the assertions around lines 147, 185, 228, 296, 355, 470). Ensure you
reference the Set-SChannelRegKeyValue mock name and assert the expected
invocation count with the -Times parameter of Should -Invoke so the tests
preserve the same behavior.
- Around line 1-6: Remove the early return that exits the test script so the
tests run (delete the lone "return" after param()), and rename the test file
from MSFT_SChannelSettings.Tests.ps1 to DSC_SChannelSettings.Tests.ps1 so the
filename matches the resource name; also verify/ensure the
$script:DSCResourceName = 'DSC_SChannelSettings' and $script:DSCModuleName =
'SChannelDsc' remain correct and consistent after the rename.
- Around line 136-142: The test descriptions in the
MSFT_SChannelSettings.Tests.ps1 It blocks are incorrect and don't match their
assertions; update the It block strings to accurately describe the expected
outcomes (e.g., for the block that calls Get-TargetResource and asserts
TLS12State is 'Enabled', change the description from "Should return
TLS12State=Default" to "Should return TLS12State=Enabled"; similarly change the
Test-TargetResource description from "Should return true" to "Should return
false" or vice versa to match the actual assertion). Locate the failing blocks
that call Get-TargetResource and Test-TargetResource using `@testParams` and
adjust their human-readable descriptions to reflect the asserted values so each
It string matches its assertion.
In `@tests/Unit/SChannelDsc.Common/Public/SChannelDsc.Common.Tests.ps1`:
- Line 4: The test file is disabled by an early top-level "return" which skips
all test contexts; remove that "return" on line 4 so the Pester specs for
Convert-SCDscHashtableToString, Get-SChannelItem, Set-SChannelItem, etc. run, or
if intentionally deferred, replace the bare return with a clear TODO comment
referencing a tracking issue and leave the tests intact; if the failure reason
is Pester v5 syntax, migrate the tests to Pester 5 (update Describe/Context/It
blocks and assertion calls) before re-enabling them.
🧹 Nitpick comments (7)
source/DSCResources/MSFT_SChannelSettings/MSFT_SChannelSettings.psm1 (6)
54-54: Use localized strings for verbose messages.This verbose message uses a hardcoded English string instead of a localized string from
$script:localizedData. Per MOF resource guidelines, all messages should be localized. Additionally, theGet-TargetResourceverbose message should start with "Getting the current state of...".Suggested fix
- Write-Verbose -Message 'Getting SChannel configuration settings' + Write-Verbose -Message ($script:localizedData.GetTargetResource_GettingCurrentState)Add to the localization file:
GetTargetResource_GettingCurrentState = 'Getting the current state of SChannel configuration settings.'
61-62: Remove commented-out code.Per coding guidelines, commented-out code should be removed rather than left in the codebase. If this code is no longer needed, delete it entirely.
Suggested fix
- # $dotnetKey = 'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full' - # $keyExists = Get-ItemProperty -Path $dotnetKey -ErrorAction SilentlyContinue -
383-383: Use localized strings for verbose messages.This verbose message should use a localized string and follow the required pattern "Setting the desired state of..." for
Set-TargetResourcefunctions.Suggested fix
- Write-Verbose -Message 'Setting SChannel configuration settings' + Write-Verbose -Message ($script:localizedData.SetTargetResource_SettingDesiredState)Add to the localization file:
SetTargetResource_SettingDesiredState = 'Setting the desired state of SChannel configuration settings.'
387-388: Remove commented-out code.Same issue as in
Get-TargetResource- remove this dead code entirely.Suggested fix
- # $dotnetKey = 'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full' - # $keyExists = Get-ItemProperty -Path $dotnetKey -ErrorAction SilentlyContinue -
733-733: Use localized strings for verbose messages.This verbose message should use a localized string and follow the required pattern "Determining the current state of..." for
Test-TargetResourcefunctions.Suggested fix
- Write-Verbose -Message 'Testing SChannel configuration settings' + Write-Verbose -Message ($script:localizedData.TestTargetResource_DeterminingCurrentState)Add to the localization file:
TestTargetResource_DeterminingCurrentState = 'Determining the current state of SChannel configuration settings.'
743-751: Avoid backtick line continuation; use splatting instead.Per coding guidelines, backtick line continuation should not be used in production code. Consider using splatting for better readability.
Suggested fix
- $compliant = Test-DscParameterState -TurnOffTypeChecking ` - -CurrentValues $CurrentValues ` - -DesiredValues $PSBoundParameters ` - -ValuesToCheck @('DiffieHellmanMinClientKeySize', ` - 'DiffieHellmanMinServerKeySize', ` - 'EnableFIPSAlgorithmPolicy', ` - 'TLS12State', ` - 'KerberosSupportedEncryptionType', ` - 'WinHttpDefaultSecureProtocols') + $testDscParameterStateParameters = @{ + TurnOffTypeChecking = $true + CurrentValues = $CurrentValues + DesiredValues = $PSBoundParameters + ValuesToCheck = @( + 'DiffieHellmanMinClientKeySize' + 'DiffieHellmanMinServerKeySize' + 'EnableFIPSAlgorithmPolicy' + 'TLS12State' + 'KerberosSupportedEncryptionType' + 'WinHttpDefaultSecureProtocols' + ) + } + + $compliant = Test-DscParameterState `@testDscParameterStateParameters`tests/Unit/DSC_Cipher.Tests.ps1 (1)
26-29: Error message should recommendnooptask instead ofbuild.Per DSC Community guidelines, new code should instruct users to run
nooptask for quick test environment setup, as the module should already be built before running tests.♻️ Proposed fix
catch [System.IO.FileNotFoundException] { - throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks build" first.' + throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks noop" first.' }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
source/Examples/Resources/Protocol/2-DisableProtocol.ps1 (1)
16-16: Broken URL in.LICENSEURImetadata.The URL appears malformed:
mainICENSEshould bemain/LICENSE.🔗 Proposed fix
-.LICENSEURI https://github.com/dsccommunity/SChannelDsc/blob/mainICENSE +.LICENSEURI https://github.com/dsccommunity/SChannelDsc/blob/main/LICENSEsource/DSCResources/DSC_Protocol/DSC_Protocol.psm1 (1)
234-235: AddExport-ModuleMemberstatement to export the DSC resource functions.The file is missing the required
Export-ModuleMember -Function *-TargetResourcedirective at the end. DSC MOF-based resources must export functions using the*-TargetResourcepattern to be properly recognized and used as DSC resources.Add the following at the end of the file after the closing brace of
Test-TargetResource:Export-ModuleMember -Function '*-TargetResource'source/DSCResources/DSC_Hash/DSC_Hash.psm1 (1)
134-136: AddExport-ModuleMemberto all DSC resource modules to export functions using the*-TargetResourcepattern.DSC Community Guidelines require explicit export of DSC resource functions. Currently, none of the DSC resource modules (
DSC_Cipher,DSC_CipherSuites,DSC_Hash,DSC_KeyExchangeAlgorithm,DSC_Protocol,DSC_SChannelSettings) includeExport-ModuleMember. Add the following to the end of each resource module:Export-ModuleMember -Function @( 'Get-TargetResource', 'Set-TargetResource', 'Test-TargetResource' )
🤖 Fix all issues with AI agents
In @.vscode/settings.json:
- Around line 60-61: Remove the trailing comma after the JSON property
"pester.suppressCodeLensNotice": true so the object closes cleanly; locate the
settings object that contains the "pester.suppressCodeLensNotice" key and delete
the comma following true to produce valid JSON before the closing brace.
- Line 15: The "powershell.scriptAnalysis.settingsPath" value currently uses a
leading slash ("/.vscode/analyzersettings.psd1") which makes it an absolute
path; update the setting in .vscode/settings.json by removing the leading slash
so it points to the workspace-relative file (e.g.
".vscode/analyzersettings.psd1") or use
"${workspaceFolder}/.vscode/analyzersettings.psd1" to ensure the PowerShell
analyzer can locate the analyzersettings.psd1 file.
In `@source/DSCResources/DSC_Cipher/en-US/DSC_Cipher.strings.psd1`:
- Around line 1-8: Rename the CamelCase localization keys in
DSC_Cipher.strings.psd1 to use underscore-separated names (e.g., ItemEnable ->
item_enable, ItemDisable -> item_disable, ItemDefault -> item_default,
GettingConfiguration -> getting_configuration, SettingConfiguration ->
setting_configuration, TestingConfiguration -> testing_configuration) and update
all references to those keys in the module (search for
$script:localizedData.ItemEnable, $script:localizedData.ItemDisable,
$script:localizedData.ItemDefault, $script:localizedData.GettingConfiguration,
$script:localizedData.SettingConfiguration,
$script:localizedData.TestingConfiguration in DSC_Cipher.psm1) so they use the
new underscore keys; ensure string values remain unchanged and run a quick
module test to confirm no missing localization lookups.
In
`@source/DSCResources/DSC_KeyExchangeAlgorithm/DSC_KeyExchangeAlgorithm.schema.mof`:
- Around line 2-3: The test file is misnamed: rename the test file from
MSFT_KeyExchangeAlgorithm.Tests.ps1 to DSC_KeyExchangeAlgorithm.Tests.ps1 so it
matches the resource class DSC_KeyExchangeAlgorithm and the resource name
variable $script:DSCResourceName = 'DSC_KeyExchangeAlgorithm'; update any
internal references inside the test (if present) to use
DSC_KeyExchangeAlgorithm.Tests.ps1 and the DSC_KeyExchangeAlgorithm resource
name to keep filenames and resource identifiers consistent.
In `@source/DSCResources/DSC_SChannelSettings/DSC_SChannelSettings.psm1`:
- Line 54: Replace hardcoded verbose messages in the DSC_SChannelSettings module
with localized keys that use the required prefixes: for the Get-TargetResource
code path change Write-Verbose -Message 'Getting SChannel configuration
settings' to use $script:localizedData['GettingCurrentState'] (or similarly
named key) so the message reads "Getting the current state of <resource>" and
for the Set-TargetResource and Test-TargetResource paths replace their hardcoded
"Setting…" and "Determining…" messages with
$script:localizedData['SettingDesiredState'] and
$script:localizedData['DeterminingCurrentState'] respectively; update the
module’s localizedData hashtable to include these keys if missing and ensure
each Write-Verbose uses the localized string (e.g.,
$script:localizedData['GettingCurrentState']) in Get-TargetResource,
Set-TargetResource, and Test-TargetResource.
- Around line 599-600: Replace the raw throw expression in
DSC_SChannelSettings.psm1 with a New-InvalidOperationException using a localized
message: add the key HotfixNotInstalled = 'Hotfix KB3140245 is not installed.
Setting these registry keys will not do anything. Please install the hotfix
first.' to en-US/DSC_SChannelSettings.strings.psd1, then replace the throw (...)
with code that builds the message from the module's localized data (e.g., use
$LocalizedData['HotfixNotInstalled'] or the existing localized variable) and
call New-InvalidOperationException -Message $localizedMessage (or
New-InvalidOperationException $localizedMessage) so the resource uses
New-InvalidOperationException with the localized string instead of throw.
In `@source/Examples/Resources/CipherSuites/1-ConfigureCipherSuitesOrder.ps1`:
- Around line 49-54: The CipherSuitesOrder array in the ConfigureCipherSuites
block contains a missing comma that concatenates
'TLS_PSK_WITH_AES_128_CBC_SHA256' and 'TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384'
into one invalid entry; open the CipherSuitesOrder definition in
ConfigureCipherSuites and insert a comma between those two items to split them
into separate strings, then review and remove the unintended duplicated sequence
of cipher names (the repeated block starting at the second
'TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384') so the list contains each cipher
exactly once.
In `@tests/Unit/MSFT_CipherSuites.Tests.ps1`:
- Around line 1-3: Remove the top-level short-circuiting statement: delete the
standalone "return" at the top of the test file (the line after the param()
block) so Pester can discover and run the tests; keep the existing
[CmdletBinding()] and param() declarations intact and ensure no other top-level
returns remain before the Describe/It blocks.
In `@tests/Unit/MSFT_Hash.Tests.ps1`:
- Around line 1-3: Remove the top‑level "return" statement that short‑circuits
the test file so Pester can discover and run the tests; locate the standalone
return in the MSFT_Hash.Tests.ps1 file (the top of the file after the param
block) and delete that line, leaving the CmdletBinding and param() intact so the
rest of the test script is executed.
In `@tests/Unit/MSFT_KeyExchangeAlgorithm.Tests.ps1`:
- Around line 1-3: Remove the top-level "return" statement that short-circuits
execution so Pester can discover and run the tests; keep the existing
[CmdletBinding()] and param() boilerplate but delete the standalone "return" at
the file scope so test definitions are executed.
In `@tests/Unit/MSFT_Protocol.Tests.ps1`:
- Around line 1-3: The file currently contains a top-level "return" that
short-circuits execution and prevents Pester from discovering tests; remove the
top-level "return" statement (the bare return on line 3 in the
param/CmdletBinding block) so the test script executes normally and Pester can
find and run the tests.
In `@tests/Unit/MSFT_SChannelSettings.Tests.ps1`:
- Around line 1-3: Remove the top-level "return" statement at the start of the
test file (the standalone return after the param() block) so the Pester tests
are not short-circuited; keep the existing [CmdletBinding()] and param()
declaration but delete the lone return line so the tests in
MSFT_SChannelSettings.Tests.ps1 execute normally.
- Around line 72-75: Replace each Assert-MockCalled assertion with the Pester 5
syntax using Should -Invoke: for every occurrence (e.g., the assertion that
Set-SChannelRegKeyValue was called in the Set-TargetResource test), wrap the
mock invocation in a scriptblock and use Should -Invoke with the same -Times
count (for example: { Set-SChannelRegKeyValue } | Should -Invoke -Times 8). Do
this for all six mock assertions so they use Should -Invoke and preserve the
original mock name and expected call count.
In `@tests/Unit/SChannelDsc.Common/Public/SChannelDsc.Common.Tests.ps1`:
- Line 4: Remove the unconditional top-level "return" that disables the whole
file and instead mark tests as skipped via Pester so they remain visible: delete
the standalone "return" and add a -Skip "migrating to Pester5" (or similar
message) to the top-level Describe/Context blocks in
SChannelDsc.Common.Tests.ps1 (or wrap the file's Describe calls with a
conditional check such as if (-not $env:SKIP_PESTER_MIGRATION) { ... } to
control execution) so tests are not silently disabled.
🧹 Nitpick comments (8)
source/Modules/SChannelDsc.Common/Public/Set-SChannelRegKeyValue.ps1 (1)
60-65: Remove commented-out code instead of leaving it in place.Per coding guidelines, commented-out code should not be left in source files. This code was also dead code—
$rootand$pathwere never used sinceSet-ItemPropertyon line 73 uses$fullSubKeydirectly.♻️ Proposed fix
else { # Update registry key - # if ($Key -match "(.*:).*") - # { - # $root = $Matches[1] - # } - - # $path = ($Key -replace $root, "").TrimStart('\') - $currentKey = Get-Item -Path $fullSubKey -ErrorAction SilentlyContinuetests/Unit/SChannelDsc.Common/Public/SChannelDsc.Common.Tests.ps1 (2)
5-27: Test setup requires migration to Pester 5 format.When re-enabling these tests, the setup structure needs to follow the DSC Community unit test guidelines with
BeforeDiscovery,BeforeAll, andAfterAllblocks. The current manual module discovery pattern in the HEADER region should be replaced with the standard template.
82-99: Mocks insideInModuleScopeviolate testing guidelines.Lines 84, 89-90, and 95-96 define
Mockcommands inside theInModuleScopeblock. Per DSC Community testing guidelines, mocks should be defined outsideInModuleScopeblocks using$PSDefaultParameterValues['Mock:ModuleName'].When migrating to Pester 5, restructure to define mocks in
BeforeAllblocks outsideInModuleScope.tests/Unit/MSFT_Protocol.Tests.ps1 (1)
41-41: Context names should start with “When”.Rename the Context to start with “When” to keep naming consistent. As per coding guidelines, follow the prescribed test naming conventions.
♻️ Suggested rename
- Context -Name 'TLS 1.3 is specified with a pre-Windows Server 2022 OS' -Fixture { + Context -Name 'When TLS 1.3 is specified with a pre-Windows Server 2022 OS' -Fixture {tests/Unit/DSC_Cipher.Tests.ps1 (2)
28-28: Error message should recommend-Tasks noopinstead ofbuild.Per DSC Community guidelines, the error message should instruct users to run the
nooptask for quick test environment setup, notbuild. Thebuildtask is more extensive and unnecessary when running tests in a separate process.Proposed fix
- throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks build" first.' + throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks noop" first.'
139-177: Consider adding test case forRebootWhenRequired = $false.The test covers all three
Statevalues but always setsRebootWhenRequired = $true. Consider adding a test case to verify thatSet-DscMachineRebootRequiredis not called whenRebootWhenRequired = $false.Example additional test
It 'Should not call Set-DscMachineRebootRequired when RebootWhenRequired is false' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $testParams = @{ Cipher = 'AES 128/128' State = 'Enabled' RebootWhenRequired = $false } $null = Set-TargetResource `@testParams` } Should -Invoke -CommandName Set-SChannelItem -Exactly -Times 1 -Scope It Should -Invoke -CommandName Set-DscMachineRebootRequired -Exactly -Times 0 -Scope It }source/DSCResources/DSC_Cipher/DSC_Cipher.psm1 (1)
20-28: Remove unused parameters fromGet-TargetResource.Per DSC Community guidelines,
Get-TargetResourceshould only include parameters needed to retrieve the actual current state. TheStateandRebootWhenRequiredparameters are not used in this function.Proposed fix
function Get-TargetResource { [CmdletBinding()] [OutputType([System.Collections.Hashtable])] param ( [Parameter(Mandatory = $true)] [ValidateSet('AES 128/128', 'AES 256/256', 'DES 56/56', 'NULL', 'RC2 128/128', 'RC2 40/128', 'RC2 56/128', 'RC4 128/128', 'RC4 40/128', 'RC4 56/128', 'RC4 64/128', 'Triple DES 168')] [System.String] - $Cipher, - - [Parameter()] - [ValidateSet('Enabled', 'Disabled', 'Default')] - [System.String] - $State = 'Default', - - [Parameter()] - [System.Boolean] - $RebootWhenRequired = $false + $Cipher ).vscode/analyzersettings.psd1 (1)
83-94: Consider enablingCheckOperatorfor whitespace consistency.The coding guidelines specify "One space around operators:
$a = 1 + 2". CurrentlyCheckOperatoris set to$false. Consider enabling it to enforce operator spacing, unless there are known false positive issues.Proposed change
PSUseConsistentWhitespace = @{ Enable = $true CheckOpenBrace = $true CheckInnerBrace = $true CheckOpenParen = $true - CheckOperator = $false + CheckOperator = $true CheckSeparator = $true CheckPipe = $true CheckPipeForRedundantWhitespace = $true CheckParameter = $false }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/DSCResources/DSC_CipherSuites/DSC_CipherSuites.psm1 (1)
50-55: Return hashtable should include all resource properties.Per guidelines,
Get-TargetResourcemust return a hashtable with all resource properties. TheIsSingleInstancekey property is missing from the return value.Proposed fix
$returnValue = @{ + IsSingleInstance = $IsSingleInstance CipherSuitesOrder = [System.String[]]$order Ensure = [System.String]$Ensure }
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_CipherSuites/DSC_CipherSuites.psm1`:
- Line 89: The verbose messages use the wrong format parameter: change the
Write-Verbose calls that pass $Ensure to instead pass the cipher list string
variable (e.g., $cipherSuitesAsString) so the localized messages
($script:localizedData.ItemEnable and $script:localizedData.ItemDisable) receive
the expected cipher-suite text; move the assignment that builds
$cipherSuitesAsString to before the Write-Verbose invocation(s) (references:
Write-Verbose, $script:localizedData.ItemEnable,
$script:localizedData.ItemDisable, $Ensure, $cipherSuitesAsString) and update
both occurrences (around the current Write-Verbose lines and the similar call at
the second location).
In `@tests/Unit/DSC_CipherSuites.Tests.ps1`:
- Around line 1-4: The SYNOPSIS header in the test file incorrectly names the
resource as "DSC_Cipher"; update the .SYNOPSIS line in
tests/Unit/DSC_CipherSuites.Tests.ps1 to reference the correct resource
"DSC_CipherSuites" so the top-of-file docblock matches the file and tests,
particularly the .SYNOPSIS string near the file header.
- Line 28: The thrown error message in the test file DSC_CipherSuites.Tests.ps1
currently tells users to run ".\build.ps1 -ResolveDependency -Tasks build";
update the throw string to instruct running the noop task instead (e.g.
".\build.ps1 -ResolveDependency -Tasks noop") so the guidance is correct; locate
the throw statement in DSC_CipherSuites.Tests.ps1 and change only the message
text to reference the noop task.
🧹 Nitpick comments (8)
source/DSCResources/DSC_CipherSuites/DSC_CipherSuites.psm1 (4)
34-34: Verbose message should follow the required pattern.Per MOF-based DSC resource guidelines,
Get-TargetResourceverbose messages must start with "Getting the current state of...". The current localized string keyGettingConfigurationproduces "Getting configuration for cipher suites order".
82-82: Verbose message should follow the required pattern.Per guidelines,
Set-TargetResourceverbose messages must start with "Setting the desired state of...". The current message says "Setting configuration for cipher suites order".
133-133: Verbose message should follow the required pattern.Per guidelines,
Test-TargetResourceverbose messages must start with "Determining the current state of...". The current message says "Testing configuration for cipher suites order".
143-143: Consider adding explicitExport-ModuleMember.The file is missing
Export-ModuleMember -Function *-TargetResource. While functions are exported by default in script modules, explicit declaration improves clarity and follows the guideline to "Export using*-TargetResourcepattern".Proposed addition at end of file
Export-ModuleMember -Function *-TargetResourcesource/DSCResources/DSC_CipherSuites/en-US/DSC_CipherSuites.strings.psd1 (2)
2-5: String key naming convention: use underscores for multi-word keys.Per localization guidelines, multi-word string keys in
.strings.psd1files should use underscores as word separators. Consider renaming:
ItemEnable→Item_EnableItemDisable→Item_DisableItemNotCompliant→Item_Not_CompliantItemCompliant→Item_CompliantAlso,
ItemNotCompliantandItemCompliantappear to be defined but not used in the module code.
7-9: Inconsistent alignment and verbose message patterns.
- Line 8 has an extra space before
=compared to other lines (inconsistent formatting).- The verbose messages don't follow the required patterns:
GettingConfigurationshould produce "Getting the current state of..."TestingConfigurationshould produce "Determining the current state of..."SettingConfigurationshould produce "Setting the desired state of..."Proposed fix
- GettingConfiguration = Getting configuration for cipher suites order - TestingConfiguration = Testing configuration for cipher suites order - SettingConfiguration = Setting configuration for cipher suites order + Getting_Configuration = Getting the current state of cipher suites order + Testing_Configuration = Determining the current state of cipher suites order + Setting_Configuration = Setting the desired state of cipher suites ordertests/Unit/DSC_CipherSuites.Tests.ps1 (2)
36-40: Avoid backtick for line continuation.Per guidelines, never use backtick as line continuation in production code. Use splatting instead.
Proposed fix using splatting
- $script:testEnvironment = Initialize-TestEnvironment ` - -DSCModuleName $script:dscModuleName ` - -DSCResourceName $script:dscResourceName ` - -ResourceType 'Mof' ` - -TestType 'Unit' + $initializeTestEnvironmentParams = @{ + DSCModuleName = $script:dscModuleName + DSCResourceName = $script:dscResourceName + ResourceType = 'Mof' + TestType = 'Unit' + } + + $script:testEnvironment = Initialize-TestEnvironment `@initializeTestEnvironmentParams`
214-271: Consider adding test forRebootWhenRequired = $false.The tests cover scenarios where
RebootWhenRequired = $true, verifying thatSet-DscMachineRebootRequiredis called. Consider adding a test case whereRebootWhenRequired = $falseto verify thatSet-DscMachineRebootRequiredis NOT invoked.Example additional test
Context 'When the resource needs to be created and RebootWhenRequired is false' { BeforeAll { Mock -CommandName New-Item Mock -CommandName New-ItemProperty Mock -CommandName Set-DscMachineRebootRequired } It 'Should not call Set-DscMachineRebootRequired' { InModuleScope -ScriptBlock { Set-StrictMode -Version 1.0 $mockParams = @{ IsSingleInstance = 'Yes' CipherSuitesOrder = @( 'TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384' ) Ensure = 'Present' RebootWhenRequired = $false } $null = Set-TargetResource `@mockParams` } Should -Invoke -CommandName Set-DscMachineRebootRequired -Exactly -Times 0 -Scope It } }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_Hash/DSC_Hash.psm1`:
- Line 30: Update the localized strings referenced by the DSC_Hash module so
verbose messages use the required MOF resource prefixes: change the value for
the GettingConfiguration key (used in Write-Verbose -Message
($script:localizedData.GettingConfiguration -f $Hash)) to start with "Getting
the current state of…", update the SettingConfiguration key to start with
"Setting the desired state of…", and the DeterminingConfiguration/Test key to
start with "Determining the current state of…"; preserve existing format
placeholders (like -f $Hash) and keep the localization keys unchanged so the
calls in DSC_Hash.psm1 (GettingConfiguration and the other two localized keys
referenced at the other locations) continue to match.
In `@tests/Unit/DSC_Hash.Tests.ps1`:
- Around line 151-154: The ParameterFilter is tautological because it compares
$State to itself; update the filter in the mock assertion for Set-SChannelItem
so it compares the incoming $State parameter to the test's expected state value
(e.g. replace "$State -eq $State" with "$State -eq $ExpectedState" or the actual
variable name used in the test that represents the desired/requested state),
ensuring the assertion validates the requested state alongside the existing
"$ItemSubKey -eq $Hash" check.
- Line 28: The throw in DSC_Hash.Tests.ps1 currently recommends running
".\build.ps1 -ResolveDependency -Tasks build"; update that error message to
recommend the noop task instead (e.g., instruct users to run ".\build.ps1
-ResolveDependency -Tasks noop") so unit tests use the quick dependency setup;
modify the throw statement's message string accordingly.
🧹 Nitpick comments (1)
source/DSCResources/DSC_Hash/en-US/DSC_Hash.strings.psd1 (1)
2-8: Use underscore-separated keys for multi‑word localization entries.Guidelines require underscores for multi‑word string keys (e.g.,
Item_Enable,Getting_Configuration). Renaming will require updating the corresponding$script:localizedData.<Key>usages insource/DSCResources/DSC_Hash/DSC_Hash.psm1. Based on learnings, please align key naming.♻️ Proposed key rename (update references in DSC_Hash.psm1 accordingly)
- ItemEnable = Enabling Hash {0} - ItemDisable = Disabling Hash {0} - ItemDefault = Defaulting Hash {0} + Item_Enable = Enabling Hash {0} + Item_Disable = Disabling Hash {0} + Item_Default = Defaulting Hash {0} - GettingConfiguration = Getting configuration for hash {0} - TestingConfiguration = Testing configuration for hash {0} - SettingConfiguration = Setting configuration for hash {0} + Getting_Configuration = Getting configuration for hash {0} + Testing_Configuration = Testing configuration for hash {0} + Setting_Configuration = Setting configuration for hash {0}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_KeyExchangeAlgorithm/DSC_KeyExchangeAlgorithm.psm1`:
- Around line 113-122: Test-DscParameterState can skip comparing the resource's
State when it's not passed because $PSBoundParameters omits it; update the
preparation of DesiredValues so it always includes a State entry (use
Test-DscParameterState, Get-TargetResource, $PSBoundParameters and
$compareDscParameterStateParameters to locate the code) by creating a copy of
$PSBoundParameters and, if the 'State' key is absent, set it to the resource
default (e.g., 'Default') before assigning DesiredValues; then pass that
augmented hash to Test-DscParameterState.
🧹 Nitpick comments (2)
source/DSCResources/DSC_Hash/DSC_Hash.psm1 (1)
20-28: Remove unused parameters fromGet-TargetResource.Per MOF resource guidelines,
Get-TargetResourceshould only include parameters needed to retrieve the actual current state. The$Stateand$RebootWhenRequiredparameters are never used in this function body and should be removed.♻️ Suggested fix
function Get-TargetResource { [CmdletBinding()] [OutputType([System.Collections.Hashtable])] param ( [Parameter(Mandatory = $true)] [ValidateSet('MD5', 'SHA', 'SHA256', 'SHA384', 'SHA512')] [System.String] - $Hash, - - [Parameter()] - [ValidateSet('Enabled', 'Disabled', 'Default')] - [System.String] - $State = 'Default', - - [Parameter()] - [System.Boolean] - $RebootWhenRequired = $false + $Hash )As per coding guidelines,
Get-TargetResourceshould remove non-mandatory parameters that are never used.tests/Unit/DSC_KeyExchangeAlgorithm.Tests.ps1 (1)
1-56: Avoid backtick line continuations in setup.
Use splatting to improve readability and align with the no-backtick guideline.As per coding guidelines, ...♻️ Proposed refactor
- $script:testEnvironment = Initialize-TestEnvironment ` - -DSCModuleName $script:dscModuleName ` - -DSCResourceName $script:dscResourceName ` - -ResourceType 'Mof' ` - -TestType 'Unit' + $initializeParams = @{ + DSCModuleName = $script:dscModuleName + DSCResourceName = $script:dscResourceName + ResourceType = 'Mof' + TestType = 'Unit' + } + $script:testEnvironment = Initialize-TestEnvironment `@initializeParams`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_Protocol/DSC_Protocol.psm1`:
- Around line 34-35: The verbose messages use localized strings that don't start
with the required DSC prefixes; update the localizedData entries used by
Write-Verbose so they begin with the mandated phrases. Specifically, change the
values for $script:localizedData.GettingConfiguration, the localized string used
at the Write-Verbose call around line 109 (the "Setting..." message), and the
localized string used at the Write-Verbose call around line 200 (the
"Determining..." message) to start respectively with "Getting the current state
of...", "Setting the desired state of...", and "Determining the current state
of...". Ensure the format placeholder (-f $Protocol) remains intact and only the
localized string prefix is adjusted.
In `@tests/Unit/DSC_Protocol.Tests.ps1`:
- Around line 6-8: The SuppressMessageAttribute applied at the top of the test
file is missing a justification string; update the attribute
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments',
'')] to include the standard justification (set the Justification named
argument) so the suppression is documented—for example, add
Justification='Pester test harness intentionally assigns variables for
assertions; analyzer false positive.' to the attribute on the param() block.
🧹 Nitpick comments (2)
source/DSCResources/DSC_Protocol/en-US/DSC_Protocol.strings.psd1 (1)
1-12: Align localized string keys with underscore naming.Multi‑word keys should use underscores in resource
.strings.psd1files (e.g.,Set_Client_Protocol) to match localization conventions. Renaming will require updating the corresponding$script:localizedDatausages insource/DSCResources/DSC_Protocol/DSC_Protocol.psm1and the unit tests. Based on learnings, please follow the underscore key rule.tests/Unit/DSC_Protocol.Tests.ps1 (1)
36-40: Prefer splatting over backtick line continuations.Backticks are discouraged in this codebase; splatting keeps the call readable and guideline‑compliant. As per coding guidelines, avoid backtick continuations.
♻️ Proposed refactor
- $script:testEnvironment = Initialize-TestEnvironment ` - -DSCModuleName $script:dscModuleName ` - -DSCResourceName $script:dscResourceName ` - -ResourceType 'Mof' ` - -TestType 'Unit' + $initializeTestEnvironmentParams = @{ + DSCModuleName = $script:dscModuleName + DSCResourceName = $script:dscResourceName + ResourceType = 'Mof' + TestType = 'Unit' + } + + $script:testEnvironment = Initialize-TestEnvironment `@initializeTestEnvironmentParams`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SChannelSettings/DSC_SChannelSettings.psm1`:
- Around line 61-63: Remove the commented-out legacy code blocks in
DSC_SChannelSettings.psm1: delete the two commented lines that define $dotnetKey
and $keyExists (the lines beginning with "# $dotnetKey =
'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full'" and "# $keyExists =
Get-ItemProperty -Path $dotnetKey -ErrorAction SilentlyContinue'") and also
remove the other commented-out pair noted around the same file (the commented
lines at the later occurrence referenced in the review). Ensure no residual
commented variables or dead code remain so the module contains only active,
relevant code.
In
`@source/DSCResources/DSC_SChannelSettings/en-US/DSC_SChannelSettings.strings.psd1`:
- Around line 16-18: The resource keys in
DSC_SChannelSettings.en-US.strings.psd1 use concatenated words
(GettingConfiguration, TestingConfiguration, SettingConfiguration); rename them
to underscore-separated forms (Getting_Configuration, Testing_Configuration,
Setting_Configuration) and update all references that consume these keys in
DSC_SChannelSettings.psm1 to use the new key names (search for usages of
GettingConfiguration, TestingConfiguration, SettingConfiguration and replace
with the underscore versions) so the naming follows the .strings.psd1
localization convention.
In `@tests/Unit/DSC_SChannelSettings.Tests.ps1`:
- Around line 207-210: Replace the commented-out WinHTTP assertions with a
proper skipped test: create a new It block (e.g., It "Validates
WinHttpDefaultSecureProtocols" -Skip "32-bit/64-bit mismatch; see ISSUE-XYZ")
that contains the two assertions currently commented out
($result.WinHttpDefaultSecureProtocols | Should -Be
$mockParams.WinHttpDefaultSecureProtocols and
$result.WinHttpDefaultSecureProtocols.Count | Should -Be
$mockParams.WinHttpDefaultSecureProtocols.Count) so they are tracked by test
tooling; do the same refactor for the other commented groups at the locations
referencing the same WinHttpDefaultSecureProtocols assertions (the blocks around
the occurrences you noted) and provide a concise skip reason.
- Around line 165-170: The ParameterFilter in the Mock for
Get-SChannelRegKeyValue is using assignment ($Key = '...') instead of
comparison, so the $Key check never applies and mocks can collide; update the
ParameterFilter block in the Mock -CommandName Get-SChannelRegKeyValue to use
-eq for $Key (e.g. $Key -eq
'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Internet Settings\WinHttp') and
combine it with the $Name check using -and (e.g. $Key -eq '...' -and $Name -eq
'DefaultSecureProtocols'), and apply the same -eq/-and pattern to the other
WinHTTP/FIPS ParameterFilter mocks as well.
- Around line 6-8: The suppression attribute at the top currently lacks the
required Justification string; update the attribute usage for
PSUseDeclaredVarsMoreThanAssignments to include a Justification parameter and
replace the current top-of-file setup with the standard Pester test setup block:
add a BeforeDiscovery block that imports the DscResource.Test module, a
BeforeAll block that configures PSDefaultParameterValues (and any required
mock/setup), and an AfterAll block for cleanup before the Describe block; ensure
the suppression attribute (for PSUseDeclaredVarsMoreThanAssignments) remains
present but includes the exact Justification text required by the template.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@tests/Unit/SChannelDsc.Common/Public/Get-SChannelItem.Tests.ps1`:
- Around line 81-83: The test description in the It block for the
Get-SChannelItem unit test uses "It" instead of the required "Should"; update
the It statement's description string to start with "Should" (e.g., change "It
should return '<ExpectedResult>'" to "Should return '<ExpectedResult>'") in the
test containing Get-SChannelItem and $ExpectedResult, and ensure the description
does not include the word "when" to comply with the guideline.
In `@tests/Unit/SChannelDsc.Common/Public/Set-SChannelItem.Tests.ps1`:
- Around line 84-94: The test's It description is misleading: update the It
block description that currently reads "Should remove the key" to reflect the
action being tested (setting Enabled state); edit the It description in the
Set-SChannelItem test (the block that calls Set-SChannelItem with ItemKey
'TestRegistry:\SCHANNEL\Protocols', ItemSubKey 'SSL 3.0', State 'Enabled' and
asserts Enabled equals 1) to something like "Should set the key to Enabled" so
the description matches the assertion.
- Around line 104-114: The test description for the It block labeled 'Should
remove the key' is misleading because the test calls Set-SChannelItem with State
= 'Disabled' and asserts the registry value 'Enabled' equals 0; update the It
description to accurately describe the behavior being tested (e.g., "Should set
Enabled to 0 when State is 'Disabled'") so it's clear this verifies the Disabled
state handling of Set-SChannelItem rather than key removal.
In `@tests/Unit/SChannelDsc.Common/Public/Set-SChannelRegKeyValue.Tests.ps1`:
- Around line 76-79: The test description is misleading: update the It block's
description string for the test that calls Set-SChannelRegKeyValue (the block
that sets Key 'TestRegistry:\SCHANNEL\Protocols', SubKey 'SSL 3.0', Name
'Enabled', Value 1 and asserts the value equals 1) to accurately describe the
behavior (e.g., "Should set the registry value to 1" or "Should enable the
registry key") by replacing "Should remove the registry key" with a correct
description in the It(...) declaration.
- Around line 82-91: The test's It block is misleading and asserts the wrong
value for the "Disabled" protocol state; update the It description to reflect
that the registry value is being set (e.g., "Should set the registry value to
Disabled") and change the call to Set-SChannelRegKeyValue and the subsequent
Get-ItemPropertyValue assertion to expect 0 for the Disabled state (use
Set-SChannelRegKeyValue -Key 'TestRegistry:\SCHANNEL\Protocols' -SubKey 'SSL
3.0' -Name 'Enabled' -Value 0 and assert the retrieved value is 0), keeping
references to Set-SChannelRegKeyValue and Get-ItemPropertyValue so the test
intent matches the Disabled state.
🧹 Nitpick comments (1)
tests/Unit/SChannelDsc.Common/Public/Get-SCDscOSVersion.Tests.ps1 (1)
57-65: Consider adding a Context block for scenario organization.Per DSC Community testing guidelines, each scenario should be wrapped in a separate
Contextblock with a description starting with "When". While this is a simple test, adding the Context block improves consistency and makes it easier to add additional scenarios in the future.✨ Suggested improvement
Describe 'SChannelDsc.Common\Get-SCDscOSVersion' { - BeforeAll { - $currentOS = [System.Environment]::OSVersion.Version - } + Context 'When getting the OS version' { + BeforeAll { + $currentOS = [System.Environment]::OSVersion.Version + } - It 'Should return the correct result' { - Get-SCDscOSVersion | Should -Be $currentOS + It 'Should return the correct OS version' { + Get-SCDscOSVersion | Should -Be $currentOS + } } }As per coding guidelines: "Each scenario = separate
Contextblock" and "Contextdescriptions start with 'When'".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_CipherSuites/en-US/DSC_CipherSuites.strings.psd1`:
- Around line 1-7: Rename multi-word localization keys in the
DSC_CipherSuites.strings.psd1 to use underscores (e.g., change
GettingConfiguration → Getting_Configuration, TestingConfiguration →
Testing_Configuration, SettingConfiguration → Setting_Configuration, ItemEnable
→ Item_Enable, ItemDisable → Item_Disable) and then update every reference to
those keys in DSC_CipherSuites.psm1 (search for the original identifiers like
GettingConfiguration, TestingConfiguration, SettingConfiguration, ItemEnable,
ItemDisable and replace them with the underscored versions) so string lookups
use the new keys consistently.
🧹 Nitpick comments (1)
source/DSCResources/DSC_KeyExchangeAlgorithm/en-US/DSC_KeyExchangeAlgorithm.strings.psd1 (1)
2-8: Use underscores for multi-word localization keys.Keys like
ItemEnableandGettingConfigurationare multi-word; the localization guideline for DSC resource strings prefers underscores. Please rename and update references accordingly.♻️ Suggested rename pattern
- ItemEnable = Enabling KeyExchangeAlgorithm {0} - ItemDisable = Disabling KeyExchangeAlgorithm {0} - ItemDefault = Defaulting KeyExchangeAlgorithm {0} + Item_Enable = Enabling KeyExchangeAlgorithm {0} + Item_Disable = Disabling KeyExchangeAlgorithm {0} + Item_Default = Defaulting KeyExchangeAlgorithm {0} - GettingConfiguration = Getting the current state of key exchange algorithm {0} - TestingConfiguration = Determining the current state of key exchange algorithm {0} - SettingConfiguration = Setting the desired state of key exchange algorithm {0} + Getting_Configuration = Getting the current state of key exchange algorithm {0} + Testing_Configuration = Determining the current state of key exchange algorithm {0} + Setting_Configuration = Setting the desired state of key exchange algorithm {0}As per coding guidelines, use underscores as word separators in localized string key names for multi-word keys.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
====================================
Coverage ? 99%
====================================
Files ? 6
Lines ? 399
Branches ? 0
====================================
Hits ? 398
Misses ? 1
Partials ? 0 🚀 New features to boost your workflow:
|
dan-hughes
added
needs review
johlju
requested changes
Jan 28, 2026
Member
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju partially reviewed 71 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @dan-hughes).
source/Modules/SChannelDsc.Common/Public/Set-SChannelRegKeyValue.ps1 line 65 at r7 (raw file):
# } # $path = ($Key -replace $root, "").TrimStart('\')
Shall we remove the commented code?
Code quote:
# Update registry key
# if ($Key -match "(.*:).*")
# {
# $root = $Matches[1]
# }
# $path = ($Key -replace $root, "").TrimStart('\')
johlju
reviewed
Jan 28, 2026
johlju
reviewed
Jan 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SChannelSettings/DSC_SChannelSettings.psm1`:
- Around line 309-315: The return hash is coercing $null DH key-size values to 0
by casting `$dhMinClientKeySizeValue` and `$dhMinServerKeySizeValue` to
[System.UInt32]; change this so you only cast when a value is present and
otherwise preserve $null. Update where the return keys
DiffieHellmanMinClientKeySize and DiffieHellmanMinServerKeySize are assigned: if
`$dhMinClientKeySizeValue`/`$dhMinServerKeySizeValue` is not $null cast to
[System.UInt32], otherwise leave the value as $null (do the conditional
conversion before placing into the return hashtable).
Comment on lines
309
to
+315
| $returnValue = @{ | ||
| IsSingleInstance = 'Yes' | ||
| TLS12State = $currentTls12State | ||
| DiffieHellmanMinClientKeySize = $dhMinClientKeySizeValue | ||
| DiffieHellmanMinServerKeySize = $dhMinServerKeySizeValue | ||
| KerberosSupportedEncryptionType = $kerberosEncrTypes | ||
| WinHttpDefaultSecureProtocols = $winhttpProtocols | ||
| DiffieHellmanMinClientKeySize = [System.UInt32] $dhMinClientKeySizeValue | ||
| DiffieHellmanMinServerKeySize = [System.UInt32] $dhMinServerKeySizeValue | ||
| KerberosSupportedEncryptionType = [System.String[]] $kerberosEncrTypes | ||
| WinHttpDefaultSecureProtocols = [System.String[]] $winhttpProtocols |
Contributor
There was a problem hiding this comment.
Preserve $null for DH key sizes instead of coercing to 0.
[System.UInt32] $null becomes 0, which can report a configured value when the registry entry is missing. Consider only casting when a value is present.
🛠️ Suggested adjustment
- DiffieHellmanMinClientKeySize = [System.UInt32] $dhMinClientKeySizeValue
- DiffieHellmanMinServerKeySize = [System.UInt32] $dhMinServerKeySizeValue
+ DiffieHellmanMinClientKeySize = if ($null -ne $dhMinClientKeySizeValue) { [System.UInt32] $dhMinClientKeySizeValue } else { $null }
+ DiffieHellmanMinServerKeySize = if ($null -ne $dhMinServerKeySizeValue) { [System.UInt32] $dhMinServerKeySizeValue } else { $null }🤖 Prompt for AI Agents
In `@source/DSCResources/DSC_SChannelSettings/DSC_SChannelSettings.psm1` around
lines 309 - 315, The return hash is coercing $null DH key-size values to 0 by
casting `$dhMinClientKeySizeValue` and `$dhMinServerKeySizeValue` to
[System.UInt32]; change this so you only cast when a value is present and
otherwise preserve $null. Update where the return keys
DiffieHellmanMinClientKeySize and DiffieHellmanMinServerKeySize are assigned: if
`$dhMinClientKeySizeValue`/`$dhMinServerKeySizeValue` is not $null cast to
[System.UInt32], otherwise leave the value as $null (do the conditional
conversion before placing into the return hashtable).
9 tasks
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
Migrate tests to Pester 5.
Rename resources with MSFT prefix to DSC
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
help.
This change is