[WIP] Add multi-language testing and security framework#12
[WIP] Add multi-language testing and security framework#12AprilDeFeu merged 3 commits intofeat/testing-frameworkfrom
Conversation
|
@copilot Try again |
…test assertions Co-authored-by: AprilDeFeu <36605389+AprilDeFeu@users.noreply.github.com>
Fixed the test failures. The issue was that Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot You had a warning for the access to the powershellgallery website, i added it to your firewall |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes multiple CI test failures in the PowerShell system-maintenance script tests. The changes address path resolution issues, correct PowerShell cmdlet parameter usage, and fix variable scoping problems that were preventing tests from running successfully.
Key changes:
- Enhanced path resolution logic with fallback mechanisms for different test environments
- Fixed
Get-HelpandGet-Commandcmdlet parameter usage to work correctly with script files - Added variable scoping workaround for scriptblock execution in Pester tests
- Implemented fallback to temp directory when MyDocuments folder is unavailable
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/PowerShell/system-maintenance.Tests.ps1 | Updated test file with robust path resolution, corrected cmdlet parameters, and fixed variable scoping for scriptblock execution |
| PowerShell/system-administration/maintenance/system-maintenance.ps1 | Added fallback logic in Get-LogFilePath function to use temp directory when MyDocuments is unavailable |
| if (-not $testDir) { | ||
| $testDir = Get-Location | ||
| } | ||
| $script:scriptPath = Join-Path $testDir "../../../PowerShell/system-administration/maintenance/system-maintenance.ps1" | Resolve-Path | Select-Object -ExpandProperty Path |
There was a problem hiding this comment.
The Resolve-Path cmdlet will throw a terminating error if the path doesn't exist, causing the BeforeAll block to fail. Consider adding -ErrorAction Stop and wrapping this in a try-catch block, or use -ErrorAction SilentlyContinue with validation:
$script:scriptPath = Join-Path $testDir "../../../PowerShell/system-administration/maintenance/system-maintenance.ps1" | Resolve-Path -ErrorAction Stop | Select-Object -ExpandProperty PathAlternatively, test the path exists before resolving:
$relativePath = Join-Path $testDir "../../../PowerShell/system-administration/maintenance/system-maintenance.ps1"
if (-not (Test-Path $relativePath)) {
throw "Script not found at: $relativePath"
}
$script:scriptPath = Resolve-Path $relativePath | Select-Object -ExpandProperty Path| $script:scriptPath = Join-Path $testDir "../../../PowerShell/system-administration/maintenance/system-maintenance.ps1" | Resolve-Path | Select-Object -ExpandProperty Path | |
| $relativePath = Join-Path $testDir "../../../PowerShell/system-administration/maintenance/system-maintenance.ps1" | |
| if (-not (Test-Path $relativePath)) { | |
| throw "Script not found at: $relativePath" | |
| } | |
| $script:scriptPath = Resolve-Path $relativePath | Select-Object -ExpandProperty Path |
|
|
||
| It "should support -WhatIf" { | ||
| $command = Get-Command -Path $scriptPath | ||
| $command = Get-Command -Name $scriptPath |
There was a problem hiding this comment.
Get-Command -Name expects a command name (like 'Get-Process'), not a file path. When working with script file paths, you should use the -LiteralPath parameter instead:
$command = Get-Command -LiteralPath $scriptPathThe -Name parameter will fail to find the command or may behave unpredictably when given a full file path.
| $command = Get-Command -Name $scriptPath | |
| $command = Get-Command -LiteralPath $scriptPath |
| $userDocs = [Environment]::GetFolderPath('MyDocuments') | ||
| # Fallback to temp directory if MyDocuments is not available | ||
| if ([string]::IsNullOrWhiteSpace($userDocs)) { | ||
| $userDocs = [System.IO.Path]::GetTempPath() |
There was a problem hiding this comment.
[nitpick] [System.IO.Path]::GetTempPath() returns a path with a trailing backslash on Windows (e.g., C:\Users\User\AppData\Local\Temp\). While Join-Path handles this correctly, it's more robust to trim trailing path separators for consistency:
$userDocs = [System.IO.Path]::GetTempPath().TrimEnd([System.IO.Path]::DirectorySeparatorChar, [System.IO.Path]::AltDirectorySeparatorChar)This ensures consistent path handling across different scenarios and prevents potential double-separator issues.
| $userDocs = [System.IO.Path]::GetTempPath() | |
| $userDocs = [System.IO.Path]::GetTempPath().TrimEnd([System.IO.Path]::DirectorySeparatorChar, [System.IO.Path]::AltDirectorySeparatorChar) |
|
@copilot apply changes based on the comments in this thread |
… separators Co-authored-by: AprilDeFeu <36605389+AprilDeFeu@users.noreply.github.com>
Applied the code review suggestions with modifications:
All tests still pass (4/4). (commit 66f7b4b) |
Get-LogFilePathto handle empty MyDocuments folder (fallback to temp directory)Get-Helpassertion to check for correct script name (system-maintenance.ps1notsystem-maintenance)Get-Commandto use-Nameparameter instead of-Path💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.