Reviewer checklist + Pester v5 unit tests for dependency scripts#166
Open
HeyItsGilbert wants to merge 4 commits intomasterfrom
Open
Reviewer checklist + Pester v5 unit tests for dependency scripts#166HeyItsGilbert wants to merge 4 commits intomasterfrom
HeyItsGilbert wants to merge 4 commits intomasterfrom
Conversation
Captures the conventions every dependency script under PSDepend/PSDependScripts follows (parameter contract, comment-based help shape, Test/Install/Import action semantics, version comparison, AddToPath handling) as a PR review checklist so reviewers can spot drift early. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds one *.Type.Tests.ps1 file per script under PSDepend/PSDependScripts/
(14 total, 50 tests). Each suite covers the contract checks from the
reviewer checklist — Name/DependencyName fallback, Version default,
Test/Install/Test+Install action semantics, scope vs path branching,
Credential pass-through, missing-tool error paths — while mocking the
external surface (Install-Module, Find-Module, Invoke-ExternalCommand,
Get-WebFile, Get-NodeModule, Test-Dotnet, etc.) so the suite runs offline
in under 20s.
Tests/Shared/TestHelpers.psm1 exposes New-PSDependFixture for cheaply
building [PSTypeName('PSDepend.Dependency')] inputs in-memory, avoiding
the .depend.psd1 round-trip when only the script-under-test matters.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
HeyItsGilbert
force-pushed
the
tests/dependency-script-coverage
branch
from
e992fc3 to
e5693a2
Compare
Test Results 3 files ± 0 51 suites +42 1m 52s ⏱️ -5s For more details on these failures, see this check. Results for commit 9f3260f. ± Comparison against base commit 50a5dec. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Adds a documented review checklist for PSDepend dependency scripts and introduces a new Pester v5 test suite (plus shared helpers) that unit-tests each PSDepend/PSDependScripts/*.ps1 dependency type in isolation.
Changes:
- Added
docs/PSDependScripts-ReviewerChecklist.mdto codify conventions/expectations for dependency scripts. - Added
Tests/Shared/TestHelpers.psm1withNew-PSDependFixture(and a credential helper) to construct in-memoryPSDepend.Dependencyobjects for tests. - Added per-type Pester v5 test files under
Tests/*.Type.Tests.ps1to cover common contracts and key branches for each dependency script.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/PSDependScripts-ReviewerChecklist.md | New checklist documenting script conventions and review criteria. |
| Tests/Shared/TestHelpers.psm1 | New helper module to create dependency fixtures and test credentials. |
| Tests/Chocolatey.Type.Tests.ps1 | New unit tests for Chocolatey dependency script (tagged WindowsOnly). |
| Tests/Command.Type.Tests.ps1 | New unit tests for Command dependency script behavior. |
| Tests/DotnetSdk.Type.Tests.ps1 | New unit tests for DotnetSdk dependency script behavior. |
| Tests/FileDownload.Type.Tests.ps1 | New unit tests for FileDownload dependency script behavior. |
| Tests/FileSystem.Type.Tests.ps1 | New unit tests for FileSystem dependency script behavior. |
| Tests/Git.Type.Tests.ps1 | New unit tests for Git dependency script behavior. |
| Tests/GitHub.Type.Tests.ps1 | New unit tests for GitHub dependency script behavior. |
| Tests/Noop.Type.Tests.ps1 | New unit tests for Noop dependency script behavior. |
| Tests/Npm.Type.Tests.ps1 | New unit tests for Npm dependency script behavior. |
| Tests/Nuget.Type.Tests.ps1 | New unit tests for Nuget dependency script behavior. |
| Tests/Package.Type.Tests.ps1 | New unit tests for Package dependency script behavior (with cmdlet stubs for mocking). |
| Tests/PSGalleryModule.Type.Tests.ps1 | New unit tests for PSGalleryModule dependency script behavior. |
| Tests/PSGalleryNuget.Type.Tests.ps1 | New unit tests for PSGalleryNuget dependency script behavior. |
| Tests/Task.Type.Tests.ps1 | New unit tests for Task dependency script behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+55
| It 'Throws by default when the Source errors' { | ||
| $dep = New-PSDependFixture -DependencyName 'CmdFail' -DependencyType 'Command' -Source "throw 'boom'" | ||
| { | ||
| InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep | ||
| } | ||
| } | Should -Throw -ExpectedMessage '*boom*' | ||
| } | ||
|
|
||
| It 'Continues past errors when -FailOnError is specified' { | ||
| $dep = New-PSDependFixture -DependencyName 'CmdSwallow' -DependencyType 'Command' -Source "throw 'boom'" | ||
| $err = $null | ||
| InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep -FailOnError -ErrorAction SilentlyContinue -ErrorVariable err | ||
| $script:capturedErr = $err | ||
| } | ||
| # Did not throw — Write-Error was used | ||
| $true | Should -BeTrue |
Comment on lines
+48
to
+54
| $warnings = $null | ||
| InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep -WarningVariable warn -WarningAction SilentlyContinue | ||
| $script:capturedWarnings = $warn | ||
| } | ||
| # No exception means the script handled the missing file gracefully. | ||
| $true | Should -BeTrue |
Comment on lines
+28
to
+34
| $dep = New-PSDependFixture -DependencyName 'git' -DependencyType 'Chocolatey' | ||
| InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep -WarningAction SilentlyContinue | ||
| } | ||
| # We can't verify the default by inspecting choco args (script bails out | ||
| # when latest lookup returns nothing) but the script should not throw. | ||
| $true | Should -BeTrue |
Comment on lines
+36
to
+47
| It 'PSDependAction Test returns $false when target is missing' { | ||
| $src = Join-Path 'TestDrive:' 'src2.txt' | ||
| $tgtDir = (New-Item 'TestDrive:/missing-tgt' -ItemType Directory -Force).FullName | ||
| Set-Content -Path $src -Value 'content' | ||
| $srcResolved = (Resolve-Path $src).ProviderPath | ||
|
|
||
| $dep = New-PSDependFixture -DependencyName 'fs-test' -DependencyType 'FileSystem' -Source $srcResolved -Target $tgtDir | ||
| $result = InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep -PSDependAction Test | ||
| } | ||
| $result | Should -Be $false | ||
| } |
Comment on lines
+25
to
+31
| It 'Clones the repo via git when the target does not exist' { | ||
| $targetDir = (New-Item 'TestDrive:/git-target' -ItemType Directory -Force).FullName | ||
| $dep = New-PSDependFixture -DependencyName 'https://example.com/user/repo.git' -DependencyType 'Git' -Target $targetDir | ||
|
|
||
| InModuleScope PSDepend -Parameters @{ Dep = $dep; ScriptPath = $script:ScriptPath } { | ||
| & $ScriptPath -Dependency $Dep | ||
| } |
Git: the mocked Invoke-ExternalCommand was a no-op, so the script's unconditional `Set-Location $RepoPath` after `git clone` failed when the repo directory didn't exist. The non-terminating error was tolerated locally but treated as a test failure on CI, and it terminated the script before `Pop-Location` ran — leaking PWD into downstream tests (Npm, PSGalleryModule pipeline, DotnetSdk). Have the mock materialise the expected repo directory under PWD when the 'clone' arg is seen. FileSystem: `(Resolve-Path 'TestDrive:/...').ProviderPath` resolved to a path that GetUnresolvedProviderPathFromPSPath misinterpreted as relative on macOS/Linux. Use `(New-Item -ItemType Directory).FullName` + Join-Path to build absolute paths directly, matching the pattern used elsewhere. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
(New-Item 'TestDrive:/x' -ItemType Directory).FullName returns a path that resolves as relative-to-PWD when later passed to Get-Hash and GetUnresolvedProviderPathFromPSPath on Linux/macOS, breaking the hash-compare branch of FileSystem.ps1. Use the Pester-supplied \$TestDrive filesystem variable (always an absolute path) to anchor source and target paths instead. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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.
Summary
docs/PSDependScripts-ReviewerChecklist.md— codifies the conventions every script underPSDepend/PSDependScripts/follows (parameter contract, comment-based help shape,Test/Install/Importsemantics, version comparison,AddToPathhandling) as a PR review checklist.Tests/Shared/TestHelpers.psm1—New-PSDependFixturebuilds[PSTypeName('PSDepend.Dependency')]inputs in-memory so tests can target one script at a time without going through.depend.psd1parsing.Tests/*.Type.Tests.ps1files (50 tests, ~17s offline) — one per dependency script. Each covers Name/DependencyName fallback, Version default,Test/Install/Test,Installshort-circuit, scope vs path branching, Credential pass-through, and missing-tool error paths, mocking the external surface (Install-Module,Find-Module,Invoke-ExternalCommand,Get-WebFile,Get-NodeModule,Test-Dotnet, …).PowerShellBuild auto-discovers
**/*.Tests.ps1so the new files plug into the existingInvoke-psake Testflow without wiring.Chocolatey.Type.Tests.ps1is taggedWindowsOnlyto be excluded on the non-Windows CI legs (perpsakeFile.ps1:20).Test plan
Invoke-Pester Tests/*.Type.Tests.ps1locally → 50/50 passing in 16.5s on Windows / PS 7.Tests/PSDepend.Tests.ps1orTests/PSModuleGallery.Type.Tests.ps1.Notes for reviewers
New-PSDependFixtureuses[AllowNull()][object]forName/Version/Target/Sourcerather than[string], because[string]coerces unbound values to'', which several scripts (e.g.Nuget.ps1:83) interpret as "value was supplied".InModuleScope PSDepend -Parameters @{ ... }to pass fixture data across the scope boundary — matches existing v5 style inTests/PSModuleGallery.Type.Tests.ps1.Package.Type.Tests.ps1injects simplescript:stubs for the PackageManagement cmdlets (their dynamic parameter sets defeat Pester's mock proxy generator) — pattern from commit `e8c1c54`.🤖 Generated with Claude Code