All Resources: Fix CDXML module import scope for wmiprvse.exe (LCM)#137
Merged
dan-hughes merged 13 commits intodsccommunity:mainfrom Mar 20, 2026
Merged
All Resources: Fix CDXML module import scope for wmiprvse.exe (LCM)#137dan-hughes merged 13 commits intodsccommunity:mainfrom
dan-hughes merged 13 commits intodsccommunity:mainfrom
Conversation
The RemoteDesktop CDXML module proxy commands were not visible inside wmiprvse.exe because Assert-Module imports into its own function scope. - Add Import-RemoteDesktopModule helper to the common module with: - RDMS service start (required for WMI namespace registration) - Import-Module -Global -Force (makes CDXML proxies visible globally) - Replace all 21 Assert-Module calls across 9 DSC resource files - Remove redundant inline RDMS service start block from DSC_RDSessionDeployment Get-TargetResource - Update all unit tests to mock/assert the new function - Add unit tests for Import-RemoteDesktopModule helper - Update CHANGELOG.md
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a shared helper function Changes
Sequence DiagramsequenceDiagram
participant DSCResource as DSC Resource<br/>(Get/Set-TargetResource)
participant Helper as Import-RemoteDesktopModule
participant Service as Get-Service<br/>(RDMS)
participant StartSvc as Start-Service<br/>(RDMS)
participant ModuleCmd as Get-Module<br/>(Check Loaded)
participant Import as Import-Module<br/>(-Global -Force)
DSCResource->>Helper: Call Import-RemoteDesktopModule
Helper->>Service: Query RDMS service status
alt Service not running
Helper->>StartSvc: Start RDMS service
StartSvc-->>Helper: Service started / Ready
else Service running or absent
Note over Helper: No startup needed
end
Helper->>ModuleCmd: Check RemoteDesktop module loaded?
alt Module not in session
ModuleCmd-->>Helper: Not found
Helper->>Import: Import RemoteDesktop -Global -Force
Import-->>Helper: Module imported (commands available globally)
else Module already loaded
Note over Helper: Skip import
end
Helper-->>DSCResource: Return (CDXML commands now available)
DSCResource->>DSCResource: Call Get-RDServer / other cmdlets<br/>(now visible in wmiprvse.exe scope)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
===================================
- Coverage 97% 97% -1%
===================================
Files 9 9
Lines 539 536 -3
===================================
- Hits 526 523 -3
Misses 13 13
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/DSC_RDSessionDeployment.Tests.ps1 (1)
141-145:⚠️ Potential issue | 🟡 Minor
Set-TargetResourcecoverage is missing the helper assertion.
Set-TargetResourcenow callsImport-RemoteDesktopModulebefore reading current state, but none of theSetexamples assert it. BecauseGet-TargetResourceis mocked in these specs, removing that direct helper call would not fail the suite. Seesource/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1:50-110.Minimal assertion to add to each `Set` example
$null = Set-TargetResource `@testParams` } + Should -Invoke -CommandName Import-RemoteDesktopModule -Exactly -Times 1 -Scope It Should -Invoke -CommandName Get-TargetResource -Exactly -Times 1 -Scope It🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/DSC_RDSessionDeployment.Tests.ps1` around lines 141 - 145, The Set-TargetResource tests are missing an assertion that the helper Import-RemoteDesktopModule is invoked; because Get-TargetResource is mocked the suite won't catch its omission. In each Set example in DSC_RDSessionDeployment.Tests.ps1, after invoking Set-TargetResource add an assertion such as Assert-MockCalled -CommandName Import-RemoteDesktopModule -Times 1 (or -Exactly 1) to verify Import-RemoteDesktopModule is called before reading state; reference the Set-TargetResource invocation and the mocked Get-TargetResource to locate where to add the Assert-MockCalled.
🧹 Nitpick comments (2)
source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1 (1)
1-19: Expand function metadata/help to match repository PowerShell requirements.Please add missing comment-based help sections (
PARAMETER,EXAMPLE,INPUTS,OUTPUTS) and add an explicit[OutputType()]attribute for this function.🧹 Minimal metadata update example
function Import-RemoteDesktopModule { [CmdletBinding()] + [OutputType()] param ()As per coding guidelines "Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections" and "INPUTS/OUTPUTS must be included", and "Include
[OutputType({return type})]for functions with output, no output use[OutputType()]."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1` around lines 1 - 19, The Import-RemoteDesktopModule function lacks complete comment-based help and an OutputType attribute; update the function by adding PARAMETER, EXAMPLE, INPUTS and OUTPUTS comment-based help blocks under the existing SYNOPSIS/DESCRIPTION (ensure DESCRIPTION is 40+ chars) and document the (empty) param list and any outputs (or None) in those sections, and add an explicit [OutputType()] attribute above the function declaration (use [OutputType()] if the function produces no output or [OutputType(<Type>)] if it returns an object); reference the function name Import-RemoteDesktopModule and the existing CmdletBinding() block when inserting these additions.tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1 (1)
136-137: Assert or remove this helper mock.
Import-RemoteDesktopModuleis mocked for theTest-TargetResourcedescribe, but none of the examples verify it. IfTest-TargetResourceis expected to call the helper, add aShould -Invoke; otherwise this is dead setup and can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1` around lines 136 - 137, The Mock for Import-RemoteDesktopModule in the BeforeAll block is unused for the Test-TargetResource describe; either remove that Mock to avoid dead setup or assert its use by adding a Should -Invoke (or equivalent invocation assertion) in the Test-TargetResource examples that exercise Test-TargetResource so the test verifies Import-RemoteDesktopModule was called; update the BeforeAll accordingly and ensure Test-TargetResource examples reference the mocked helper if you choose to keep the mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-20: Update the two Unreleased bullets that mention fixes to
include the issue link in the required changelog format: replace or append the
plain reference to issue 136 in the bullet that starts "All DSC resources now
import the RemoteDesktop CDXML module..." and the bullet that starts "The RDMS
service is now started before importing the RemoteDesktop module..." with the
canonical format [issue `#136`](https://github.com/<owner>/<repo>/issues/136);
also apply the same change to the similar entries around lines 24-29 so every
mention of this fix references [issue
`#136`](https://github.com/<owner>/<repo>/issues/136).
In
`@source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1`:
- Around line 22-26: The comment-based help for the Import-RemoteDesktopModule
function is incomplete and the service-start uses a hardcoded sleep; add full
help sections (PARAMETER/INPUTS/OUTPUTS/EXAMPLE) above the function opening
brace in the Import-RemoteDesktopModule comment block specifying INPUTS: None,
OUTPUTS: None, and an EXAMPLE showing calling Import-RemoteDesktopModule with
its intent, and add the [OutputType()] attribute immediately after
[CmdletBinding()] to satisfy PowerShell guidelines; also replace the Start-Sleep
-Seconds 2 after Start-Service -Name RDMS with logic that waits for the RDMS
service to reach Running, e.g., using the service object's
WaitForStatus('Running', [TimeSpan]::FromSeconds(30)) or equivalent on the
$rdmsService object to avoid a fixed delay while still handling Start-Service
and StartPending cases.
In `@tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1`:
- Around line 62-63: The tests mock Import-RemoteDesktopModule but never assert
it was invoked, so add assertions after each InModuleScope block to verify the
helper was called by Get-TargetResource; specifically, in the contexts around
the existing Mock -CommandName Import-RemoteDesktopModule (and the similar block
at lines ~126-127) add a Should -Invoke/Assert-MockCalled style check for
Import-RemoteDesktopModule (or equivalent Pester mock assertion) to ensure
Get-TargetResource actually calls that helper during the test.
In
`@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1`:
- Line 51: Add a new Pester contract test in
Import-RemoteDesktopModule.Tests.ps1 that validates the exported parameter-set
surface of the public command Import-RemoteDesktopModule: use Get-Command -Name
Import-RemoteDesktopModule to retrieve the command metadata, inspect
.ParameterSets (or .Parameters) and assert the expected parameter-set names and
required parameters exist and no unexpected sets are present; place this
alongside the existing Describe block so the test fails if the function
signature or parameter sets drift.
- Around line 100-105: The test's invocation assertions for
Import-RemoteDesktopModule only check that Import-Module is called with -Global;
update the Should -Invoke -CommandName Import-Module ParameterFilter blocks in
the Import-RemoteDesktopModule.Tests (the assertion around
Import-RemoteDesktopModule and the similar assertion at the 156-161 region) to
also assert $Force -eq $true (i.e., ensure each ParameterFilter checks $Name -eq
'RemoteDesktop' -and $Global -eq $true -and $Force -eq $true) so the contract
Import-Module -Global -Force is fully validated.
- Around line 30-35: The test currently resolves the parent module via
Get-Module (Get-Module -Name $script:dscModuleName -ListAvailable) which can
pick a different installed version; change the resolution to use the
repository/build output instead by deriving the parent module path from
$PSScriptRoot (or an explicit environment variable like
$env:BUILD_ARTIFACTS_DIR) and set $script:parentModule.ModuleBase to that
folder; then recompute $script:subModulesFolder and $script:subModulePath from
that explicit parent path and use Import-Module -Name $script:subModulePath
-Force -ErrorAction 'Stop' so the test imports the exact module built for the
tests rather than whatever is on PSModulePath.
---
Outside diff comments:
In `@tests/Unit/DSC_RDSessionDeployment.Tests.ps1`:
- Around line 141-145: The Set-TargetResource tests are missing an assertion
that the helper Import-RemoteDesktopModule is invoked; because
Get-TargetResource is mocked the suite won't catch its omission. In each Set
example in DSC_RDSessionDeployment.Tests.ps1, after invoking Set-TargetResource
add an assertion such as Assert-MockCalled -CommandName
Import-RemoteDesktopModule -Times 1 (or -Exactly 1) to verify
Import-RemoteDesktopModule is called before reading state; reference the
Set-TargetResource invocation and the mocked Get-TargetResource to locate where
to add the Assert-MockCalled.
---
Nitpick comments:
In
`@source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1`:
- Around line 1-19: The Import-RemoteDesktopModule function lacks complete
comment-based help and an OutputType attribute; update the function by adding
PARAMETER, EXAMPLE, INPUTS and OUTPUTS comment-based help blocks under the
existing SYNOPSIS/DESCRIPTION (ensure DESCRIPTION is 40+ chars) and document the
(empty) param list and any outputs (or None) in those sections, and add an
explicit [OutputType()] attribute above the function declaration (use
[OutputType()] if the function produces no output or [OutputType(<Type>)] if it
returns an object); reference the function name Import-RemoteDesktopModule and
the existing CmdletBinding() block when inserting these additions.
In `@tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1`:
- Around line 136-137: The Mock for Import-RemoteDesktopModule in the BeforeAll
block is unused for the Test-TargetResource describe; either remove that Mock to
avoid dead setup or assert its use by adding a Should -Invoke (or equivalent
invocation assertion) in the Test-TargetResource examples that exercise
Test-TargetResource so the test verifies Import-RemoteDesktopModule was called;
update the BeforeAll accordingly and ensure Test-TargetResource examples
reference the mocked helper if you choose to keep the mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2664050d-6873-438a-a28e-0117b0875182
📒 Files selected for processing (21)
CHANGELOG.mdsource/DSCResources/DSC_RDCertificateConfiguration/DSC_RDCertificateConfiguration.psm1source/DSCResources/DSC_RDConnectionBrokerHAMode/DSC_RDConnectionBrokerHAMode.psm1source/DSCResources/DSC_RDGatewayConfiguration/DSC_RDGatewayConfiguration.psm1source/DSCResources/DSC_RDLicenseConfiguration/DSC_RDLicenseConfiguration.psm1source/DSCResources/DSC_RDRemoteApp/DSC_RDRemoteApp.psm1source/DSCResources/DSC_RDServer/DSC_RDServer.psm1source/DSCResources/DSC_RDSessionCollection/DSC_RDSessionCollection.psm1source/DSCResources/DSC_RDSessionCollectionConfiguration/DSC_RDSessionCollectionConfiguration.psm1source/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1tests/Unit/DSC_RDCertificateConfiguration.Tests.ps1tests/Unit/DSC_RDConnectionBrokerHAMode.Tests.ps1tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1tests/Unit/DSC_RDLicenseConfiguration.Tests.ps1tests/Unit/DSC_RDRemoteApp.Tests.ps1tests/Unit/DSC_RDServer.Tests.ps1tests/Unit/DSC_RDSessionCollection.Tests.ps1tests/Unit/DSC_RDSessionCollectionConfiguration.Tests.ps1tests/Unit/DSC_RDSessionDeployment.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1
Show resolved
Hide resolved
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
Show resolved
Hide resolved
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
Show resolved
Hide resolved
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
Show resolved
Hide resolved
dan-hughes
reviewed
Mar 18, 2026
Contributor
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
Just a few things that AI picked up.
@dan-hughes reviewed 18 files and all commit messages, and made 1 comment.
Reviewable status: 18 of 21 files reviewed, 6 unresolved discussions (waiting on raandree).
…emoteDesktopModule function
…in Get-TargetResource tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1`:
- Around line 34-36: The current selection of $script:parentModuleBase can pick
an older build because it uses Select-Object -First 1 without deterministic
ordering; update the logic that resolves $script:parentModuleBase (using
$script:builtModuleRoot and $script:dscModuleName) to enumerate directories,
sort them deterministically (e.g., by LastWriteTime descending to pick the
newest), pick the first result, and if no directory is found throw a clear
terminating error that includes the searched path and module name so the test
fails fast with actionable information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c749980-3933-4d11-880f-fa855c6c649e
📒 Files selected for processing (5)
CHANGELOG.mdsource/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/DSC_RDGatewayConfiguration.Tests.ps1
- source/Modules/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.ps1
- CHANGELOG.md
.../RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1 (1)
34-36: Make built-module folder selection deterministic.
Select-Object -First 1on directory enumeration can be nondeterministic if multiple folders exist underoutput/builtModule/RemoteDesktopServicesDsc. Sort explicitly (and optionally fail fast if nothing is found) to avoid test flakiness.Suggested patch
- $script:parentModuleBase = Get-ChildItem -Path (Join-Path -Path $script:builtModuleRoot -ChildPath $script:dscModuleName) -Directory | - Select-Object -First 1 | - Select-Object -ExpandProperty FullName + $script:parentModuleBase = Get-ChildItem -Path (Join-Path -Path $script:builtModuleRoot -ChildPath $script:dscModuleName) -Directory | + Sort-Object -Property Name -Descending | + Select-Object -First 1 -ExpandProperty FullName + + if ($null -eq $script:parentModuleBase) + { + throw 'No built RemoteDesktopServicesDsc module directory was found under output/builtModule.' + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1` around lines 34 - 36, The directory selection using Select-Object -First 1 for $script:parentModuleBase is non-deterministic when multiple folders exist; modify the pipeline that builds $script:parentModuleBase (which uses $script:builtModuleRoot and $script:dscModuleName) to explicitly sort the Get-ChildItem results (e.g., Sort-Object Name or LastWriteTime) before selecting the first entry and add a fail-fast check that throws or asserts if no folder is found so tests cannot silently proceed with an empty/null $script:parentModuleBase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1`:
- Around line 1-2: The suppress-message attribute at the top
([System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments',
'')]) is missing the required Justification; update that attribute to include
the Justification named parameter with the project-standard explanation text
(e.g. Justification = 'Required setup block per project template') so the setup
block exactly matches the required template before the Describe block; ensure
you only modify the attribute line (leave param () intact) and use the same
attribute and argument order as in the template.
---
Nitpick comments:
In
`@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1`:
- Around line 34-36: The directory selection using Select-Object -First 1 for
$script:parentModuleBase is non-deterministic when multiple folders exist;
modify the pipeline that builds $script:parentModuleBase (which uses
$script:builtModuleRoot and $script:dscModuleName) to explicitly sort the
Get-ChildItem results (e.g., Sort-Object Name or LastWriteTime) before selecting
the first entry and add a fail-fast check that throws or asserts if no folder is
found so tests cannot silently proceed with an empty/null
$script:parentModuleBase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9074b4a2-e3ba-4615-b06a-02014fe390a8
📒 Files selected for processing (1)
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
Outdated
Show resolved
Hide resolved
Contributor
|
@raandree, let me know when it's ready again. |
Contributor
Author
|
It is ready to be reviewed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/DSC_RDSessionDeployment.Tests.ps1 (1)
138-278: Consider addingImport-RemoteDesktopModuleinvocation assertion inSet-TargetResourcetests.The mock for
Import-RemoteDesktopModuleis correctly added at line 141, but unlike theGet-TargetResourcetests, there's noShould -Invoke -CommandName Import-RemoteDesktopModuleassertion to verify it's called. While this may be intentional sinceSet-TargetResourcealso callsGet-TargetResourceinternally (which would trigger the import), explicitly asserting the import call in at least oneSet-TargetResourcetest would strengthen coverage.Example assertion to add in one of the Set-TargetResource test cases
Should -Invoke -CommandName Get-TargetResource -Exactly -Times 1 -Scope It Should -Invoke -CommandName New-RDSessionDeployment -Exactly -Times 1 -Scope It Should -Invoke -CommandName Add-RDServer -Exactly -Times 0 -Scope It + Should -Invoke -CommandName Import-RemoteDesktopModule -Exactly -Times 1 -Scope It🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/DSC_RDSessionDeployment.Tests.ps1` around lines 138 - 278, Add an assertion to verify Import-RemoteDesktopModule is invoked when Set-TargetResource runs: in one existing test (e.g., the "When the deployment does not exist" It block or any one of the ForEach It blocks) add a Should -Invoke -CommandName Import-RemoteDesktopModule -Exactly -Times 1 -Scope It after calling Set-TargetResource so the mocked Import-RemoteDesktopModule is explicitly asserted; this ensures the test for Set-TargetResource (the function under test) checks that Import-RemoteDesktopModule is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Unit/DSC_RDSessionDeployment.Tests.ps1`:
- Around line 138-278: Add an assertion to verify Import-RemoteDesktopModule is
invoked when Set-TargetResource runs: in one existing test (e.g., the "When the
deployment does not exist" It block or any one of the ForEach It blocks) add a
Should -Invoke -CommandName Import-RemoteDesktopModule -Exactly -Times 1 -Scope
It after calling Set-TargetResource so the mocked Import-RemoteDesktopModule is
explicitly asserted; this ensures the test for Set-TargetResource (the function
under test) checks that Import-RemoteDesktopModule is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6db461b2-885a-437c-bca1-7c5dccc3520c
📒 Files selected for processing (5)
CHANGELOG.mdsource/DSCResources/DSC_RDSessionDeployment/DSC_RDSessionDeployment.psm1tests/Unit/DSC_RDSessionDeployment.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1
- CHANGELOG.md
dan-hughes
requested changes
Mar 20, 2026
Contributor
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
@dan-hughes reviewed 7 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on raandree).
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1 line 32 at r5 (raw file):
# Derive the repository root from $PSScriptRoot so the test always loads the # built module from the build output rather than a version on PSModulePath. $script:repoRoot = Join-Path -Path $PSScriptRoot -ChildPath '../../../../' -Resolve
This seems unnecessary. AI might have been hallucinating.
This can be reverted to the standard pattern.
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Test-RemoteDesktopServicesDscOsRequirement.Tests.ps1 line 32 at r5 (raw file):
# Derive the repository root from $PSScriptRoot so the test always loads the # built module from the build output rather than a version on PSModulePath. $script:repoRoot = Join-Path -Path $PSScriptRoot -ChildPath '../../../../' -Resolve
Again this doesn't seem to be necessary.
Contributor
Author
|
Done, @dan-hughes. I did the fix and the PR on the side, and sure, the AI might have made a few small mistakes here and there - which I would have done as well, but overall it was incredibly helpful. The AI handled all the troubleshooting, and the fix as well. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1 (1)
1-1: Consider using the exact template justification text.The justification currently reads "Required setup block per project template", but the guidelines specify: "Suppressing this rule because Script Analyzer does not understand Pester syntax." While functionally equivalent, matching the exact template text ensures consistency across the codebase.
📝 Suggested template alignment
-[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Required setup block per project template')] +[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Suppressing this rule because Script Analyzer does not understand Pester syntax.')] param ()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1` at line 1, Replace the current SuppressMessageAttribute justification text "Required setup block per project template" with the exact project template wording "Suppressing this rule because Script Analyzer does not understand Pester syntax." so the attribute on the top-level declaration (the [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute(...)] line) uses the canonical justification string for consistency across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1`:
- Line 1: Replace the current SuppressMessageAttribute justification text
"Required setup block per project template" with the exact project template
wording "Suppressing this rule because Script Analyzer does not understand
Pester syntax." so the attribute on the top-level declaration (the
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute(...)] line) uses the
canonical justification string for consistency across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c92e7d7-a72a-496d-842b-d517fd8cc9c6
📒 Files selected for processing (1)
tests/Unit/RemoteDesktopServicesDsc.Common/Public/Import-RemoteDesktopModule.Tests.ps1
dan-hughes
approved these changes
Mar 20, 2026
Contributor
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
@dan-hughes reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on raandree).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
RemoteDesktopServicesDsc 4.0.0 is completely broken when applied by the DSC LCM — every resource fails with
The term 'Get-RDServer' is not recognized. The commands work fine in an interactive session; the failure only occurs inside the DSC WMI provider host process (wmiprvse.exe).Root cause: All 9
DSC_RD*.psm1resource files callAssert-Module -ModuleName 'RemoteDesktop' -ImportModule.Assert-Module(fromDscResource.Common) runsImport-Module -Name RemoteDesktopinside its own function scope. TheRemoteDesktopmodule is a CDXML module — its cmdlets are WMI/CIM proxy functions generated at import time. Insidewmiprvse.exe, these proxy functions do not propagate fromAssert-Module's function scope back to the calling scope.There is a secondary issue: the RDMS service uses
Automatic (Delayed Start)and may not yet be running when the LCM kicks in after a reboot. The RemoteDesktop CDXML module requires the RDMS WMI namespace to be registered, so the import fails if the service hasn't started.Fix:
Import-RemoteDesktopModuletoRemoteDesktopServicesDsc.Commonthat:RemoteDesktopmodule with-Global -Forceso CDXML proxy commands are visible across all scopesAssert-Module -ModuleName 'RemoteDesktop' -ImportModulecalls across all 9 DSC resource files with the new helperDSC_RDSessionDeploymentGet-TargetResourceImport-RemoteDesktopModulehelper covering all RDMS service statesThis Pull Request (PR) fixes the following issues
RDSessionDeployment: Get-RDServer not recognized when DSC LCM applies configuration#136Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
help.
This change is