Add integration test for ConvertTo-SqlDscServerPermission command#2305
Merged
Add integration test for ConvertTo-SqlDscServerPermission command#2305
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughAdds integration tests for ConvertTo-SqlDscServerPermission, updates the CI pipeline to include the new test, documents the command in the integration tests README, and adjusts the changelog to reflect the new test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…test Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Co-authored-by: johlju <7189721+johlju@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add integration test for ConvertTo-SqlDscServerPermission
Add integration test for ConvertTo-SqlDscServerPermission command
Oct 12, 2025
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1 (1)
41-41: Harden tests: fail-fast on errors and avoid false positives when prerequisites are missing.
- Use -ErrorAction 'Stop' on Connect/Disconnect to surface failures immediately.
- Several It-blocks silently pass when no permission data is returned (guarded by if (...) without an else). Mark these as skipped to avoid false positives.
- Prefer checking existence (e.g., Test-SqlDscIsLogin/Test-SqlDscIsRole) and skipping accordingly, instead of using -ErrorAction 'SilentlyContinue'.
Apply examples below (pattern applies to all guarded It-blocks):
- $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential + $script:serverObject = Connect-SqlDscDatabaseEngine -InstanceName $script:mockInstanceName -Credential $script:mockSqlAdminCredential -ErrorAction 'Stop'- Disconnect-SqlDscDatabaseEngine -ServerObject $script:serverObject + Disconnect-SqlDscDatabaseEngine -ServerObject $script:serverObject -ErrorAction 'Stop'Example for 'sa' context (repeat the else-branch pattern in other It-blocks that guard with if ($serverPermissionInfo)):
- # Only proceed if we have permission data to work with - if ($serverPermissionInfo) { + # Only proceed if we have permission data to work with + if ($serverPermissionInfo) { $result = ConvertTo-SqlDscServerPermission -ServerPermissionInfo $serverPermissionInfo # Validate the result structure $result | Should -Not -BeNullOrEmpty foreach ($permission in $result) { $permission.State | Should -Not -BeNullOrEmpty $permission.Permission | Should -Not -BeNullOrEmpty $permission.State | Should -BeIn @('Grant', 'Deny', 'GrantWithGrant') } - } + } + else { + Set-ItResult -Skipped -Because 'No ServerPermissionInfo returned for principal "sa" in this environment.' + }For system login case, prefer existence check and fail-fast:
- # Get permissions for NT AUTHORITY\SYSTEM - $serverPermissionInfo = Get-SqlDscServerPermission -ServerObject $script:serverObject -Name 'NT AUTHORITY\SYSTEM' -ErrorAction 'SilentlyContinue' + # Proceed only if the login exists; keeps -ErrorAction 'Stop' semantics + if (Test-SqlDscIsLogin -ServerObject $script:serverObject -Name 'NT AUTHORITY\SYSTEM') + { + $serverPermissionInfo = Get-SqlDscServerPermission -ServerObject $script:serverObject -Name 'NT AUTHORITY\SYSTEM' -ErrorAction 'Stop' + } + else + { + Set-ItResult -Skipped -Because 'Login "NT AUTHORITY\SYSTEM" not present on this instance.' + return + }This keeps tests deterministic and aligned with the integration test guidelines (use -ErrorAction 'Stop'; avoid silently passing when preconditions are unmet). Based on guidelines
Also applies to: 45-45, 71-87, 93-109, 115-133, 139-157, 161-185, 191-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)azure-pipelines.yml(1 hunks)tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1(1 hunks)tests/Integration/Commands/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow Requirements
- Run PowerShell script files from repository root
- Setup build and test environment (once per
pwshsession):./build.ps1 -Tasks noop- Build project before running tests:
./build.ps1 -Tasks build- Always run tests in new
pwshsession:Invoke-Pester -Path @({test paths}) -Output DetailedFile Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow instructions over existing code patterns
- Follow PowerShell style and test guideline instructions strictly
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- DSC resources should always be created as class-based resources
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
tests/Integration/Commands/README.mdtests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1azure-pipelines.ymlCHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
tests/Integration/Commands/README.mdCHANGELOG.md
{**/*.ps1,**/*.psm1,**/*.psd1}
⚙️ CodeRabbit configuration file
{**/*.ps1,**/*.psm1,**/*.psd1}: # PowerShell GuidelinesNaming
- Use descriptive names (3+ characters, no abbreviations)
- Functions: PascalCase with Verb-Noun format using approved verbs
- Parameters: PascalCase
- Variables: camelCase
- Keywords: lower-case
- Classes: PascalCase
- Include scope for script/global/environment variables:
$script:,$global:,$env:File naming
- Class files:
###.ClassName.ps1format (e.g.001.SqlReason.ps1,004.StartupParameters.ps1)Formatting
Indentation & Spacing
- Use 4 spaces (no tabs)
- One space around operators:
$a = 1 + 2- One space between type and variable:
[String] $name- One space between keyword and parenthesis:
if ($condition)- No spaces on empty lines
- Try to limit lines to 120 characters
Braces
- Newline before opening brace (except variable assignments)
- One newline after opening brace
- Two newlines after closing brace (one if followed by another brace or continuation)
Quotes
- Use single quotes unless variable expansion is needed:
'text'vs"text $variable"Arrays
- Single line:
@('one', 'two', 'three')- Multi-line: each element on separate line with proper indentation
- Do not use the unary comma operator (
,) in return statements to force
an arrayHashtables
- Empty:
@{}- Each property on separate line with proper indentation
- Properties: Use PascalCase
Comments
- Single line:
# Comment(capitalized, on own line)- Multi-line:
<# Comment #>format (opening and closing brackets on own line), and indent text- No commented-out code
Comment-based help
- Always add comment-based help to all functions and scripts
- Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections before function/class
- Comment-based help indentation: keywords 4 spaces, text 8 spaces
- Include examples for all parameter sets and combinations
- INPUTS: List each pipeline‑accepted type (one per line) with a 1‑line description...
Files:
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit configuration file
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test verbose messages, debug messages or parameter binding behavior
- Pass all mandatory parameters to avoid prompts
Requirements
- Inside
Itblocks, assign unused return objects to$null(unless part of pipeline)- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Avoid try-catch-finally for cleanup, use
AfterAllorAfterEach- Avoid unnecessary remove/recreate cycles
Naming
- One
Describeblock per file matching the tested entity nameContextdescriptions start with 'When'Itdescriptions start with 'Should', must not contain 'when'- Mock variables prefix: 'mock'
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each class method = separate
Contextblock- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEach- Use
-BeTrue/-BeFalsenever-Be $true/-Be $false- Never use
Assert-MockCalled, useShould -Invokeinstead- No
Should -Not -Throw- invoke commands directly- Never add an empty
-MockWithblock- Omit
-MockWithwhen returning$null- Set
$PSDefaultParameterValuesforMock:ModuleName,Should:ModuleName,InModuleScope:ModuleName- Omit
-ModuleNameparameter on Pester commands- Never use
Mockinside `InModuleSc...
Files:
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1
tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1
⚙️ CodeRabbit configuration file
tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1: # Integration Tests GuidelinesRequirements
- Location Commands:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1- Location Resources:
tests/Integration/Resources/{ResourceName}.Integration.Tests.ps1- No mocking - real environment only
- Cover all scenarios and code paths
- Use
Get-ComputerNamefor computer names in CI- Avoid
ExpectedMessageforShould -Throwassertions- Only run integration tests in CI unless explicitly instructed.
- Call commands with
-Forceparameter where applicable (avoids prompting).- Use
-ErrorAction 'Stop'on commands so failures surface immediatelyRequired Setup Block
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param () 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 } # 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.' } } BeforeAll { $script:moduleName = '{MyModuleName}' Import-Module -Name $script:moduleName -Force -ErrorAction 'Stop' }
Files:
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format issue #<issue_number>
- No empty lines between list items in same section
- Skip adding entry if same change already exists in Unreleased section
- No duplicate sections or items in Unreleased section
Files:
CHANGELOG.md
🧠 Learnings (10)
📚 Learning: 2025-09-16T16:35:31.909Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines.instructions.md:0-0
Timestamp: 2025-09-16T16:35:31.909Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Add integration tests for all public commands (and resources)
Applied to files:
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1azure-pipelines.yml
📚 Learning: 2025-10-12T11:23:30.111Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-integration-tests.instructions.md:0-0
Timestamp: 2025-10-12T11:23:30.111Z
Learning: Applies to tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1 : Call commands with -Force where applicable to avoid prompting
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-10-12T11:23:30.111Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-integration-tests.instructions.md:0-0
Timestamp: 2025-10-12T11:23:30.111Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Place command integration tests at tests/Integration/Commands/{CommandName}.Integration.Tests.ps1
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-09-16T16:35:31.909Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines.instructions.md:0-0
Timestamp: 2025-09-16T16:35:31.909Z
Learning: Applies to tests/Integration/Commands/*.Integration.Tests.ps1 : Place integration tests for public commands in tests/Integration/Commands/{CommandName}.Integration.Tests.ps1
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : All public commands and class-based resources must have integration tests
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-10-12T11:23:30.111Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-integration-tests.instructions.md:0-0
Timestamp: 2025-10-12T11:23:30.111Z
Learning: Applies to tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1 : Cover all scenarios and code paths in integration tests
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-09-16T16:35:31.909Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines.instructions.md:0-0
Timestamp: 2025-09-16T16:35:31.909Z
Learning: Applies to tests/Unit/{Classes,Public,Private}/*.Tests.ps1 : Add unit tests for all commands, functions, and resources
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-10-12T11:23:30.111Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-integration-tests.instructions.md:0-0
Timestamp: 2025-10-12T11:23:30.111Z
Learning: Applies to tests/[iI]ntegration/**/*.[iI]ntegration.[tT]ests.ps1 : Use -ErrorAction 'Stop' on commands so failures surface immediately
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-09-12T13:21:31.054Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-pester.instructions.md:0-0
Timestamp: 2025-09-12T13:21:31.054Z
Learning: Applies to **/*.[Tt]ests.ps1 : All public commands, private functions, and classes must have unit tests
Applied to files:
azure-pipelines.yml
📚 Learning: 2025-10-12T11:23:30.111Z
Learnt from: CR
PR: dsccommunity/SqlServerDsc#0
File: .github/instructions/dsc-community-style-guidelines-integration-tests.instructions.md:0-0
Timestamp: 2025-10-12T11:23:30.111Z
Learning: Applies to tests/Integration/Resources/*.Integration.Tests.ps1 : Place resource integration tests at tests/Integration/Resources/{ResourceName}.Integration.Tests.ps1
Applied to files:
azure-pipelines.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: PSScriptAnalyzer
- GitHub Check: PSScriptAnalyzer
- GitHub Check: dsccommunity.SqlServerDsc (Integration Test Commands - SQL Server Commands SQL2022_WIN2025)
- GitHub Check: dsccommunity.SqlServerDsc (Integration Test Commands - SQL Server Commands SQL2022_WIN2022)
- GitHub Check: dsccommunity.SqlServerDsc (Integration Test Commands - SQL Server Commands SQL2019_WIN2025)
- GitHub Check: dsccommunity.SqlServerDsc (Integration Test Commands - SQL Server Commands SQL2019_WIN2022)
- GitHub Check: dsccommunity.SqlServerDsc (Integration Test Commands - SQL Server Commands SQL2017_WIN2022)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Publish Code Coverage)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Unit)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test Determine DSC Resource Test Requirements)
- GitHub Check: dsccommunity.SqlServerDsc (Quality Test and Unit Test HQRM)
- GitHub Check: dsccommunity.SqlServerDsc (Build Package Module)
🔇 Additional comments (2)
tests/Integration/Commands/README.md (1)
80-80: LGTM: entry and dependencies align with the new test.Run order and dependency on Grant-SqlDscServerPermission match the assertions that verify CreateEndpoint is present.
azure-pipelines.yml (1)
323-327: Nice addition keeping the server-permission flow together.Hooking the new ConvertTo integration test in right after ConvertFrom keeps Group 2 coherent and guarantees the pipeline exercises the full permission lifecycle. Looks solid.
…dding -ErrorAction 'Stop' to database connection and disconnection calls
…tests in CHANGELOG.md
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
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
This PR adds comprehensive integration test coverage for the
ConvertTo-SqlDscServerPermissioncommand to ensure its reliability in real SQL Server environments.Context
The
ConvertTo-SqlDscServerPermissioncommand converts a collection ofMicrosoft.SqlServer.Management.Smo.ServerPermissionInfoobjects into an array ofServerPermissionobjects. While unit tests exist, there was no integration test to validate the command's behavior against a real SQL Server instance.Changes
Integration Test Coverage
Created
tests/Integration/Commands/ConvertTo-SqlDscServerPermission.Integration.Tests.ps1with the following test scenarios:ServerPermissionInfocollectionssalogin (system administrator)NT AUTHORITY\SYSTEM(Windows system account)publicserver roleSqlDscIntegrationTestRole_Persistent(test role with granted permissions)ServerPermissionInfoobjects through the pipelinePipeline Configuration
Updated
azure-pipelines.ymlto include the new integration test in Group 2, positioned logically afterConvertFrom-SqlDscServerPermission.Integration.Tests.ps1.Documentation
tests/Integration/Commands/README.mdwith test dependencies and run order informationCHANGELOG.mdwith the new integration test additionTesting
All QA tests pass successfully (788 tests passed, 0 failed), and the test file follows repository coding standards and guidelines.
Related Issue
Fixes #2234
Original prompt
Fixes #2207
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
This change is