-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] Add multi-language testing and security framework #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@copilot Try again |
…test assertions Co-authored-by: AprilDeFeu <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 <[email protected]>
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.