Conversation
FileTransporter is the core of this gem, 573 lines that build every PowerShell script the upload path sends and parse every CSV report that comes back. It has had no unit tests since commit 4b9a646 (June 2016) deleted the 828-line spec that covered it, leaving the whole suite at three examples against TmpZip. The deleted spec could not simply be reverted. Both it and the code moved on since 2016: - digests went from MD5 to SHA1 - the upload path went from writing Base64 temp files and running decode_files to streaming into a FileStream plus extract_files - WinRM::Output dropped its Hash-like `o[:exitcode] =` writer in favour of an attr writer and `<<` So this is a rewrite against the current pipeline rather than a revert, covering: the rendered check_files and extract_files hash tables, streamed file contents, directory uploads via a temporary zip and its cleanup, StringIO uploads and their single-source restriction, destination reconciliation when the target is a folder, transfer size accounting, progress callbacks, and the FileTransporterFailed paths for a non-zero exit code and for stderr on a zero exit. Two things make these harder to rot than their predecessors: - the shell is an instance_double of WinRM::Shells::Powershell, so a rename in chef-winrm fails the suite instead of passing against a stale stub - the fake host answers check_files using whichever digests the transporter actually asked for, which keeps directory tests honest without pinning them to a zip digest that changes every run Streaming is asserted by decoding every Base64 payload and comparing the result to the source bytes, rather than by pinning an expected chunk count, so the tests do not break when PSRP fragment sizing changes. Verified by mutating the implementation: swapping SHA1 for MD5, breaking the ps_hash separator, dropping the temporary zip cleanup, disabling folder reconciliation, inverting the dirty check, breaking the transfer size math, always skipping extract_files, dropping the stderr check, and removing the digest from the tmpzip name each fail the suite. Cookstyle is clean. Unit examples go from 3 to 29. Signed-off-by: Tim Smith <tsmith84@proton.me>
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.
Restores unit coverage for
FileTransporter— 573 lines that build every PowerShell script the upload path sends and parse every CSV report that comes back, and which has had no unit tests since June 2016.Commit
4b9a646("delete file_transporter unit tests") removed an 828-line spec and nothing replaced it. That left the entire unit suite at three examples againstTmpZip, finishing in 15ms, while the code that actually talks to Windows went untested.Unit examples go from 3 to 29.
Why this is a rewrite, not a revert
Both the spec and the code moved on since 2016, so
git revertwas never an option:Digest::SHA1decode_filesFileStream+extract_fileso[:exitcode] = 10WinRM::Output#exitcode=and#<<What is covered
check_filesandextract_filesPowerShell hash tables, includingps_hashformattingextract_filesround trip, and cleanup of the temp zipStringIOuploads and the single-source restriction (UploadSourceError)FileTransporterFailedon a non-zero exit code, and on stderr with a zero exit codeErrno::ENOENTfor a missing local pathTwo choices that should keep these from rotting again
The shell is an
instance_double(WinRM::Shells::Powershell), not a plain double. A plain double is what let the old specs drift — it happily accepts calls to methods the real class no longer has. With a verifying double, a rename inchef-winrmfails the suite instead of passing against a stale stub.The fake host answers
check_filesusing whichever digests the transporter actually asked for, read back out of the rendered script. Zip digests change on every run, so this keeps the directory tests honest without pinning them to a value that cannot be predicted.Streaming is likewise asserted on decoded bytes rather than an expected chunk count, so the tests survive changes to PSRP fragment sizing.
Verification
Mutation-tested against the implementation — each of these fails the suite:
Digest::SHA1→Digest::MD5ps_hashseparator";\n"→",\n"zip_io.unlinkincleanuptarget_is_folderreconciliationchk_dirtychecksize / 3 * 4→sizeextract_filesparse_responsebundle exec rake spec→ 29 examples, 0 failures.cookstyle --chefstyle -c .rubocop.yml→ 17 files, no offenses.Note
No production code is touched — this PR is spec-only.
Local verification on Ruby 4.0 needs the
loggerdependency that is currently only on theci/test-all-supported-rubiesbranch, sincelib/chef-winrm-fs.rbrequiresloggerand it stopped being a default gem in Ruby 4.0. This does not affect CI onmain, whose matrix is Ruby 3.1 and 3.4.