Skip to content

Revive the FileTransporter unit tests - #24

Open
tas50 wants to merge 1 commit into
chef:mainfrom
tas50:test/revive-file-transporter-specs
Open

tas50 wants to merge 1 commit into
chef:mainfrom
tas50:test/revive-file-transporter-specs

Conversation

@tas50

@tas50 tas50 commented Aug 28, 2026

Copy link
Copy Markdown

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 against TmpZip, 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 revert was never an option:

2016 today
MD5 digests Digest::SHA1
Base64 temp files + decode_files streaming into a FileStream + extract_files
o[:exitcode] = 10 WinRM::Output#exitcode= and #<<

What is covered

  • the rendered check_files and extract_files PowerShell hash tables, including ps_hash formatting
  • streamed file contents, asserted by decoding every Base64 payload and comparing to the source bytes
  • directory uploads via a temporary zip, the extract_files round trip, and cleanup of the temp zip
  • StringIO uploads and the single-source restriction (UploadSourceError)
  • destination reconciliation when the remote target turns out to be a folder
  • Base64 transfer-size accounting and the progress callback
  • FileTransporterFailed on a non-zero exit code, and on stderr with a zero exit code
  • Errno::ENOENT for a missing local path

Two 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 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, 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:

Mutation Failures
Digest::SHA1Digest::MD5 10
ps_hash separator ";\n"",\n" 2
drop zip_io.unlink in cleanup 1
disable target_is_folder reconciliation 1
invert the chk_dirty check 7
size / 3 * 4size 2
always skip extract_files 2
drop the stderr check in parse_response 1
remove the digest from the tmpzip name 1

bundle 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 logger dependency that is currently only on the ci/test-all-supported-rubies branch, since lib/chef-winrm-fs.rb requires logger and it stopped being a default gem in Ruby 4.0. This does not affect CI on main, whose matrix is Ruby 3.1 and 3.4.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant