Build common module with ModuleBuilder#2430
Conversation
WalkthroughSqlServerDsc.Common refactored: the monolithic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@source/Modules/SqlServerDsc.Common/Public/Test-ImpersonatePermissions.ps1`:
- Line 37: The verbose message in Test-ImpersonatePermissions uses a misspelled
property $testLoginEffectivePermissionsParams.SInstanceName which doesn't exist;
update the Write-Verbose call to use
$testLoginEffectivePermissionsParams.InstanceName instead so the instance name
is printed correctly (verify the same
$testLoginEffectivePermissionsParams.InstanceName property is used consistently
elsewhere in the Test-ImpersonatePermissions function).
In
`@source/Modules/SqlServerDsc.Common/Public/Test-LoginEffectivePermissions.ps1`:
- Around line 82-99: The query in Test-LoginEffectivePermissions.ps1 builds
$queryToGetEffectivePermissionsForLogin by directly interpolating $LoginName,
$SecurableName and $SecurableClass which allows SQL injection; fix this by using
a parameterized SqlCommand (preferred) or by sanitizing inputs before
interpolation (at minimum replace single quotes with doubled single quotes for
$LoginName and $SecurableName) and keep $SecurableClass validated; update the
code paths that set $queryToGetEffectivePermissionsForLogin to either construct
the SQL with parameters bound (using SqlCommand.Parameters) or with escaped
values so that EXECUTE AS LOGIN and fn_my_permissions are safe.
In `@tests/Unit/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.Tests.ps1`:
- Around line 28-84: Set the CI flag and fix path depth: in BeforeAll set
$env:SqlServerDscCI = $true and in AfterAll remove it; in BeforeDiscovery update
the build invocation path from "$PSScriptRoot/../../build.ps1" to
"$PSScriptRoot/../../../../build.ps1". Add a parameter set validation test for
the public command Copy-ItemWithRobocopy by adding a new It block that uses
Get-Command -Name 'Copy-ItemWithRobocopy' and asserts the ParameterSets contains
the expected mandatory parameter set(s) (follow the existing template pattern
used elsewhere), referencing the Copy-ItemWithRobocopy command and the test
file's BeforeAll/AfterAll setup to ensure the environment is correct.
In
`@tests/Unit/SqlServerDsc.Common/Public/Test-LoginEffectivePermissions.Tests.ps1`:
- Around line 120-134: The test hashtables
$testLoginEffectiveServerPermissionsParams and
$testLoginEffectiveLoginPermissionsParams supply a parameter named Login but the
function under test is Test-LoginEffectivePermissions which expects LoginName;
update both hashtables to replace the Login key with LoginName so the mandatory
parameter is correctly passed to Test-LoginEffectivePermissions.
🟠 Major comments (72)
tests/Unit/SqlServerDsc.Common/Public/Get-ServiceAccount.Tests.ps1-86-128 (1)
86-128: Add required parameter set and parameter attribute validation tests.This is a public command test file and must include the parameter set validation and parameter properties templates (via
Get-CommandandParameterSets). As per coding guidelines, please add those required test blocks.tests/Unit/SqlServerDsc.Common/Public/Get-ServiceAccount.Tests.ps1-50-52 (1)
50-52: Set and clear$env:SqlServerDscCIin unit tests.Unit tests must set
$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll. Missing this can change CI behavior. Based on learnings, please add the required environment handling.✅ Proposed fix
BeforeAll { $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' + + $env:SqlServerDscCI = $true @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')Also applies to: 74-78
tests/Unit/SqlServerDsc.Common/Public/Get-ServiceAccount.Tests.ps1-63-67 (1)
63-67: Correct SMO stub path to the required location.The SMO stub should be loaded from
"$PSScriptRoot/../Stubs/SMO.cs"; the current path points to aStubsfolder underPublic, which is not the required location. Based on learnings, please align to the standard stub path.✅ Proposed fix
- Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\SMO.cs')tests/Unit/SqlServerDsc.Common/Public/Get-ServiceAccount.Tests.ps1-33-38 (1)
33-38: Fix build.ps1 path in the required unit-test scaffold.Line 37 uses
"$PSScriptRoot/../../build.ps1", but the mandated scaffold uses"$PSScriptRoot/../../../build.ps1". With the current path, dependency bootstrap can fail whenDscResource.Testis missing. As per coding guidelines, please align to the standard template.✅ Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $nullsource/Modules/SqlServerDsc.Common/Public/ConvertTo-ServerInstanceName.ps1-12-36 (1)
12-36: Rename to follow public command naming conventionPublic commands under
Public/must follow{Verb}-SqlDsc{Noun}. ConsiderConvertTo-SqlDscServerInstanceNameand update call sites/tests accordingly. As per coding guidelines.source/Modules/SqlServerDsc.Common/Public/Test-FeatureFlag.ps1-11-24 (1)
11-24: Rename to follow public command naming conventionPublic commands under
Public/must follow{Verb}-SqlDsc{Noun}. ConsiderTest-SqlDscFeatureFlagand update call sites/tests accordingly. As per coding guidelines.tests/Unit/SqlServerDsc.Common/Public/Find-ExceptionByNumber.Tests.ps1-86-107 (1)
86-107: Add parameter set and parameter attribute validation testsPublic command unit tests must include the parameter set validation and parameter attribute templates using
Get-Command. Please add those blocks forFind-ExceptionByNumber. As per coding guidelines.tests/Unit/SqlServerDsc.Common/Public/Find-ExceptionByNumber.Tests.ps1-50-58 (1)
50-58: Add SqlServerDscCI env var setup/teardownUnit tests must set and remove
$env:SqlServerDscCIinBeforeAll/AfterAll.As per coding guidelines.🔧 Suggested fix
BeforeAll { $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' + + $env:SqlServerDscCI = $true @@ AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') @@ # Remove module common test helper. Get-Module -Name 'CommonTestHelper' -All | Remove-Module -Force + + Remove-Item -Path 'env:SqlServerDscCI' }Also applies to: 74-84
source/Modules/SqlServerDsc.Common/Public/Start-SqlSetupProcess.ps1-14-29 (1)
14-29: Add CmdletBinding/OutputType and ShouldProcess gatingThis command starts a setup process (state-changing). Add
[CmdletBinding(SupportsShouldProcess = $true, ConfirmImpact = 'Medium')], implement the standardShouldProcesspattern with localized messages, include aForceparameter + ConfirmPreference override, and add[OutputType([System.Int32])]. As per coding guidelines.source/Modules/SqlServerDsc.Common/Public/Start-SqlSetupProcess.ps1-31-40 (1)
31-40: Apply ErrorActionPreference guard around -ErrorAction StopThe guidelines require setting
$ErrorActionPreference = 'Stop'immediately before each-ErrorAction 'Stop'call and restoring it after.As per coding guidelines.🔧 Suggested fix
- $sqlSetupProcess = Start-Process `@startProcessParameters` -PassThru -NoNewWindow -ErrorAction 'Stop' + $previousErrorActionPreference = $ErrorActionPreference + $ErrorActionPreference = 'Stop' + $sqlSetupProcess = Start-Process `@startProcessParameters` -PassThru -NoNewWindow -ErrorAction 'Stop' + $ErrorActionPreference = $previousErrorActionPreference @@ - Wait-Process -InputObject $sqlSetupProcess -Timeout $Timeout -ErrorAction 'Stop' + $previousErrorActionPreference = $ErrorActionPreference + $ErrorActionPreference = 'Stop' + Wait-Process -InputObject $sqlSetupProcess -Timeout $Timeout -ErrorAction 'Stop' + $ErrorActionPreference = $previousErrorActionPreferencesource/Modules/SqlServerDsc.Common/Public/Start-SqlSetupProcess.ps1-1-13 (1)
1-13: Complete comment-based helpAdd DESCRIPTION, EXAMPLE, INPUTS, and OUTPUTS sections to the help block.
As per coding guidelines.📝 Suggested help skeleton
<# .SYNOPSIS Starts the SQL setup process. + .DESCRIPTION + <Add 40+ character description here.> .PARAMETER FilePath String containing the path to setup.exe. .PARAMETER ArgumentList The arguments that should be passed to setup.exe. .PARAMETER Timeout The timeout in seconds to wait for the process to finish. + + .EXAMPLE + Start-SqlSetupProcess -FilePath 'C:\setup.exe' -ArgumentList '/q' -Timeout 600 + + .INPUTS + `System.String` - FilePath/ArgumentList. + + .OUTPUTS + `System.Int32` - Process exit code. #>tests/Unit/SqlServerDsc.Common/Public/Find-ExceptionByNumber.Tests.ps1-63-67 (1)
63-67: Align SMO stub path with unit-test conventionUse the standard
../Stubs/SMO.cslocation for unit tests.As per coding guidelines.🔧 Suggested fix
- Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath '../Stubs') -ChildPath 'SMO.cs')source/Modules/SqlServerDsc.Common/Public/Test-FeatureFlag.ps1-1-10 (1)
1-10: Add full comment-based help sectionsPublic commands require DESCRIPTION, EXAMPLE, INPUTS, and OUTPUTS sections with the mandated detail/format.
As per coding guidelines.📝 Suggested help skeleton
<# .SYNOPSIS Test if the specific feature flag should be enabled. + .DESCRIPTION + <Add 40+ character description here.> .PARAMETER FeatureFlag An array of feature flags that should be compared against. .PARAMETER TestFlag The feature flag that is being check if it should be enabled. + + .EXAMPLE + Test-SqlDscFeatureFlag -FeatureFlag @('Foo') -TestFlag 'Foo' + + .INPUTS + `System.String[]` - Feature flag list. + + .OUTPUTS + `System.Boolean` - True when the flag is enabled. #>tests/Unit/SqlServerDsc.Common/Public/Find-ExceptionByNumber.Tests.ps1-33-38 (1)
33-38: Fix build.ps1 relative path in BeforeDiscoveryFrom this file’s location,
../../build.ps1points totests/Unit/build.ps1rather than repo root. Use the correct relative path.🔧 Suggested fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $nullsource/Modules/SqlServerDsc.Common/Public/ConvertTo-ServerInstanceName.ps1-1-11 (1)
1-11: Add full comment-based help sectionsPublic commands require DESCRIPTION, EXAMPLE, INPUTS, and OUTPUTS sections with the mandated detail/format.
As per coding guidelines.📝 Suggested help skeleton
<# .SYNOPSIS Converts the combination of server name and instance name to the correct server instance name. + .DESCRIPTION + <Add 40+ character description here.> .PARAMETER InstanceName Specifies the name of the SQL Server instance on the host. .PARAMETER ServerName Specifies the host name of the SQL Server. + .EXAMPLE + ConvertTo-SqlDscServerInstanceName -ServerName 'Server01' -InstanceName 'MSSQLSERVER' + + .INPUTS + `System.String` - Server and instance names. + + .OUTPUTS + `System.String` - Normalized server instance name. #>tests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1-86-121 (1)
86-121: Add parameter-set and parameter-property validation testsStart-SqlSetupProcess is a public command and needs the standard parameter set and parameter property validation tests (Get-Command templates). Please add those contexts alongside the behavioral tests. As per coding guidelines, follow the unit-test templates.
tests/Unit/SqlServerDsc.Common/Public/Test-FeatureFlag.Tests.ps1-86-104 (1)
86-104: Add parameter-set and parameter-property validation testsPublic commands require the standard Get-Command-based parameter set and parameter property validation coverage; this file only tests runtime behavior. Please add the required template-based tests for Test-FeatureFlag. As per coding guidelines, please align with the unit-test templates.
source/Modules/SqlServerDsc.Common/Public/Get-ServiceAccount.ps1-1-7 (1)
1-7: Complete comment-based helpHelp text is missing DESCRIPTION/INPUTS/OUTPUTS/EXAMPLE sections. Please add the required sections and match the standard format. As per coding guidelines, comment-based help must include the full required sections.
source/Modules/SqlServerDsc.Common/Public/Split-FullSqlInstanceName.ps1-1-11 (1)
1-11: Complete comment-based helpHelp text is missing DESCRIPTION/EXAMPLE/INPUTS and the OUTPUTS section doesn’t follow the required format. Please add the missing sections and match the required structure. As per coding guidelines, comment-based help must include the full required sections.
tests/Unit/SqlServerDsc.Common/Public/Update-AvailabilityGroupReplica.Tests.ps1-86-111 (1)
86-111: Add parameter-set and parameter-property validation testsUpdate-AvailabilityGroupReplica is a public command and should include the standard parameter set and parameter property validation tests (Get-Command templates). Please add those contexts alongside the behavioral tests. As per coding guidelines, follow the unit-test templates.
source/Modules/SqlServerDsc.Common/Public/Get-SqlInstanceMajorVersion.ps1-1-11 (1)
1-11: Complete comment-based help and fix the default-value mismatchHelp text is missing DESCRIPTION/EXAMPLE/INPUTS, and it claims a default of
MSSQLSERVEReven thoughInstanceNameis mandatory with no default. Please update the help to match the actual parameter behavior and required sections. As per coding guidelines, comment-based help must include the full required sections.Also applies to: 18-20
source/Modules/SqlServerDsc.Common/Public/Split-FullSqlInstanceName.ps1-12-20 (1)
12-20: Add CmdletBinding and OutputType metadataPublic functions must be advanced cmdlets with
[CmdletBinding()]and explicit[OutputType()]. This function currently lacks both.🛠️ Proposed fix
function Split-FullSqlInstanceName { + [CmdletBinding()] + [OutputType([System.Collections.Hashtable])] param ( [Parameter(Mandatory = $true)] [ValidateNotNullOrEmpty()] [System.String] $FullSqlInstanceName )As per coding guidelines, public functions must include CmdletBinding and OutputType.
tests/Unit/SqlServerDsc.Common/Public/Update-AvailabilityGroupReplica.Tests.ps1-28-38 (1)
28-38: Fix unit-test bootstrap paths and CI env varThe build.ps1 and SMO.cs paths are off for this nested test folder, and
$env:SqlServerDscCIisn’t set/cleared as required. This will break bootstrapping on clean environments.🛠️ Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & (Join-Path -Path $PSScriptRoot -ChildPath '..\..\..\..\build.ps1') -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null @@ BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' @@ if (-not ('Microsoft.SqlServer.Management.Smo.Server' -as [Type])) { - Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\SMO.cs') } @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings and coding guidelines, please align with the standard unit-test setup requirements.
Also applies to: 50-84
source/Modules/SqlServerDsc.Common/Public/Get-SqlInstanceMajorVersion.ps1-23-29 (1)
23-29: Guard against missing instance IDs before reading SetupIf
$InstanceNameisn’t present underInstance Names\SQL,$sqlInstanceIdbecomes$nulland the code will read the shared Setup key (or emit a non-terminating error), potentially returning a misleading version. Add an explicit check and throw a localized error for unknown instances.🛠️ Proposed fix
$sqlInstanceId = (Get-ItemProperty -Path 'HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\Instance Names\SQL').$InstanceName +if (-not $sqlInstanceId) +{ + $errorMessage = $script:localizedData.SqlInstanceNotFound -f $InstanceName + New-InvalidResultException -Message $errorMessage +} $sqlVersion = (Get-ItemProperty -Path "HKLM:\SOFTWARE\Microsoft\Microsoft SQL Server\$sqlInstanceId\Setup").VersionAs per coding guidelines, ensure the error string is localized (add a key if needed).
tests/Unit/SqlServerDsc.Common/Public/Test-FeatureFlag.Tests.ps1-28-38 (1)
28-38: Fix unit-test bootstrap paths and CI env varThe current build.ps1 and SMO.cs paths resolve to non-existent locations from this nested folder, and the required
$env:SqlServerDscCIflag isn’t set/cleared in setup/teardown. This will break bootstrapping on clean environments.🛠️ Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & (Join-Path -Path $PSScriptRoot -ChildPath '..\..\..\..\build.ps1') -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null @@ BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' @@ if (-not ('Microsoft.SqlServer.Management.Smo.Server' -as [Type])) { - Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\SMO.cs') } @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings and coding guidelines, please align with the standard unit-test setup requirements.
Also applies to: 50-84
source/Modules/SqlServerDsc.Common/Public/Get-ServiceAccount.ps1-45-51 (1)
45-51: Avoid returning plaintext passwords
GetNetworkCredential().Passwordexposes secrets to the pipeline, which violates the guidance to avoid emitting sensitive data and to prefer PSCredential. Please keep the password as a SecureString/PSCredential and only convert to plaintext at the final external call site (if absolutely required).🛠️ Safer baseline change
- Password = $ServiceAccount.GetNetworkCredential().Password + Password = $ServiceAccount.PasswordAs per coding guidelines, avoid emitting secrets and prefer PSCredential/SecureString handling.
tests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1-28-38 (1)
28-38: Fix unit-test bootstrap paths and CI env varThe build.ps1 and SMO.cs paths are off by multiple levels for this nested test folder, and
$env:SqlServerDscCIisn’t set/cleared as required. This breaks bootstrapping on clean environments.🛠️ Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & (Join-Path -Path $PSScriptRoot -ChildPath '..\..\..\..\build.ps1') -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null @@ BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' @@ if (-not ('Microsoft.SqlServer.Management.Smo.Server' -as [Type])) { - Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\SMO.cs') } @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings and coding guidelines, please align with the standard unit-test setup requirements.
Also applies to: 50-84
source/Modules/SqlServerDsc.Common/Public/Split-FullSqlInstanceName.ps1-12-12 (1)
12-12: Add SqlDsc prefix to public command name or create backward-compatible aliasThis function is located in the Public directory and must follow the
Verb-SqlDsc{Noun}naming convention. Rename toSplit-SqlDscFullInstanceNameor, if backward compatibility is required, add a[Alias('Split-SqlDscFullInstanceName')]attribute and consider deprecating the old name.source/Modules/SqlServerDsc.Common/Public/Find-ExceptionByNumber.ps1-14-26 (1)
14-26: Missing required function attributes.Per coding guidelines, all functions must include
[CmdletBinding()]and[OutputType()]attributes.🔧 Add required attributes
function Find-ExceptionByNumber { - # Define parameters + [CmdletBinding()] + [OutputType([System.Boolean])] param (source/Modules/SqlServerDsc.Common/Public/Update-AvailabilityGroupReplica.ps1-8-15 (1)
8-15: Missing required function attributes.Per coding guidelines, all functions must include
[CmdletBinding()]and[OutputType()]attributes.🔧 Add required attributes
function Update-AvailabilityGroupReplica { + [CmdletBinding()] + [OutputType()] param (tests/Unit/SqlServerDsc.Common/Public/Get-SqlInstanceMajorVersion.Tests.ps1-50-72 (1)
50-72: Missing required environment variable for CI.Per SqlServerDsc guidelines, unit tests must add
$env:SqlServerDscCI = $truein theBeforeAllblock and remove it in theAfterAllblock.🔧 Add CI environment variable
BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc'And in AfterAll:
AfterAll { + Remove-Item -Path 'env:SqlServerDscCI' -ErrorAction 'SilentlyContinue' + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/ConvertTo-ServerInstanceName.Tests.ps1-50-72 (1)
50-72: Missing required environment variable for CI.Per SqlServerDsc guidelines, unit tests must add
$env:SqlServerDscCI = $truein theBeforeAllblock and remove it in theAfterAllblock.🔧 Add CI environment variable
BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc'And in AfterAll:
AfterAll { + Remove-Item -Path 'env:SqlServerDscCI' -ErrorAction 'SilentlyContinue' + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Split-FullSqlInstanceName.Tests.ps1-50-72 (1)
50-72: Missing required environment variable for CI.Per SqlServerDsc guidelines, unit tests must add
$env:SqlServerDscCI = $truein theBeforeAllblock and remove it in theAfterAllblock.🔧 Add CI environment variable
BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc'And in AfterAll:
AfterAll { + Remove-Item -Path 'env:SqlServerDscCI' -ErrorAction 'SilentlyContinue' + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Get-SqlInstanceMajorVersion.Tests.ps1-128-128 (1)
128-128: Variable defined outside BeforeAll causes scoping issue.
$mockInstance_InstanceIdis defined at line 128 outside any Pester block, which means it's evaluated at discovery time. However, it references$mockSqlMajorVersionand$mockInstanceNamewhich are defined insideBeforeAll(lines 88-89) and won't be available at discovery time.This variable should be moved inside the
BeforeAllblock after the variables it depends on.🐛 Move variable inside BeforeAll
BeforeAll { $mockSqlMajorVersion = 13 $mockInstanceName = 'TEST' + $mockInstance_InstanceId = "MSSQL$($mockSqlMajorVersion).$($mockInstanceName)" $mockGetItemProperty_MicrosoftSQLServer_InstanceNames_SQL = {And remove line 128.
source/Modules/SqlServerDsc.Common/Public/Import-Assembly.ps1-37-38 (1)
37-38: Public command name should follow{Verb}-SqlDsc{Noun}.
Import-AssemblyinPublicdoesn’t comply with the SqlDsc naming convention; consider renaming (and adding[Alias()]if you need backward compatibility).As per coding guidelines, public commands must follow the SqlDsc naming format.
source/Modules/SqlServerDsc.Common/Public/Connect-UncPath.ps1-20-21 (1)
20-21: Public command name should follow{Verb}-SqlDsc{Noun}.
Consider renaming to something likeConnect-SqlDscUncPath(optionally add[Alias()]for compatibility).As per coding guidelines, public command naming must follow the SqlDsc convention.
source/Modules/SqlServerDsc.Common/Public/Connect-SqlAnalysis.ps1-16-20 (1)
16-20: Align public command name and declare output type.
This public command should follow{Verb}-SqlDsc{Noun}and should declare[OutputType([Microsoft.AnalysisServices.Server])](or the correct return type).As per coding guidelines, public commands must use SqlDsc naming and declare output types.
source/Modules/SqlServerDsc.Common/Public/Connect-UncPath.ps1-22-55 (1)
22-55: SMB mapping creation should supportShouldProcess(+Force).
Creating an SMB mapping is a state change; addSupportsShouldProcesswithConfirmImpact, localized ShouldProcess messages, and theForcepattern.As per coding guidelines, state-changing functions must implement
ShouldProcessand theForcepattern.source/Modules/SqlServerDsc.Common/Public/Connect-SqlAnalysis.ps1-89-105 (1)
89-105: Non-feature-flag path doesn’t validate connection success.
IfConnect()fails silently, this branch still logs success and returns a disconnected object. Mirror the Connected check from the feature-flag path.🔧 Suggested fix
$analysisServicesObject = New-Object -TypeName 'Microsoft.AnalysisServices.Server' if ($analysisServicesObject) { $analysisServicesObject.Connect($analysisServicesDataSource) } - else - { - $errorMessage = $script:localizedData.FailedToConnectToAnalysisServicesInstance -f $analysisServiceInstance - - New-InvalidOperationException -Message $errorMessage - } - - Write-Verbose -Message ($script:localizedData.ConnectedToAnalysisServicesInstance -f $analysisServiceInstance) -Verbose + if ((-not $analysisServicesObject) -or ($analysisServicesObject.Connected -eq $false)) + { + $errorMessage = $script:localizedData.FailedToConnectToAnalysisServicesInstance -f $analysisServiceInstance + + New-InvalidOperationException -Message $errorMessage + } + else + { + Write-Verbose -Message ($script:localizedData.ConnectedToAnalysisServicesInstance -f $analysisServiceInstance) -Verbose + }source/Modules/SqlServerDsc.Common/Public/Disconnect-UncPath.ps1-8-19 (1)
8-19: Align public command metadata for state-changing removal.
Please addSupportsShouldProcess(withConfirmImpact+Forcepattern), declare[OutputType()](no output), and rename to a{Verb}-SqlDsc{Noun}format.As per coding guidelines, public commands must use SqlDsc naming and state-changing operations must implement
ShouldProcess.tests/Unit/SqlServerDsc.Common/Public/Connect-SqlAnalysis.Tests.ps1-28-84 (1)
28-84: Unit-test setup missing SqlServerDscCI flag and build.ps1 path looks off.
Unit tests must set$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll, and the build path should reach the repo root from this nested directory.Based on learnings, unit tests must set and clear `$env:SqlServerDscCI`, and the setup block should target the correct build script path.🔧 Suggested fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null @@ Import-Module -Name $script:subModulePath -ErrorAction 'Stop' + + $env:SqlServerDscCI = $true @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Disconnect-UncPath.Tests.ps1-28-84 (1)
28-84: Unit-test setup missing SqlServerDscCI flag and build.ps1 path looks off.
Unit tests must set$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll, and the build path should reach the repo root from this nested directory.Based on learnings, unit tests must set and clear `$env:SqlServerDscCI`, and the setup block should target the correct build script path.🔧 Suggested fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null @@ Import-Module -Name $script:subModulePath -ErrorAction 'Stop' + + $env:SqlServerDscCI = $true @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')source/Modules/SqlServerDsc.Common/Public/Test-ImpersonatePermissions.ps1-12-23 (1)
12-23: Missing[CmdletBinding()]and[OutputType()]attributes.Per coding guidelines, all functions must include
[CmdletBinding()]and[OutputType()]attributes.Proposed fix
function Test-ImpersonatePermissions { + [CmdletBinding()] + [OutputType([System.Boolean])] param (source/Modules/SqlServerDsc.Common/Public/Test-LoginEffectivePermissions.ps1-40-70 (1)
40-70: Missing[CmdletBinding()]and[OutputType()]attributes.Per coding guidelines, all functions must include
[CmdletBinding()]and[OutputType()]attributes.Proposed fix
function Test-LoginEffectivePermissions { + [CmdletBinding()] + [OutputType([System.Boolean])] param (tests/Unit/SqlServerDsc.Common/Public/Test-LoginEffectivePermissions.Tests.ps1-37-37 (1)
37-37: Incorrect path to build.ps1.The file is located at
tests/Unit/SqlServerDsc.Common/Public/, which is 4 levels deep from the repository root.Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $nullsource/Modules/SqlServerDsc.Common/Public/Test-ImpersonatePermissions.ps1-37-112 (1)
37-112: Verbose messages use hardcoded strings instead of localized strings.Per coding guidelines, all
Write-Verbosemessages must use localized string keys from$script:localizedData, not hardcoded strings.Example of expected pattern
Instead of:
Write-Verbose -Message ( 'The login "{0}" has impersonate any login permissions...' -f ... ) -VerboseUse:
Write-Verbose -Message ($script:localizedData.LoginHasImpersonateAnyLoginPermissions -f $loginName, $serverName, $instanceName)Also remove the
-Verboseswitch to respect the caller's verbose preference.tests/Unit/SqlServerDsc.Common/Public/Test-ImpersonatePermissions.Tests.ps1-37-37 (1)
37-37: Incorrect path to build.ps1.The file is located at
tests/Unit/SqlServerDsc.Common/Public/, which is 4 levels deep from the repository root.Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $nulltests/Unit/SqlServerDsc.Common/Public/Connect-UncPath.Tests.ps1-37-37 (1)
37-37: Incorrect path to build.ps1.The file is located at
tests/Unit/SqlServerDsc.Common/Public/Connect-UncPath.Tests.ps1, which is 4 levels deep from the repository root. The path should be../../../../build.ps1.Proposed fix
- & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $nullsource/Modules/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.ps1-56-63 (1)
56-63: Potential SQL injection vulnerability.The
$AvailabilityGroupNameand$AvailabilityReplicaNamevalues are directly interpolated into the SQL query string without sanitization. If these values contain malicious SQL, it could lead to SQL injection attacks.Consider using parameterized queries or at minimum escaping single quotes.
Proposed fix - escape single quotes
+ # Escape single quotes to prevent SQL injection + $safeAvailabilityGroupName = $AvailabilityGroupName -replace "'", "''" + $safeAvailabilityReplicaName = $AvailabilityReplicaName -replace "'", "''" + $queryToGetSeedingMode = " SELECT seeding_mode_desc FROM sys.availability_replicas ar INNER JOIN sys.availability_groups ag ON ar.group_id = ag.group_id - WHERE ag.name = '$AvailabilityGroupName' - AND ar.replica_server_name = '$AvailabilityReplicaName' + WHERE ag.name = '$safeAvailabilityGroupName' + AND ar.replica_server_name = '$safeAvailabilityReplicaName' "tests/Unit/SqlServerDsc.Common/Public/Test-ClusterPermissions.Tests.ps1-86-172 (1)
86-172: Add parameter set & parameter attribute validation tests for Test-ClusterPermissions.Public command unit tests must include parameter set validation and parameter property checks (using the standard templates). These are missing here.
As per coding guidelines, public command tests must include these validations.
tests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1-86-163 (1)
86-163: Add parameter set & parameter attribute validation tests for this public command.Public command unit tests require parameter set validation and parameter property tests, but this suite only covers behavioral cases. Please add the required
Get-Command-based templates.As per coding guidelines, public command tests must include these validations.
tests/Unit/SqlServerDsc.Common/Public/Get-PrimaryReplicaServerObject.Tests.ps1-86-156 (1)
86-156: Add parameter set & parameter attribute validation tests for Get-PrimaryReplicaServerObject.Public command unit tests must include parameter set validation and parameter property checks (using the standard templates). These are missing here.
As per coding guidelines, public command tests must include these validations.
source/Modules/SqlServerDsc.Common/Public/Get-PrimaryReplicaServerObject.ps1-1-22 (1)
1-22: Add[CmdletBinding()],[OutputType()], and full comment-based help.This function is missing
[CmdletBinding()],[OutputType()], and required help sections (.DESCRIPTION,.INPUTS,.OUTPUTS,.EXAMPLE).✅ Suggested scaffold
function Get-PrimaryReplicaServerObject { + [CmdletBinding()] + [OutputType([Microsoft.SqlServer.Management.Smo.Server])] param (As per coding guidelines, public functions must include full help metadata and OutputType.
tests/Unit/SqlServerDsc.Common/Public/Connect-Sql.Tests.ps1-50-84 (1)
50-84: Add SqlServerDscCI env setup/teardown for unit tests.Unit test guidelines require
$env:SqlServerDscCI = $trueinBeforeAlland removal inAfterAll.✅ Suggested update
BeforeAll { $script:moduleName = 'SqlServerDsc' @@ $PSDefaultParameterValues['Should:ModuleName'] = $script:subModuleName + + $env:SqlServerDscCI = $true } AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings, please align with the required unit test setup.
tests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1-50-84 (1)
50-84: Add SqlServerDscCI env setup/teardown for unit tests.Unit test guidelines require
$env:SqlServerDscCI = $trueinBeforeAlland removal inAfterAll, but this file doesn’t set or clear it.✅ Suggested update
BeforeAll { $script:moduleName = 'SqlServerDsc' @@ $PSDefaultParameterValues['Should:ModuleName'] = $script:subModuleName + + $env:SqlServerDscCI = $true } AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings, please align with the required unit test setup.
tests/Unit/SqlServerDsc.Common/Public/Test-ClusterPermissions.Tests.ps1-120-136 (1)
120-136: Use localized strings for expected errors.Unit tests should source expected messages from
$script:localizedDataviaInModuleScoperather than hardcoding strings. Please retrieve the localized key used byTest-ClusterPermissionsand format it for-ExpectedMessage.As per coding guidelines, unit tests should validate localized messages via
InModuleScope.tests/Unit/SqlServerDsc.Common/Public/Connect-Sql.Tests.ps1-86-589 (1)
86-589: Add parameter set & parameter attribute validation tests for Connect-SQL.Public command unit tests must include parameter set validation and parameter property checks (using the standard templates). These are missing here.
As per coding guidelines, public command tests must include these validations.
source/Modules/SqlServerDsc.Common/Public/Restart-SqlService.ps1-53-147 (1)
53-147: Protect service/cluster restarts with ShouldProcess + Force.This function performs state-changing operations (cluster/service restarts) but lacks
SupportsShouldProcess,ConfirmImpact, and the Force pattern. Please add ShouldProcess guards immediately before each state change and include theForceparameter pattern.As per coding guidelines, state-changing functions must implement ShouldProcess with Force support.
tests/Unit/SqlServerDsc.Common/Public/Test-ClusterPermissions.Tests.ps1-50-84 (1)
50-84: Add SqlServerDscCI env setup/teardown for unit tests.Unit test guidelines require
$env:SqlServerDscCI = $trueinBeforeAlland removal inAfterAll.✅ Suggested update
BeforeAll { $script:moduleName = 'SqlServerDsc' @@ $PSDefaultParameterValues['Should:ModuleName'] = $script:subModuleName + + $env:SqlServerDscCI = $true } AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings, please align with the required unit test setup.
tests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1-63-67 (1)
63-67: SMO stub path should reference../Stubs/SMO.cs.Per the unit test requirement, SMO stubs are loaded from
$PSScriptRoot/../Stubs/SMO.cs; the current path points to aStubsfolder underPublic, which is likely incorrect.✅ Suggested update
- Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\Stubs\SMO.cs')Based on learnings, please align with the SMO stub loading requirement.
tests/Unit/SqlServerDsc.Common/Public/Get-PrimaryReplicaServerObject.Tests.ps1-50-84 (1)
50-84: Add SqlServerDscCI env setup/teardown for unit tests.Unit test guidelines require
$env:SqlServerDscCI = $trueinBeforeAlland removal inAfterAll.✅ Suggested update
BeforeAll { $script:moduleName = 'SqlServerDsc' @@ $PSDefaultParameterValues['Should:ModuleName'] = $script:subModuleName + + $env:SqlServerDscCI = $true } AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')Based on learnings, please align with the required unit test setup.
source/Modules/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.ps1-63-98 (1)
63-98: Handle Robocopy exit codes 4–7 to prevent silent partial failures.
Exit codes 4–7 currently fall through unhandled, which masks incomplete copies or mismatches. Per Microsoft Robocopy documentation:
- Code 4: Mismatched files/directories (attributes differ)
- Code 5: Some files copied AND mismatched (partial success)
- Code 6: Additional files AND mismatches, no files copied
- Code 7: Files copied with mismatches AND additional files present
These scenarios indicate incomplete or problematic synchronization that should be reported to the caller. Add explicit handling with a warning or error and localized message.
source/Modules/SqlServerDsc.Common/Public/Invoke-InstallationMediaCopy.ps1-1-17 (1)
1-17: Add missing DESCRIPTION, INPUTS, and EXAMPLE sections to comment-based help, and format OUTPUTS with the return type.This public function is missing required help sections per coding guidelines: DESCRIPTION (minimum 40 characters), INPUTS, and EXAMPLE. The OUTPUTS section must follow the format of specifying the return type (
[System.String]) with a 1-line description, and should include INPUTS specifyingNone.since there are no pipeline-accepted parameters.📝 Suggested documentation scaffold
<# .SYNOPSIS Connects to the source using the provided credentials and then uses robocopy to download the installation media to a local temporary folder. + + .DESCRIPTION + Connects to the remote source share, copies the installation media to a + temporary local folder, and optionally returns the destination path. + + .PARAMETER SourcePath + Source path to be copied. + + .PARAMETER SourceCredential + The credentials to access the SourcePath. + + .PARAMETER PassThru + If used, returns the destination path as string. + + .INPUTS + None. + + .OUTPUTS + [System.String] + + Returns the destination path when PassThru is specified. + + .EXAMPLE + Invoke-InstallationMediaCopy -SourcePath '\\server\share\media' -SourceCredential $cred #>tests/Unit/SqlServerDsc.Common/Public/Restart-SqlClusterService.Tests.ps1-86-498 (1)
86-498: Add parameter set and parameter attribute validation tests.The public command
Restart-SqlClusterServiceis missing required parameter validation tests. Unit tests for public commands must include:
- Parameter set validation using
Get-Commandto verify the parameter set name and parameter list- Parameter property validation using
Get-Commandto verify individual parameter attributes (e.g.,Mandatoryflag forInstanceName)Refer to the parameter set validation and parameter properties test templates in the coding guidelines for implementation details.
tests/Unit/SqlServerDsc.Common/Public/Restart-SqlClusterService.Tests.ps1-28-84 (1)
28-84: Add$env:SqlServerDscCIflag to BeforeAll and remove in AfterAll.Unit tests must set
$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll. This is missing from the current setup block.Suggested fix
BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common'AfterAll { + Remove-Item -Path 'env:SqlServerDscCI' -ErrorAction 'SilentlyContinue' + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Restart-SqlService.Tests.ps1-87-479 (1)
87-479: Add parameter set and parameter attribute validation tests.
Restart-SqlServiceis a public command and requires parameter set validation tests usingGet-Commandand theParameterSetsproperty, plus parameter attribute validation tests (e.g., for theMandatoryflag onServerName). These test templates are missing entirely from the unit tests.tests/Unit/SqlServerDsc.Common/Public/Restart-SqlService.Tests.ps1-88-114 (1)
88-114: RemoveInModuleScopefrom stub function definitions in public command tests.Public command tests should not use
InModuleScopeto create stub functions. The guideline permitsInModuleScopeonly for accessing localized strings or creating internal class objects. Define stubs in test scope instead or use mocks with explicit module scoping outsideInModuleScopeblocks.tests/Unit/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.Tests.ps1-86-393 (1)
86-393: Add parameter set and parameter attribute validation tests.Public commands require parameter set validation and parameter property tests via
Get-Command. TheCopy-ItemWithRobocopycommand has two mandatory parameters (PathandDestinationPath) that need validation coverage. Add:
- A parameter set validation test using
Get-Command -Name 'Copy-ItemWithRobocopy'to verify the__AllParameterSetsparameter set structure- Parameter property validation tests to confirm the
Mandatoryattribute on both parametersSee the parameter validation test templates in the style guidelines.
source/Modules/SqlServerDsc.Common/Public/Invoke-InstallationMediaCopy.ps1-38-61 (1)
38-61: Wrap connect/copy flow in try/finally to guaranteeDisconnect-UncPathcleanup.If
Copy-ItemWithRobocopythrows an exception,Disconnect-UncPathnever executes, leaving the UNC connection and credentials active. Use try/finally to ensure cleanup happens regardless of whether the copy succeeds or fails.Suggested try/finally cleanup
- Connect-UncPath -RemotePath $SourcePath -SourceCredential $SourceCredential - - $SourcePath = $SourcePath.TrimEnd('/\') + Connect-UncPath -RemotePath $SourcePath -SourceCredential $SourceCredential + try + { + $SourcePath = $SourcePath.TrimEnd('/\') @@ - Copy-ItemWithRobocopy -Path $SourcePath -DestinationPath $mediaDestinationPath - - Disconnect-UncPath -RemotePath $SourcePath + Copy-ItemWithRobocopy -Path $SourcePath -DestinationPath $mediaDestinationPath + } + finally + { + Disconnect-UncPath -RemotePath $SourcePath + }source/Modules/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.ps1-1-25 (1)
1-25: Complete comment-based help and add OutputType declaration.The help block is missing required DESCRIPTION (40+ chars), INPUTS, OUTPUTS, and EXAMPLE sections. The function also lacks an
[OutputType()]declaration. Per coding guidelines, all functions must include full comment-based help with these sections and explicit output types.📝 Help and OutputType scaffold
<# .SYNOPSIS Copy folder structure using Robocopy. Every file and folder, including empty ones are copied. + + .DESCRIPTION + Copies the full directory tree from the source path to the destination path using Robocopy, + including empty folders and files. + + .PARAMETER Path + Source path to be copied. + + .PARAMETER DestinationPath + The path to the destination. + + .INPUTS + None. + + .OUTPUTS + None. + + .EXAMPLE + Copy-ItemWithRobocopy -Path 'C:\Source' -DestinationPath 'D:\Destination' #> function Copy-ItemWithRobocopy { [CmdletBinding()] + [OutputType()]tests/Unit/SqlServerDsc.Common/Public/Restart-SqlService.Tests.ps1-28-84 (1)
28-84: Add missing$env:SqlServerDscCIflag and fix relative paths in setup block.Add
$env:SqlServerDscCI = $truetoBeforeAlland remove it inAfterAll. Additionally, correct the relative paths: change$PSScriptRoot/../../build.ps1to$PSScriptRoot/../../../build.ps1(three levels up fromtests/Unit/SqlServerDsc.Common/Public/), and change$PSScriptRoot/Stubs/SMO.csto$PSScriptRoot/../../Stubs/SMO.cs(two levels up to reachtests/Unit/Stubs/).🛠️ Suggested fix
BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' $script:parentModule = Get-Module -Name $script:moduleName -ListAvailable | Select-Object -First 1 $script:subModulesFolder = Join-Path -Path $script:parentModule.ModuleBase -ChildPath 'Modules' $script:subModulePath = Join-Path -Path $script:subModulesFolder -ChildPath $script:subModuleName Import-Module -Name $script:subModulePath -ErrorAction 'Stop' Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath '..\..\..\TestHelpers\CommonTestHelper.psm1') # Loading SMO stubs. if (-not ('Microsoft.SqlServer.Management.Smo.Server' -as [Type])) { - Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs') -ChildPath 'SMO.cs') + Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath '../../Stubs') -ChildPath 'SMO.cs') } BeforeDiscovery { try { if (-not (Get-Module -Name 'DscResource.Test')) { # Assumes dependencies have been resolved, so if this module is not available, run 'noop' task. if (-not (Get-Module -Name 'DscResource.Test' -ListAvailable)) { # Redirect all streams to $null, except the error stream (stream 2) - & "$PSScriptRoot/../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null + & "$PSScriptRoot/../../../build.ps1" -Tasks 'noop' 3>&1 4>&1 5>&1 6>&1 > $null } # If the dependencies have not been resolved, this will throw an error. Import-Module -Name 'DscResource.Test' -Force -ErrorAction 'Stop' } } catch [System.IO.FileNotFoundException] { throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -ResolveDependency -Tasks noop" first.' } } AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Invoke-InstallationMediaCopy.Tests.ps1-28-84 (1)
28-84: Align unit-test setup block with required template (CI flag + build.ps1 path).
Add$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll, and ensure thebuild.ps1path resolves to the repo root (the current../../build.ps1is likely too shallow for this folder depth). Based on learnings, unit tests must set the CI flag and follow the standard setup block.🛠️ Suggested fix for the CI flag
BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' @@ AfterAll { + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinue $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
======================================
- Coverage 94% 94% -1%
======================================
Files 226 225 -1
Lines 11182 10761 -421
======================================
- Hits 10545 10141 -404
+ Misses 637 620 -17
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@tests/Unit/SqlServerDsc.Common/Public/Connect-Sql.Tests.ps1`:
- Around line 86-88: Add parameter-set validation tests for the public command
Connect-SQL: create a new Context 'When validating parameter sets' that uses
Get-Command -Name 'Connect-SQL').ParameterSets to locate each
ExpectedParameterSetName and assert its ParameterSetName and ToString()
(ParameterListAsString) match expected values; implement an It block (ForEach
over test cases) that selects the parameter set by $_.Name, then asserts
$result.ParameterSetName | Should -Be $ExpectedParameterSetName and
$result.ParameterListAsString | Should -Be $ExpectedParameters to validate
parameter sets and parameter property strings for Connect-SQL.
- Around line 50-84: The tests are missing the CI environment variable setup; in
BeforeAll set $env:SqlServerDscCI = 'true' to enable CI-specific behavior for
the suite, and in AfterAll remove it (e.g. Remove-Item Env:SqlServerDscCI or
clear the variable) so it does not leak into other tests; update the BeforeAll
and AfterAll blocks in Connect-Sql.Tests.ps1 accordingly.
- Around line 88-156: The New-Object mock is compensating for missing SMO stubs;
update SMO.cs to add the missing Server/ServerConnection members (class Server
and its ConnectionContext/ServerConnection should expose Status, Connect(),
Disconnect(), Refresh(), ServerInstance, LoginSecure, Login, SecurePassword,
ConnectAsUser, ConnectAsUserName, ConnectAsUserPassword, ConnectTimeout,
EncryptConnection, ApplicationName) so the test can construct a concrete stub
Server; then remove the large Mock -CommandName New-Object block in
Connect-Sql.Tests.ps1 and instantiate the stub Server directly, configuring its
ConnectionContext properties and methods to match how production code uses them
(e.g., set ServerInstance, LoginSecure, SecurePassword, ConnectTimeout,
EncryptConnection, ApplicationName and implement
Connect/Disconnect/Refresh/Status behavior).
In `@tests/Unit/SqlServerDsc.Common/Public/Disconnect-UncPath.Tests.ps1`:
- Around line 86-88: Add unit tests for the Disconnect-UncPath public command
that validate its parameter sets and mandatory attributes: use Get-Command -Name
'Disconnect-UncPath' to assert the correct ParameterSetNames include the
expected string(s) and verify each parameter’s Mandatory flag (e.g., ensure the
path parameter is marked Mandatory in the intended parameter set). Update the
tests in Disconnect-UncPath.Tests.ps1 to include Get-Command based assertions
for ParameterSetName and ParameterAttributes so the parameter set string and
mandatory flags are covered by unit tests.
- Around line 50-84: Add setting and cleanup of the CI environment flag inside
the existing test fixture setup/teardown: in the BeforeAll block set
$env:SqlServerDscCI = $true (so CI-specific behavior is enabled during tests)
and in the AfterAll block remove it (e.g., Remove-Item Env:\SqlServerDscCI or
set to $null) to restore environment state; update the BeforeAll and AfterAll
blocks that contain the existing setup (look for the BeforeAll/AfterAll
functions that reference $script:moduleName, Import-Module, and Get-Module ...
Remove-Module) to include these two operations.
- Around line 90-105: The test currently defines a stub for Remove-SmbMapping
inside an InModuleScope block which prevents Pester from properly intercepting
the public command; move the stub out of InModuleScope and define the function
Remove-SmbMapping at test scope (so the Mock -CommandName Remove-SmbMapping can
intercept calls from Disconnect-UncPath), and update the AfterAll cleanup to
remove the function:/Remove-SmbMapping as before; ensure you still throw the
same StubNotImplemented message in the test-scope stub and keep Mock usage
unchanged.
In `@tests/Unit/SqlServerDsc.Common/Public/Find-ExceptionByNumber.Tests.ps1`:
- Around line 50-84: In the BeforeAll block set the CI environment flag by
assigning $env:SqlServerDscCI = $true so CI-specific behavior/path resolution is
enabled for tests, and in the AfterAll block remove that flag (e.g., remove or
clear $env:SqlServerDscCI) to restore environment; update the existing BeforeAll
and AfterAll functions referenced in the test file so the CI flag is set at
startup and removed at teardown.
- Around line 86-107: Add parameter-set and parameter-property validation tests
for the public command Find-ExceptionByNumber: create a new Context or Describe
block that asserts required parameter sets and validation attributes by testing
calls with missing mandatory parameters (invoke Find-ExceptionByNumber without
-ExceptionToSearch and without -ErrorNumber and assert they throw/validate),
test parameter property validation by passing invalid types or null/empty for
-ExceptionToSearch and invalid non-integer/negative values for -ErrorNumber and
assert the cmdlet rejects them, and include explicit tests that the cmdlet
accepts correct types for -ExceptionToSearch (System.Exception) and -ErrorNumber
(int) so the behavior and parameter metadata are covered.
In
`@tests/Unit/SqlServerDsc.Common/Public/Get-PrimaryReplicaServerObject.Tests.ps1`:
- Around line 86-156: Add standard parameter-set validation for the public
command Get-PrimaryReplicaServerObject: update the test file to call Get-Command
-Name 'Get-PrimaryReplicaServerObject' to extract its ParameterSets and assert
the expected parameter set names and counts (using Should assertions), and
include assertions that required parameters appear in each parameter set; place
these checks as a new It block alongside the existing Context blocks so the unit
tests validate the command's parameter sets and required parameters.
- Around line 50-84: Add setting and cleanup of the SqlServerDscCI environment
flag: in the BeforeAll block (function/section named BeforeAll) set
$env:SqlServerDscCI = $true before importing modules, and in the AfterAll block
(section named AfterAll) remove/unset it (e.g., Remove-Item Env:\SqlServerDscCI
or $env:SqlServerDscCI = $null) after removing modules; ensure the env variable
is only set for the duration of the test and cleaned up in AfterAll to follow
the unit test guidelines.
In
`@tests/Unit/SqlServerDsc.Common/Public/Invoke-InstallationMediaCopy.Tests.ps1`:
- Around line 50-83: Add setting and cleanup of the SqlServerDscCI environment
variable: in the BeforeAll block set $env:SqlServerDscCI = $true so tests run
with CI behavior enabled, and in the AfterAll block remove it (e.g., Remove-Item
Env:SqlServerDscCI or clear $env:SqlServerDscCI) to restore environment; update
the existing BeforeAll and AfterAll blocks (referenced by the BeforeAll and
AfterAll symbols in this file) to include these statements.
- Around line 87-224: Add a parameter set validation test for the public command
Invoke-InstallationMediaCopy inside the existing Describe
'SqlServerDsc.Common\Invoke-InstallationMediaCopy' suite: use Get-Command -Name
Invoke-InstallationMediaCopy and assert its ParameterSets contains the expected
sets (e.g., the default and any alternate sets that include
SourcePath/SourceCredential/PassThru) so the public function's parameter set
behavior is validated; place the assertions as a separate It block (e.g.,
"Should have correct parameter sets") alongside the other Contexts to follow the
project’s standard test template.
In `@tests/Unit/SqlServerDsc.Common/Public/Invoke-SqlScript.Tests.ps1`:
- Around line 86-98: The tests for the public command Invoke-SqlScript are
missing required parameter set and parameter attribute validations; add a
Context that queries (Get-Command -Name 'Invoke-SqlScript').ParameterSets to
assert the '__AllParameterSets' ParameterSetName and that its ToString() equals
'[-ServerInstance] <String> [-Query] <String> [-InputFile] <String>
[<CommonParameters>]', and add a Context that inspects (Get-Command -Name
'Invoke-SqlScript').Parameters['ServerInstance'] to assert the
Attributes.Mandatory is true; place these specs alongside the existing BeforeAll
setup in Invoke-SqlScript.Tests.ps1 so the public command conforms to the
required unit-test templates.
In `@tests/Unit/SqlServerDsc.Common/Public/Restart-SqlClusterService.Tests.ps1`:
- Around line 95-98: The test is calling the public command
Restart-SqlClusterService inside an InModuleScope wrapper; remove the
InModuleScope and invoke Restart-SqlClusterService directly inside the It block
(preserving the same parameters, e.g. InstanceName 'MSSQLSERVER' and ErrorAction
'Stop'), and make the same change for other occurrences in this file so public
command tests no longer use InModuleScope.
- Around line 50-84: The tests are missing the CI toggle env var; set
$env:SqlServerDscCI = 'true' inside the BeforeAll block (near the existing setup
of $script:moduleName/$script:subModuleName) to ensure tests run in CI mode, and
remove/unset it in AfterAll (near the existing PSDefaultParameterValues removals
and module cleanup) by removing the env var or setting it to $null so it does
not affect other tests.
In `@tests/Unit/SqlServerDsc.Common/Public/Test-ImpersonatePermissions.Tests.ps1`:
- Around line 86-168: Add unit tests that validate the public command parameter
sets and parameter attribute validation for Test-ImpersonatePermissions: create
a new Context (e.g., "Parameter validation") that uses Get-Command -Name
'Test-ImpersonatePermissions' to assert expected ParameterSets and then assert
parameter attributes (Mandatory, Position, ValidateNotNullOrEmpty, etc.) for
parameters ServerObject and SecurableName using the standard Get-Command
templates used elsewhere in the repo; ensure you reference
Test-ImpersonatePermissions, ServerObject and SecurableName in the new tests and
follow the same Mock/Should patterns and naming as existing tests.
- Around line 50-83: The BeforeAll and AfterAll blocks in
Test-ImpersonatePermissions.Tests.ps1 must set and unset the CI toggle; add
setting of the environment variable $env:SqlServerDscCI = 'true' at the start of
the BeforeAll block (near the existing BeforeAll initialization code where
$script:moduleName and $script:subModuleName are set) and remove it in the
AfterAll block (call Remove-Item Env:\SqlServerDscCI or set $env:SqlServerDscCI
= $null) alongside the existing PSDefaultParameterValues cleanup to ensure tests
behave consistently in CI and locally.
In
`@tests/Unit/SqlServerDsc.Common/Public/Update-AvailabilityGroupReplica.Tests.ps1`:
- Around line 50-83: The tests are missing the CI environment variable
setup/teardown; in the BeforeAll block add setting the environment variable
($env:SqlServerDscCI = $true) so tests run under CI mode, and in the AfterAll
block remove it (e.g., remove the Env:SqlServerDscCI entry or set to $null) to
clean up; update the BeforeAll and AfterAll sections around the existing module
import/unload logic to include these lines next to other test scaffolding like
$PSDefaultParameterValues and Get-Module cleanup.
♻️ Duplicate comments (2)
tests/Unit/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.Tests.ps1 (2)
50-84: Set and clear$env:SqlServerDscCIin test setup/teardown.Unit tests must enable the CI flag in
BeforeAlland remove it inAfterAll, ideally before module import to ensure consistent module behavior.✅ Suggested fix
BeforeAll { + $env:SqlServerDscCI = $true $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' @@ } AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') @@ # Remove module common test helper. Get-Module -Name 'CommonTestHelper' -All | Remove-Module -Force + + Remove-Item -Path 'Env:SqlServerDscCI' -ErrorAction SilentlyContinue }As per coding guidelines, unit tests must set and remove the
SqlServerDscCIflag.
86-188: Add required parameter set and parameter property validation tests.Public command unit tests must include parameter set validation and parameter attribute (e.g., Mandatory) checks; these are currently missing.
✅ Suggested additions (template)
Describe 'SqlServerDsc.Common\Copy-ItemWithRobocopy' -Tag 'CopyItemWithRobocopy' { BeforeAll { $mockRobocopyExecutableName = 'Robocopy.exe' @@ } + Context 'When validating parameter sets' { + It 'Should have the correct parameters in parameter set __AllParameterSets' -ForEach @( + @{ + ExpectedParameterSetName = '__AllParameterSets' + ExpectedParameters = '[-Path] <String> [-DestinationPath] <String> [<CommonParameters>]' + } + ) { + $result = (Get-Command -Name 'Copy-ItemWithRobocopy').ParameterSets | + Where-Object -FilterScript { $_.Name -eq $ExpectedParameterSetName } | + Select-Object -Property @( + @{ Name = 'ParameterSetName'; Expression = { $_.Name } }, + @{ Name = 'ParameterListAsString'; Expression = { $_.ToString() } } + ) + + $result.ParameterSetName | Should -Be $ExpectedParameterSetName + $result.ParameterListAsString | Should -Be $ExpectedParameters + } + } + + Context 'When validating parameter properties' { + It 'Should have Path as a mandatory parameter' { + $parameterInfo = (Get-Command -Name 'Copy-ItemWithRobocopy').Parameters['Path'] + $parameterInfo.Attributes.Mandatory | Should -BeTrue + } + + It 'Should have DestinationPath as a mandatory parameter' { + $parameterInfo = (Get-Command -Name 'Copy-ItemWithRobocopy').Parameters['DestinationPath'] + $parameterInfo.Attributes.Mandatory | Should -BeTrue + } + } + Context 'When Copy-ItemWithRobocopy is called it should return the correct arguments' {As per coding guidelines, public command tests must include parameter set and parameter property validation.
🟡 Minor comments (8)
tests/Unit/SqlServerDsc.Common/Public/Invoke-SqlScript.Tests.ps1-99-239 (1)
99-239: RenameContextblocks to start with “When”.Context descriptions should begin with “When”. Please rename each
Contextin this file (e.g., “When Invoke-SqlScript fails to import SQLPS module”, etc.).As per coding guidelines,
Contextdescriptions must start with “When”.tests/Unit/SqlServerDsc.Common/Public/Invoke-SqlScript.Tests.ps1-50-84 (1)
50-84: Add SqlServerDscCI env var setup/teardown.Unit tests in this repo must set
$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll. Please add the required setup/cleanup.✅ Suggested fix
BeforeAll { $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common' + $env:SqlServerDscCI = $true @@ AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') + + Remove-Item -Path Env:SqlServerDscCI -ErrorAction SilentlyContinueBased on learnings, please align with the unit test environment requirement.
tests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1-74-84 (1)
74-84: Missing removal of CI environment variable inAfterAll.To match the required
$env:SqlServerDscCI = $truesetup, the variable must be removed inAfterAllto prevent spillover to other tests.Proposed fix
AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') + Remove-Item -Path 'env:SqlServerDscCI' -ErrorAction 'SilentlyContinue' + # Unload the module being tested so that it doesn't impact any other tests. Get-Module -Name $script:subModuleName -All | Remove-Module -Forcetests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1-50-72 (1)
50-72: Missing required CI environment variable.Per SqlServerDsc guidelines, unit tests must set
$env:SqlServerDscCI = $truein theBeforeAllblock. This environment variable is used to signal that tests are running in a CI context.Proposed fix
BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc' $script:subModuleName = 'SqlServerDsc.Common'tests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1-74-84 (1)
74-84: Add removal of$env:SqlServerDscCIin AfterAll.Per coding guidelines, the environment variable set in
BeforeAllmust be removed inAfterAllto prevent spillover to other tests.🔧 Proposed fix
AfterAll { $PSDefaultParameterValues.Remove('InModuleScope:ModuleName') $PSDefaultParameterValues.Remove('Mock:ModuleName') $PSDefaultParameterValues.Remove('Should:ModuleName') + Remove-Item -Path 'env:SqlServerDscCI' + # Unload the module being tested so that it doesn't impact any other tests. Get-Module -Name $script:subModuleName -All | Remove-Module -Forcetests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1-22-23 (1)
22-23: Remove unnecessary suppression attribute.The
PSAvoidUsingConvertToSecureStringWithPlainTextsuppression is not needed asConvertTo-SecureStringis not used in this test file.🧹 Proposed fix
-# Suppressing this rule because ConvertTo-SecureString is used to simplify the tests. -[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingConvertToSecureStringWithPlainText', '')] # Suppressing this rule because Script Analyzer does not understand Pester's syntax. [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param ()tests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1-50-72 (1)
50-72: Remove unnecessary SMO stubs and add$env:SqlServerDscCIenvironment variable setup and cleanup.
Start-SqlSetupProcessdoes not use SMO types—it only callsStart-ProcessandWait-Process—so the SMO stub loading at lines 63-67 should be removed. Additionally, unit tests must set$env:SqlServerDscCI = $trueinBeforeAlland remove it inAfterAll.🔧 Proposed fixes
In
BeforeAllblock (line 50), add after opening brace:BeforeAll { + $env:SqlServerDscCI = $true + $script:moduleName = 'SqlServerDsc'Remove SMO stub loading (lines 63-67):
- # Loading SMO stubs. - if (-not ('Microsoft.SqlServer.Management.Smo.Server' -as [Type])) - { - Add-Type -Path (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath '..\..\Stubs') -ChildPath 'SMO.cs') - } -In
AfterAllblock (line 74), add cleanup:AfterAll { + Remove-Item -Path Env:\SqlServerDscCI -ErrorAction 'SilentlyContinue' + $PSDefaultParameterValues.Remove('InModuleScope:ModuleName')tests/Unit/SqlServerDsc.Common/Public/Copy-ItemWithRobocopy.Tests.ps1-302-305 (1)
302-305: Keep assertions insideItblocks (moveShould -Invokeout ofAfterEach).Assertions in
AfterEachviolate the rule that assertions must be insideItblocks. Move thoseShould -Invokechecks into eachIt.✅ Suggested fix
- AfterEach { - Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It - Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It - } @@ It 'Should finish successfully with exit code 3' { $mockStartSqlSetupProcessExitCode = 3 @@ $null = Copy-ItemWithRobocopy `@copyItemWithRobocopyParameter` -ErrorAction 'Stop' + + Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It } } @@ - AfterEach { - Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It - Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It - } @@ It 'Should finish successfully with exit code 1' { $mockStartSqlSetupProcessExitCode = 1 @@ $null = Copy-ItemWithRobocopy `@copyItemWithRobocopyParameter` -ErrorAction 'Stop' + + Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It } @@ It 'Should finish successfully with exit code 2' { $mockStartSqlSetupProcessExitCode = 2 @@ $null = Copy-ItemWithRobocopy `@copyItemWithRobocopyParameter` -ErrorAction 'Stop' + + Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It } @@ It 'Should finish successfully with exit code 3' { $mockStartSqlSetupProcessExitCode = 3 @@ $null = Copy-ItemWithRobocopy `@copyItemWithRobocopyParameter` -ErrorAction 'Stop' + + Should -Invoke -CommandName Get-Command -Exactly -Times 1 -Scope It + Should -Invoke -CommandName Start-Process -Exactly -Times 1 -Scope It }As per coding guidelines, assertions should only live inside
Itblocks.Also applies to: 355-358
🧹 Nitpick comments (4)
tests/Unit/SqlServerDsc.Common/Public/Update-AvailabilityGroupReplica.Tests.ps1 (1)
89-96: Prefer::new()for .NET types instead ofNew-Object.This aligns with the repo’s PowerShell best practices and keeps style consistent. As per coding guidelines, prefer
::new()for .NET types.♻️ Proposed refactor
- $availabilityReplica = New-Object -TypeName Microsoft.SqlServer.Management.Smo.AvailabilityReplica + $availabilityReplica = [Microsoft.SqlServer.Management.Smo.AvailabilityReplica]::new() @@ - $availabilityReplica = New-Object -TypeName Microsoft.SqlServer.Management.Smo.AvailabilityReplica + $availabilityReplica = [Microsoft.SqlServer.Management.Smo.AvailabilityReplica]::new()tests/Unit/SqlServerDsc.Common/Public/Test-AvailabilityReplicaSeedingModeAutomatic.Tests.ps1 (1)
86-119: Missing parameter set validation tests.Per coding guidelines, all public commands require parameter set validation tests. Consider adding a
Contextblock at the beginning of theDescribeto validate the command's parameter sets and parameter properties using the standard template.Example parameter set validation template
Context 'When command has the correct parameters' { It 'Should have the correct parameters in parameter set <ExpectedParameterSetName>' -ForEach @( @{ ExpectedParameterSetName = '__AllParameterSets' ExpectedParameters = '[-ServerName] <string> [-InstanceName] <string> [-AvailabilityGroupName] <string> [-AvailabilityReplicaName] <string> [<CommonParameters>]' } ) { $result = (Get-Command -Name 'Test-AvailabilityReplicaSeedingModeAutomatic').ParameterSets | Where-Object -FilterScript { $_.Name -eq $ExpectedParameterSetName } | Select-Object -Property @( @{ Name = 'ParameterSetName'; Expression = { $_.Name } }, @{ Name = 'ParameterListAsString'; Expression = { $_.ToString() } } ) $result.ParameterSetName | Should -Be $ExpectedParameterSetName $result.ParameterListAsString | Should -Be $ExpectedParameters } }tests/Unit/SqlServerDsc.Common/Public/Start-SqlSetupProcess.Tests.ps1 (1)
1-3: Consider making the .SYNOPSIS specific to the tested function.The synopsis "Unit test for helper functions in module SqlServerDsc.Common" is generic. It should describe that this file tests the
Start-SqlSetupProcessfunction specifically.📝 Suggested improvement
<# .SYNOPSIS - Unit test for helper functions in module SqlServerDsc.Common. + Unit test for Start-SqlSetupProcess in module SqlServerDsc.Common.tests/Unit/SqlServerDsc.Common/Public/Get-PrimaryReplicaServerObject.Tests.ps1 (1)
94-119: Avoidparam()inside-MockWithscriptblocks.Pester auto-binds parameters for mocked commands; the explicit
param()block is discouraged here. As per coding guidelines, ...♻️ Suggested simplification
$mockConnectSql = { - param - ( - [Parameter()] - [System.String] - $ServerName, - - [Parameter()] - [System.String] - $InstanceName - ) - $mock = @( ( New-Object -TypeName Object | Add-Member -MemberType NoteProperty -Name 'DomainInstanceName' -Value $ServerName -PassThru ) )
|
@johlju, I'm not sure why the integration test fails. The code is no different, just moved out to individual files. |
They fail intermittently during different times of day due to hosted-agents loosing resources (most notably when US wakes up). |
|
Hi there,
So it should probably be tested first, and any issue brought back to sampler before "forking" this approach. |
|
The suggestion in this PR is that it is Sampler that is building the module through build.yaml, and the pattern has been tested before. These functions in SqlServerDsc.Common are slowly being converted to public or private functions (or moved to DscResource.Common if it more generic). But that work is primarily done as the resources are being converted to class-based resources, the goal is that we shouldn't have a SqlServerDsc.Common at all. We could convert some to public functions today regardless of resources being MOF- och class-based but that sometime requires a breakdown of the function because it sometimes need be split into more than one public function (and sometimes a private function) and then make sure the resources uses the new public functions. That is work being do too, but slowly. 🙂
Agree, but in this case we shouldn't add more maintenance by adding a new repo as we eventually will remove the module altogether. We also try to move things that is applicable to DscResource.Common first. The benefit this helps with in the short run, is easier reading the source code, finding the function in its individual .ps1 file than search for it in a large source .psm1 which this PR suggests to move away from. |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 53 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-hughes).
|
@dan-hughes just two comments from coderabbitai that catched a bug and a splatt that isn't using the full parameter name. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@tests/Unit/SqlServerDsc.Common/Public/Test-LoginEffectivePermissions.Tests.ps1`:
- Around line 50-84: Add setting and cleanup of the CI environment flag in the
test fixture: in the BeforeAll block (around the existing setup where
$script:moduleName and imports are done) set $env:SqlServerDscCI = $true so
CI-only logic runs during tests, and in the AfterAll block remove it (use
Remove-Item Env:\SqlServerDscCI or $env:SqlServerDscCI = $null) alongside the
existing PSDefaultParameterValues cleanup and module unload logic; update the
BeforeAll and AfterAll that contain the existing Import-Module and
Get-Module/Remove-Module calls to include these changes.
- Around line 86-197: Add parameter-set and parameter-attribute validation tests
for the public command Test-LoginEffectivePermissions: create a new Context
block "When validating parameter sets" that uses (Get-Command -Name
'Test-LoginEffectivePermissions').ParameterSets to assert expected parameter set
names and their ParameterListAsString (e.g. compare to '__AllParameterSets' and
the expected parameter string), and a Context block "When validating parameters"
that inspects (Get-Command -Name
'Test-LoginEffectivePermissions').Parameters['LoginName'] (and any other
required params) to assert Attributes.Mandatory is $true; place these contexts
alongside the existing Describe block tests so they run with the unit tests and
follow the same Should style and mock setup.
|
Done. |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dan-hughes).
3 participants
Pull Request (PR) description
Move functions from SqlServerDsc.Common to separate files. Use ModuleBuilder to build the .Common module.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is