PSSAResource.common.v5.Tests: Test ps1 and psm1 files in source
#164
Conversation
WalkthroughUpdates PSSA test discovery to include both Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as TestRunner
participant FS as FileSystem
participant PSSA as PSSALinter
rect rgb(240,248,255)
Note over Runner,FS: Discovery phase (includes .psm1 and .ps1)
Runner->>FS: Get-ChildItem -Recurse -File *.psm1 / *.ps1
FS-->>Runner: Source file list
end
loop For each source file
Runner->>Runner: Build PSCustomObject { File, DescriptiveName }
Runner->>PSSA: Invoke HQRM rules on file
PSSA-->>Runner: Lint results
end
rect rgb(245,245,245)
Note over Runner: Aggregate results and format (Format-Table -AutoSize -Wrap)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
On hold until #161 merged. |
|
@johlju, exclusions are added to build.yml. Tested against SqlServerDsc and WSManDsc locally, which did find some issues. |
|
Why did this previously fail running on the built module *.psm1 and MOF-based resource *.psm1 which I would seem had been better? 🤔 You might have mentioned it before, but can you remind me? 🙂 |
|
Looking at the history, it never did run against built module. Just *.psm1 files in source. EDIT: It must have been another test. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)azure-pipelines.yml(0 hunks)source/DscResource.Test.psm1(1 hunks)source/Tests/QA/ExampleFiles.common.v4.Tests.ps1(1 hunks)
💤 Files with no reviewable changes (1)
- azure-pipelines.yml
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~10-~10: Use correct spacing
Context: ...pec/v2.0.0.html). ## [Unreleased] ### Fixed - Add ability to filter out files in `Exam...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~12-~12: Use correct spacing
Context: ... files in ExampleFiles.common.v4 #120. ### Removed - Remove ModuleBuilder patch. ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~16-~16: Use correct spacing
Context: ...). ### Removed - Remove ModuleBuilder patch. ### Changed - `PSSAResource.common.v5.Tests...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~21-~21: Use correct spacing
Context: ...Tests` - Target all source files #163. ## [0.17.2] - 2025-02-28 ### Fixed - Now ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🔇 Additional comments (2)
CHANGELOG.md (1)
18-21: Changelog entry clearly documents scope expansion.The “Changed” entry for PSSAResource targeting all source files (issue #163) is clear and aligned with the PR.
source/Tests/QA/ExampleFiles.common.v4.Tests.ps1 (1)
31-31: Good: examples are now filtered via repository exclusions.This aligns with the new ability to filter out example files and should reduce false positives.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (1)
121-131: Undefined variable\$dscResourcesPsm1Filewill break test discovery
$invokeScriptAnalyzerParameters.Path = $dscResourcesPsm1File.FullNamerefers to a variable that is no longer created after the switch from module to source files.
Accessing.FullNameon$nullthrows before theContextblock overrides the path, so the whole Describe fails early.- Path = $dscResourcesPsm1File.FullName + # Initial value is unused once overridden below, keep it safe. + Path = $nullOr create an appropriate placeholder variable derived from
$sourceFiles.
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Reviewed 2 of 5 files at r2, 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dan-hughes)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/DscResource.Test.psm1 (1)
2-2: Typo fix applied; good to go.The grammar correction matches the earlier suggestion and improves clarity. No functional impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)source/DscResource.Test.psm1(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
📚 Learning: 2025-08-09T19:29:36.323Z
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
Applied to files:
source/DscResource.Test.psm1CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~25-~25: Use correct spacing
Context: ...Tests` - Target all source files #163. ## [0.17.2] - 2025-02-28 ### Fixed - Now ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ 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). (6)
- GitHub Check: dsccommunity.DscResource.Test (Test Test Windows (PS))
- GitHub Check: dsccommunity.DscResource.Test (Test Test Windows (WPS))
- GitHub Check: dsccommunity.DscResource.Test (Test Integration)
- GitHub Check: dsccommunity.DscResource.Test (Test Test Linux)
- GitHub Check: dsccommunity.DscResource.Test (Test Test macOS)
- GitHub Check: dsccommunity.DscResource.Test (Build Package Module)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
===================================
Coverage 80% 80%
===================================
Files 42 42
Lines 550 550
===================================
Hits 443 443
Misses 107 107 🚀 New features to boost your workflow:
|
|
Waiting for your response on a code rabbit comment before merging. Also, running these tests on the built module would make module's class types etc. available when parsing the script, that will not be the case when scanning the source files as suggested here. Do you think this will be a problem? 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (1)
129-129: Remove stale variable reference for Path$dscResourcesPsm1File isn’t defined; Path is set per-file later anyway. Drop the initial assignment to avoid confusion.
- Path = $dscResourcesPsm1File.FullName + # Path is assigned per file in the Context block below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
📚 Learning: 2025-08-09T19:29:36.323Z
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
Applied to files:
source/Tests/QA/PSSAResource.common.v5.Tests.ps1
🔇 Additional comments (2)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (2)
84-85: Good: faster enumeration with -Filter and two passesSwitching to two Get-ChildItem calls with -Filter and using AddRange is a solid perf win over -Include on large trees. Nice.
124-124: Add “PS Script Analyzer on Source Files” tag for consistencyAfter searching the repo, the only references to
Common Tests - PS Script Analyzer on Resource Fileslive in the two Pester test files (PSSAResource.common.v5.Tests.ps1andPSSAResource.common.v4.Tests.ps1), and no CI workflows or scripts explicitly target that tag. It’s safe to add the new tag here so that the Describe title and tag list align:-Describe 'Common Tests - PS Script Analyzer on Source Files' -Tag @('DscPSSA', 'Common Tests - PS Script Analyzer on Resource Files') { +Describe 'Common Tests - PS Script Analyzer on Source Files' -Tag @('DscPSSA', 'Common Tests - PS Script Analyzer on Source Files', 'Common Tests - PS Script Analyzer on Resource Files') {
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (1)
145-149: Avoid placeholder Path in hashtable; set Path only hereYou overwrite Path here per file. Remove the initial Path assignment from $invokeScriptAnalyzerParameters to reduce confusion and potential nulls.
Example init (outside this block, replacing the earlier hashtable build):
$invokeScriptAnalyzerParameters = @{ IncludeDefaultRules = $true IncludeRule = @($PSSA_rule_config.required_rules + $PSSA_rule_config.flagged_rules + $PSSA_rule_config.ignore_rules) ErrorVariable = 'MyErrors' # Path set per test file below }
♻️ Duplicate comments (3)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (3)
82-82: Prefer strongly-typed file listUse List[System.IO.FileInfo] for type-safety and minor perf gains (avoids boxing), since you always store FileInfo objects.
- $sourceFiles = [System.Collections.Generic.List[System.Object]]::new() + $sourceFiles = [System.Collections.Generic.List[System.IO.FileInfo]]::new()
158-169: Clearer quote stripping without regexThe current regex works but is hard to read and removes only a single leading/trailing quote. Trim whitespace then strip both ' and " using .Trim with a char array; it’s clearer and idempotent.
- # Remove any starting or trailing ' and ". - $newItem = $rule -replace '^["'']|["'']$', '' + # Remove any surrounding whitespace and leading/trailing quote characters (' or "). + $newItem = $rule.Trim().Trim(@([char]39, [char]34))
209-213: Anchor prefix removal and escape dotEnsure only the leading “DscResource.AnalyzerRules\” prefix is removed; escape the dot to avoid unintended matches.
- Format-Table -AutoSize -Wrap -Property @{ + Format-Table -AutoSize -Wrap -Property @{ Name = 'RuleName' - Expression = { $_.RuleName -replace 'DscResource.AnalyzerRules\\' } + Expression = { $_.RuleName -replace '^DscResource\.AnalyzerRules\\' } }, Severity, ScriptName, Line, Message |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
📚 Learning: 2025-08-09T19:29:36.323Z
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
Applied to files:
source/Tests/QA/PSSAResource.common.v5.Tests.ps1
🔇 Additional comments (3)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 (3)
84-85: LGTM: fast enumeration with -Filter + AddRangeGood switch to -Filter and AddRange; matches perf feedback and scales better on large trees.
97-105: LGTM: emit PSCustomObject for Pester -ForEachUsing [PSCustomObject] with File and DescriptiveName simplifies downstream binding and presentation.
154-154: LGTM: custom rule selectionFiltering custom rules via -like 'DscResource.AnalyzerRules*' is correct and keeps output focused.
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Just a tiny comment, and need rebase - resolve the CodeRabbit comments if they are not relevant.
Reviewed 3 of 3 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dan-hughes)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 line 102 at r7 (raw file):
$descriptiveName = Get-RelativePathFromModuleRoot -FilePath $file.FullName -ModuleRootFilePath $resolvedProjectPath [PSCustomObject]@{
Space after the accelarator
Suggestion:
[PSCustomObject] @{
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 8 unresolved discussions (waiting on @johlju)
source/Tests/QA/PSSAResource.common.v5.Tests.ps1 line 102 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Space after the accelarator
Done.
|
@johlju, all done. |
|
@johlju, ready to go. Tested against SqlServerDsc. It found a few things there. |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dan-hughes)
|
This seems to work when running in SqlServerDsc (and fixing the code it fails on). I thought this would parse classes and see types that were not available, but seems it ignores SQL Server types which is good. |
Types in classes seem to be resolved when the module is imported. This is the one difference compared to script based resources. This is what makes the localization tests a little interesting. |
2 participants
Pull Request (PR) description
Run ScriptAnalyzer on all ps1/psm1 files in source.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
This change is