Conversation
…oft.AD module - Created Invoke-Tests.ps1 to run Pester tests with optional code coverage and output formats. - Added LocalizationData.Tests.ps1 to ensure localization keys are consistent across language files. - Documented test structure and usage in README.md, including prerequisites and CI integration. - Updated Todo.md with additional report generation examples and credentials for testing.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
PSScriptAnalyzer found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Pull request overview
This PR resolves merge-conflict fallout by introducing a Pester test suite and CI workflow for AsBuiltReport.Microsoft.AD, plus a couple of module metadata/function-definition adjustments.
Changes:
- Adds a new Pester test harness (
Tests/*) and a GitHub Actions workflow to run it in CI. - Updates the public entrypoint function to include
[CmdletBinding()]and updates the module manifest to declareCompatiblePSEditions. - Removes the
es-ESlocalization file (currently conflicting with the new tests and localization expectations).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
Tests/README.md |
Adds documentation for running tests, but currently contains multiple Azure-module references and incorrect paths/globs. |
Tests/LocalizationData.Tests.ps1 |
Adds localization consistency tests; parser logic needs to be more robust for here-string terminators. |
Tests/Invoke-Tests.ps1 |
Adds a wrapper script to install dependencies and run Pester (optionally with coverage). |
Tests/AsBuiltReport.Microsoft.AD.Tests.ps1 |
Adds a broad module validation test suite; currently contains a wrong module path in one context and expects es-ES. |
Src/Public/Invoke-AsBuiltReport.Microsoft.AD.ps1 |
Adds [CmdletBinding()] to the main public function. |
Language/es-ES/MicrosoftAD.psd1 |
Deletes Spanish localization file (breaks new tests and removes localization support). |
CHANGELOG.md |
Notes an added condition related to Pre-Windows 2000 group membership. |
AsBuiltReport.Microsoft.AD.psd1 |
Declares CompatiblePSEditions (Desktop, Core). |
.github/workflows/Pester.yml |
Adds CI workflow to run Pester and upload artifacts; currently tries to upload coverage without enabling it. |
Comments suppressed due to low confidence (1)
Language/es-ES/MicrosoftAD.psd1:1
- This PR deletes
Language/es-ES/MicrosoftAD.psd1, but the test suite (and likely localization expectations) still reference anes-ESlanguage folder/file. This will break CI/tests and removes non-English support. Either restore thees-ESlocalization file or update the module/tests to no longer require Spanish localization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $ModuleRoot = Join-Path -Path $PSScriptRoot -ChildPath '..\AsBuiltReport.Microsoft.Azure' | ||
| $PublicFunctions = Get-ChildItem -Path "$ModuleRoot\Src\Public" -Filter '*.ps1' -Recurse -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Function Documentation Quality is building $ModuleRoot using ..\AsBuiltReport.Microsoft.Azure, which is the wrong module name/path for this repo. Because Get-ChildItem uses -ErrorAction SilentlyContinue, this will likely yield an empty $PublicFunctions list and make the documentation tests pass without actually validating anything. Point $ModuleRoot at the AD module root (consistent with earlier contexts) and consider asserting the folder exists before enumerating files.
| $ModuleRoot = Join-Path -Path $PSScriptRoot -ChildPath '..\AsBuiltReport.Microsoft.Azure' | |
| $PublicFunctions = Get-ChildItem -Path "$ModuleRoot\Src\Public" -Filter '*.ps1' -Recurse -ErrorAction SilentlyContinue | |
| $ModuleRoot = Join-Path -Path $PSScriptRoot -ChildPath '..\' | |
| $PublicPath = Join-Path -Path $ModuleRoot -ChildPath 'Src\Public' | |
| if (-not (Test-Path -Path $PublicPath -PathType Container)) { | |
| throw "Public functions directory not found: $PublicPath" | |
| } | |
| $PublicFunctions = Get-ChildItem -Path $PublicPath -Filter '*.ps1' -Recurse |
| $Manifest.RequiredModules.Name | Should -Contain 'AsBuiltReport.Core' | ||
| } | ||
|
|
||
| It 'Should require AsBuiltReport.Core version 1.6.1 or higher' { |
There was a problem hiding this comment.
The test name says "Should require AsBuiltReport.Core version 1.6.1 or higher", but the assertion checks for >= 1.6.2. Update the test description to match the actual minimum version being enforced to avoid confusion when this fails.
| It 'Should require AsBuiltReport.Core version 1.6.1 or higher' { | |
| It 'Should require AsBuiltReport.Core version 1.6.2 or higher' { |
|
|
||
| ```powershell | ||
| # Ensure you're running from the correct directory | ||
| # The tests expect to be in the Tests folder with the module folder at ..\AsBuiltReport.Microsoft.Azure\ |
There was a problem hiding this comment.
The troubleshooting note points to ..\AsBuiltReport.Microsoft.Azure\ as the expected module folder, but this repo/module is AsBuiltReport.Microsoft.AD. This path should be corrected so new contributors run tests from the right location.
| # The tests expect to be in the Tests folder with the module folder at ..\AsBuiltReport.Microsoft.Azure\ | |
| # The tests expect to be in the Tests folder with the module folder at ..\AsBuiltReport.Microsoft.AD\ |
| - name: Run Pester Tests | ||
| run: | | ||
| $CodeCoverageParam = @{} | ||
| $OutputFormatParam = @{ OutputFormat = 'NUnitXml' } | ||
|
|
||
| .\Tests\Invoke-Tests.ps1 @CodeCoverageParam @OutputFormatParam | ||
|
|
||
| - name: Upload test results | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: test-results-${{ matrix.os }}-${{ matrix.shell }} | ||
| path: Tests/testResults.xml | ||
| retention-days: 30 | ||
|
|
||
| - name: Upload code coverage to Codecov | ||
| if: matrix.os == 'windows-latest' && matrix.shell == 'pwsh' | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| files: ./Tests/coverage.xml | ||
| flags: unit | ||
| name: codecov-${{ matrix.os }}-${{ matrix.shell }} |
There was a problem hiding this comment.
The workflow always uploads Tests/coverage.xml to Codecov, but Invoke-Tests.ps1 only generates coverage.xml when invoked with -CodeCoverage. Currently CodeCoverageParam is an empty hashtable, so the file likely won't exist. Either pass -CodeCoverage in this workflow run or make the upload step conditional on coverage being enabled/file existence.
|
|
||
| - **PowerShell 5.1** or later (PowerShell 7+ recommended) | ||
| - **Pester 5.0** or later | ||
| - **AsBuiltReport.Core** module (version 1.5.0 or later) |
There was a problem hiding this comment.
Prerequisites list AsBuiltReport.Core as "version 1.5.0 or later", but the module manifest tests and CI workflow in this PR enforce AsBuiltReport.Core >= 1.6.2. Update the README prerequisites to match the actual minimum required version.
| - **AsBuiltReport.Core** module (version 1.5.0 or later) | |
| - **AsBuiltReport.Core** module (version 1.6.2 or later) |
| When adding new features or functions to the module: | ||
|
|
||
| 1. **Add your new Get-AbrAz* function** to `Src/Private/` | ||
| 2. **Add localization strings** to `Language/en-US/MicrosoftAzure.psd1` | ||
| - Create a new section named after your function (e.g., `GetAbrAzLogAnalyticsWorkspace`) | ||
| - Add all required properties (InfoLevel, Collecting, Heading, Name, ResourceGroup, Location, etc.) | ||
| 3. **Run the tests** - The comprehensive localization tests will automatically: | ||
| - Detect your new function | ||
| - Parse all `$LocalizedData.*` references in your code | ||
| - Verify all referenced properties exist in the localization file | ||
| - **No need to manually add tests for each localization string!** |
There was a problem hiding this comment.
The contributing guidance here is Azure-specific (Get-AbrAz*, MicrosoftAzure.psd1, GetAbrAzLogAnalyticsWorkspace) and doesn't align with this AD module. Please update these references to the AD equivalents so contributors add functions/localization in the correct places.
| elseif ($currentSection -and $line -match '^\s+(\w+)\s*=' -and $line -notmatch "^'@") { | ||
| $keys += "$currentSection.$($Matches[1])" | ||
| } | ||
| # End of section | ||
| elseif ($line -match "^'@") { |
There was a problem hiding this comment.
Get-NestedLocalizationKeys treats a here-string terminator as a line that matches ^'@, but in .psd1 files the terminator can be indented (e.g., '@). If it's indented, $currentSection will never reset and keys from later sections will be attributed to the wrong section. Use a whitespace-tolerant terminator match (and similarly update the -notmatch check) to make the parser robust.
| elseif ($currentSection -and $line -match '^\s+(\w+)\s*=' -and $line -notmatch "^'@") { | |
| $keys += "$currentSection.$($Matches[1])" | |
| } | |
| # End of section | |
| elseif ($line -match "^'@") { | |
| elseif ($currentSection -and $line -match '^\s+(\w+)\s*=' -and $line -notmatch "^\s*'@") { | |
| $keys += "$currentSection.$($Matches[1])" | |
| } | |
| # End of section | |
| elseif ($line -match "^\s*'@") { |
|
|
||
| ```powershell | ||
| # Install Pester 5.x | ||
| Install-Module -Name Pester -MinimumVersion 5.0.0 -Force -SkipPublisherCheck |
There was a problem hiding this comment.
The Install-Module command here downloads and installs the Pester module from the public PowerShell Gallery with -MinimumVersion and -SkipPublisherCheck, which means any newer version from an untrusted or compromised publisher can be installed and executed without prompt. An attacker who compromises the Pester package or its publishing account could gain code execution in developer environments where this command is run and potentially exfiltrate credentials or modify code. Consider pinning to a specific trusted version, removing -SkipPublisherCheck, and enforcing trusted publishers/signatures when installing modules used for your tooling and tests.
| - name: Install Pester | ||
| shell: pwsh | ||
| run: Install-Module -Name Pester -MinimumVersion 5.0.0 -Force -SkipPublisherCheck | ||
|
|
||
| - name: Install AsBuiltReport.Core | ||
| shell: pwsh | ||
| run: Install-Module -Name AsBuiltReport.Core -MinimumVersion 1.5.0 -Force |
There was a problem hiding this comment.
In this GitHub Actions example, the Install-Module commands pull Pester (and other modules) from the PowerShell Gallery using only -MinimumVersion and -SkipPublisherCheck, so the workflow will execute whatever newer version is available from potentially untrusted publishers. If the gallery account or module is compromised, an attacker would obtain code execution in your CI environment (with access to GITHUB_TOKEN and any configured secrets) and could tamper with build artifacts or exfiltrate data. To reduce supply chain risk, pin modules to specific trusted versions, avoid -SkipPublisherCheck, and prefer verified publishers and/or additional integrity checks for CI tooling.
| $RequiredModules = @( | ||
| @{ Name = 'Pester'; MinimumVersion = '5.0.0' } | ||
| @{ Name = 'PScribo'; MinimumVersion = '0.11.1' } | ||
| @{ Name = 'PSScriptAnalyzer'; MinimumVersion = '1.0.0' } | ||
| ) | ||
|
|
||
| foreach ($Module in $RequiredModules) { | ||
| $InstalledModule = Get-Module -Name $Module.Name -ListAvailable | | ||
| Where-Object { $_.Version -ge [Version]$Module.MinimumVersion } | | ||
| Sort-Object -Property Version -Descending | | ||
| Select-Object -First 1 | ||
|
|
||
| if (-not $InstalledModule) { | ||
| Write-Host "Installing $($Module.Name) (minimum version $($Module.MinimumVersion))..." -ForegroundColor Yellow | ||
| Install-Module -Name $Module.Name -MinimumVersion $Module.MinimumVersion -Repository PSGallery -Force -AllowClobber -Scope CurrentUser |
There was a problem hiding this comment.
This helper installs and then loads third-party modules (Pester, PScribo, PSScriptAnalyzer) from the PowerShell Gallery using only -MinimumVersion, so each run may pull and execute a newer, unvetted version if one is published. If any of these packages or their publishing accounts are compromised, running this script (especially in CI or on admin workstations) gives an attacker code execution in that environment. Consider pinning to specific known-good versions, restricting to trusted publishers, and treating these installations as part of your supply chain with appropriate integrity controls.
Fix Merge conflicts