Rewrite the test suite and fix seven driver bugs - #162
Merged
Conversation
Replaces the Minitest suite (one assertion, and unrunnable because the `test` bundle group pulled in Berkshelf, Vagrant and Dokken) with an RSpec suite of 103 examples, and ports the PowerShell tests to Pester 5. The specs fake only the Train connection, so every command travels the real run_ps -> wrap_command -> encode_command path and the fake decodes the -encodedcommand payload back to readable PowerShell. Assertions are on the script that would have reached the host, not on a mock. Bugs found while writing the tests, each with a regression test: - powershell.rb called Base64.strict_encode64 without requiring base64; it worked only because Train happens to require it first. base64 is a bundled gem from Ruby 3.4 on, so it is now required and declared. - update_state raised `undefined method '[]' for nil` when Get-VmDetail returned nothing. It now reports what actually went wrong. - Additional disks always used the local kitchen root, so against a remote host they were created where the Hyper-V server cannot see them. They now sit under remote_vm_path beside the differencing disk. - destroy returned early when the VM was already gone without clearing state[:id], leaving a stale id in the state file permanently. - The support script was uploaded even on the local backend, which reads the in-gem copy and never looks at the uploaded one. - Memory and VLAN bounds were checked with between? on the raw config, so a value quoted in kitchen.yml raised "comparison of String with 1 failed" instead of the intended message. Both are coerced first. - local_script_path kept its ".." segments, and differencing_disk_exists returned nil rather than false. The Pester port is a semantic change, not a syntax refresh. Pester 5 splits discovery from execution, so calls made in a Describe body run before any Mock is active; `Should -Invoke` defaults to `-Scope It` and counts only invocations from the same It block; and `-Times N` means "at least N", so `-Times 0` can never fail without `-Exactly`. Ported literally, the old tests would have asserted nothing. TestHelper.ps1 supplies mockable shims because Pester builds CommandMetadata for a mocked command, which cannot resolve the Hyper-V parameter types the stub module declares. Also: YARD documentation on the public API (100% documented, no warnings), `rake yard`, `rake yard:stats`, `rake yard:server` and `rake pester` tasks, none of them prerequisites of `rake default` so docs and PowerShell cannot fail a build; the heavy plugins moved to an `integration` bundle group; and specs are no longer excluded from Cookstyle. Co-Authored-By: Claude Opus 5 (1M context) <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.
What this does
Replaces the unit test suite from the ground up and fixes the bugs that writing it uncovered.
The previous suite was one Minitest file with a single assertion (
api_version == 2), and it could not run at all — thetestbundle group pulled in Berkshelf, Vagrant, Dokken and Inspec, sobundle installfailed to resolve before a single assertion ran.integrationbundle group so unit work installs fastHow the specs are built
The driver's job is turning configuration into PowerShell and running it in the right order. So the specs fake only the outer boundary — the Train connection — and let everything above it run for real. Each command travels the genuine
run_ps→wrap_command→encode_commandpath, and the fake decodes the-encodedcommandpayload back to readable PowerShell:Stubbing
run_psinstead would assert that the driver called a mock, not that it generated the right PowerShell.Bugs fixed
powershell.rbusedBase64.strict_encode64without requiringbase64base64is a bundled gem from Ruby 3.4 on, so this was one dependency change away from breaking everyrun_pscall. Now required and declared in the gemspec.update_statedid@vm["Id"]on nilGet-VmDetailreturning nothing surfaced asundefined method '[]' for nilinstead of anything actionable.remote_vm_pathdestroyleft a stalestate[:id]base_script_pathand never looks at the uploaded one.between?on raw config valueskitchen.ymlraisedcomparison of String with 1 failedrather than the intended validation message. Both memory and VLAN bounds now coerce first.local_script_pathkept its..segments;differencing_disk_existsreturnednilThe Pester port is semantic, not cosmetic
Pester 5 changed three things that each silently produce tests which pass no matter what the code does. Ported literally, the old tests would have asserted nothing:
New-KitchenVMin theDescribe/Contextbody, which runs during discovery — before anyMockis active.Should -Invokedefaults to-Scope Itand counts only invocations from the sameItblock. Calls made in aBeforeAllare not counted.-Times Nmeans "at least N" since Pester 4, so-Times 0can never fail. Roughly half the original assertions are "should not have been called" — all now use-Exactly.spec/powershell/TestHelper.ps1supplies mockable cmdlet shims: Pester buildsCommandMetadatafor a mocked command, which forces resolution of every parameter type, and the stub module's signatures name types like[Microsoft.HyperV.PowerShell.VirtualMachine]that only exist on a real Hyper-V host.Verification
Both suites were run locally, and mutation-tested to confirm they can actually fail:
$VlanId -ne $nullguardiso_pathguardstate[:id]fixBoth suites run on any platform — neither needs Hyper-V, Windows, or an elevated shell.
Rake tasks
rake yard,rake yard:stats,rake yard:server, andrake pesterare added. None are prerequisites ofrake default, so documentation coverage and PowerShell availability cannot fail a build.rake testandrake unitremain as aliases forrake spec, so the shared CI workflow is unaffected.Notes for review
spec/support/tospec/powershell/.spec/support/is now RSpec's auto-loaded Ruby helper directory, so the two could not share it..rubocop.ymlno longer excludesspec/**/*— the specs lint clean.CONTRIBUTING.mdhad a stale claim thatbundle exec rake changelogexists. It never did, andgithub_changelog_generatoris not in the Gemfile; release-please handles the changelog.support/ci/windows_ci.batdid need updating — it raninvoke-pester ./specdirectly and installed Pester unversioned.feat:so release-please cuts a minor bump — the seven fixes change runtime behavior and should not ship silently.🤖 Generated with Claude Code