From a6229ad7497ab1e759d69d92d3f68cd8206951b5 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Sat, 29 Aug 2026 19:04:10 -0700 Subject: [PATCH] test: cover the branches a mutant could survive The suite has good volume and reads well, but a mutation pass over lib/ found twenty-five changes to the driver that left all 344 examples green. Most of them are places where a builder is tested thoroughly in isolation and never checked for being wired in, or where a guard exists and nothing ever takes the branch it guards. The largest hole was create_instance_object. Deleting any of the eight assignments in it - disks, guest accelerators, metadata, network interfaces, scheduling, service accounts, tags, labels - left the suite passing, because every builder is asserted on its own and the object they are attached to never was. spec/support/driver_context.rb already defined created_instance_payload, which runs a real create and hands back what insert_instance actually received; nothing called it. It does now. The rest, each of which used to survive: - create ran generate_server_name before create_disks_config without complaint, which is the reordering the disk-name budget exists to prevent, and reintroduces the illegal 67-character disk name - create no longer calling update_windows_password at all - destroy deleting disks before the instance still holding them - the instance-name budget off by one in either direction - the standalone disk attachment sending no autoDelete, which orphans a billing disk after a successful destroy - the modulus and exponent check on the serial port response, which the specs never reached: the port is scanned newest line first and the matching response was always the last line, so the noise before it was never parsed and "ignores a response generated for a different key" was tautological - parse_event's Hash guard, without which a bare JSON scalar on the port raises TypeError mid-create - a configured Windows username reaching the agent, rather than the built-in Administrator that Google's images ship disabled - the serial port number - nil service_account_scopes, which is what "service_account_scopes:" written bare in kitchen.yml parses to - nil disk_size, likewise - disk_size_for_image at the boundary, where the request already matches the image exactly - image_disk_size_gb's memoization, its nil-image guard, and its ClientError rescue, which had no line coverage at all - the auto-migrate warning for guest accelerators No lib/ changes: every one of these was already correct, and is now asserted. 344 examples to 373. Signed-off-by: Tim Smith --- spec/kitchen/driver/gce/disk_build_spec.rb | 74 ++++++++++++++++++ .../driver/gce/instance_payload_spec.rb | 74 +++++++++++++++++- spec/kitchen/driver/gce/lifecycle_spec.rb | 77 +++++++++++++++++++ spec/kitchen/driver/gce/validation_spec.rb | 14 ++++ .../driver/gce/windows_password_spec.rb | 64 ++++++++++++++- 5 files changed, 298 insertions(+), 5 deletions(-) diff --git a/spec/kitchen/driver/gce/disk_build_spec.rb b/spec/kitchen/driver/gce/disk_build_spec.rb index 12868cd..cfc447f 100644 --- a/spec/kitchen/driver/gce/disk_build_spec.rb +++ b/spec/kitchen/driver/gce/disk_build_spec.rb @@ -93,6 +93,23 @@ def built_disks .to eq("projects/test-project/zones/test-zone-1a/disks/tk-test-1-data") end + # A standalone disk is not removed with the instance unless the + # attachment says so, so an unsent autoDelete leaves it billing after + # `kitchen destroy` has reported success. + it "auto-deletes the standalone disk with the instance" do + expect(built_disks.last.auto_delete).to be(true) + end + + context "with autodelete_disk disabled on it" do + let(:driver_config) do + { disks: { boot: { boot: true }, data: { disk_size: 50, autodelete_disk: false } } } + end + + it "leaves the standalone disk behind" do + expect(built_disks.last.auto_delete).to be(false) + end + end + it "waits for the standalone disk to become READY" do expect(compute).to receive(:get_disk).at_least(:once).and_return(ComputeApi.disk(status: "READY")) @@ -238,6 +255,63 @@ def built_disks expect(built_disks.first.initialize_params.disk_size_gb).to eq(100) end end + + # `disk_size:` written bare in kitchen.yml parses to nil rather than to + # the default, so the comparison has to cope with having no size at all. + context "and the size was left empty in kitchen.yml" do + let(:driver_config) { { disks: { disk1: { boot: true, disk_size: nil } } } } + + it "sizes the boot disk to the image rather than raising" do + expect(built_disks.first.initialize_params.disk_size_gb).to eq(50) + end + end + + # The boundary: a request that exactly matches the image is already + # legal, so it must be sent unchanged and without a warning. + context "and exactly the image's size was requested" do + let(:driver_config) { { disks: { disk1: { boot: true, disk_size: 50 } } } } + + it "honours the requested size" do + expect(built_disks.first.initialize_params.disk_size_gb).to eq(50) + end + + it "does not claim the request was too small" do + built_disks + + expect(log).not_to include("smaller than image") + end + end + end + + describe "reading the image's size" do + # The boot image is looked up more than once during a single create -- + # once to validate it, once to size the disk from it. + it "asks GCE for an image's size only once" do + expect(compute).to receive(:get_image).at_most(:once).and_return(ComputeApi.image) + + driver.image_disk_size_gb("test-image") + driver.image_disk_size_gb("test-image") + end + + it "has no size to report for no image" do + expect(driver.image_disk_size_gb(nil)).to be_nil + end + + # A size that cannot be read is not a reason to fail the run: the + # requested size is sent as-is and GCE decides. + context "when the image lookup fails" do + before { allow(compute).to receive(:get_image).and_raise(ComputeApi.client_error) } + + it "reports no size rather than raising" do + expect(driver.image_disk_size_gb("test-image")).to be_nil + end + + it "says why in the debug log" do + driver.image_disk_size_gb("test-image") + + expect(log).to include("Unable to read the size of image test-image") + end + end end context "when an extra disk's custom image is larger than its requested size" do diff --git a/spec/kitchen/driver/gce/instance_payload_spec.rb b/spec/kitchen/driver/gce/instance_payload_spec.rb index 270807e..717460e 100644 --- a/spec/kitchen/driver/gce/instance_payload_spec.rb +++ b/spec/kitchen/driver/gce/instance_payload_spec.rb @@ -40,9 +40,71 @@ end end - it "builds an instance the Google client recognises" do + end + + # Every builder below is exercised in isolation further down this file. This + # block proves each one is actually wired into the object that reaches + # `insert_instance`, which is the one failure an isolated example cannot see: + # a dropped assignment ships an instance with no tags, no labels and no + # scheduling, and every per-builder example still passes. + describe "the payload handed to insert_instance" do + subject(:payload) { created_instance_payload } + + let(:driver_config) do + { + tags: %w{web db}, + labels: { "team" => "platform" }, + metadata: { "owner" => "platform" }, + guest_accelerators: [{ type: "nvidia-tesla-t4", count: 2 }], + service_account_scopes: ["storage-ro"], + } + end + + it "is an instance the Google client recognises" do expect(payload).to be_a(Google::Apis::ComputeV1::Instance) end + + it "attaches the disks" do + expect(payload.disks).not_to be_empty + expect(payload.disks).to all(be_a(Google::Apis::ComputeV1::AttachedDisk)) + end + + it "attaches the guest accelerators" do + expect(payload.guest_accelerators.map(&:accelerator_count)).to eq([2]) + end + + it "attaches the metadata" do + expect(payload.metadata.items.map(&:key)).to include("owner", "created-by") + end + + it "attaches the network interface" do + expect(payload.network_interfaces.size).to eq(1) + end + + it "attaches the scheduling options" do + expect(payload.scheduling).to be_a(Google::Apis::ComputeV1::Scheduling) + end + + it "attaches the service account and its scopes" do + expect(payload.service_accounts.first.scopes) + .to eq(["https://www.googleapis.com/auth/devstorage.read_only"]) + end + + it "attaches the network tags" do + expect(payload.tags.items).to eq(%w{web db}) + end + + it "attaches the labels" do + expect(payload.labels).to eq("team" => "platform") + end + + context "with no service account scopes" do + let(:driver_config) { { service_account_scopes: [] } } + + it "attaches no service account at all" do + expect(payload.service_accounts).to be_nil + end + end end describe "#instance_scheduling" do @@ -385,6 +447,16 @@ expect(driver.instance_service_accounts).to be_nil end end + + # `service_account_scopes:` written bare in kitchen.yml parses to nil + # rather than to an empty array, and nil has no #empty?. + context "with the scope list present but empty in kitchen.yml" do + let(:driver_config) { { service_account_scopes: nil } } + + it "is nil rather than raising" do + expect(driver.instance_service_accounts).to be_nil + end + end end describe "#service_account_scope_url" do diff --git a/spec/kitchen/driver/gce/lifecycle_spec.rb b/spec/kitchen/driver/gce/lifecycle_spec.rb index 82cf94c..437715e 100644 --- a/spec/kitchen/driver/gce/lifecycle_spec.rb +++ b/spec/kitchen/driver/gce/lifecycle_spec.rb @@ -56,6 +56,45 @@ driver.create(server_name: "already-created") end + # The instance name is budgeted against the longest configured disk name, + # so the disk configuration has to be settled before the name is generated. + # Only a create can catch a reordering: `generate_server_name` called on + # its own is always handed a config that has already been normalised. + # The default disk configuration is the case that matters: `config[:disks]` + # is nil until `create_disks_config` fills it in, so a name generated first + # is budgeted against no disk name at all. + context "with names that only fit if the disks are configured first" do + let(:kitchen_instance_name) { "long-suite-name-to-overflow-disk-naming-ubuntu-2204" } + + it "sends no disk name longer than GCE allows" do + disk_names = created_instance_payload.disks.map { |d| d.initialize_params.disk_name }.compact + + expect(disk_names).not_to be_empty + expect(disk_names.map(&:length)) + .to all(be <= Kitchen::Driver::Gce::MAX_INSTANCE_NAME_LENGTH) + end + end + + # update_windows_password is covered on its own in windows_spec.rb; what + # this adds is that a create actually calls it, without which a Windows + # suite comes up and then fails WinRM auth with no password in state. + context "with a WinRM transport" do + let(:transport_name) { "winrm" } + let(:transport_username) { "kitchen" } + let(:driver_config) { { email: "user@example.com" } } + + it "resets the Windows password and records it in the state file" do + allow_successful_create + allow(Kitchen::Driver::Gce::WindowsPassword).to receive(:new).and_return( + instance_double(Kitchen::Driver::Gce::WindowsPassword, new_password: "s3cret") + ) + + driver.create(state) + + expect(state[:password]).to eq("s3cret") + end + end + context "when use_private_ip is set" do let(:driver_config) { { use_private_ip: true } } @@ -219,6 +258,23 @@ driver.destroy({}) end + # An instance holds its disks until it is gone, so deleting them first + # fails and leaves them behind, billing. + it "deletes the instance before the disks it is holding" do + order = [] + allow(compute).to receive(:get_instance).and_return(ComputeApi.instance) + allow(compute).to receive(:get_disk).and_return(ComputeApi.disk) + allow(compute).to receive(:get_zone_operation).and_return(ComputeApi.operation) + allow(compute).to receive(:delete_instance) { order << :instance; ComputeApi.operation } + allow(compute).to receive(:delete_disk) { order << :disk; ComputeApi.operation } + + driver.destroy( + server_name: "tk-test-1", zone: "test-zone-1a", created_disks: ["tk-test-1-data"] + ) + + expect(order).to eq(%i{instance disk}) + end + it "deletes the instance and clears the state file" do state = { server_name: "tk-test-1", hostname: "203.0.113.4", zone: "test-zone-1a" } allow(compute).to receive(:get_instance).and_return(ComputeApi.instance) @@ -430,6 +486,27 @@ def generated_name end end + # The budget is inclusive: a name exactly as long as it allows is legal, + # and only one character more has to give way to the fallback. Off by one + # in either direction is a 64-character disk name or a Test Kitchen name + # discarded for no reason. + context "with a name exactly as long as the budget allows" do + let(:kitchen_instance_name) { "a" * 47 } + + it "keeps the Test Kitchen name" do + expect(generated_name).to match(/\Atk-a{47}-[0-9a-f]{6}\z/) + end + end + + context "with a name one character longer than the budget allows" do + let(:kitchen_instance_name) { "a" * 48 } + + it "gives way to the fallback so the disk name still fits" do + expect("#{generated_name}-disk1".length) + .to be <= Kitchen::Driver::Gce::MAX_INSTANCE_NAME_LENGTH + end + end + context "with a disk name that leaves room for no instance name at all" do let(:driver_config) { { disks: { "#{"d" * 60}": { boot: true } } } } diff --git a/spec/kitchen/driver/gce/validation_spec.rb b/spec/kitchen/driver/gce/validation_spec.rb index d30fc2e..78c3763 100644 --- a/spec/kitchen/driver/gce/validation_spec.rb +++ b/spec/kitchen/driver/gce/validation_spec.rb @@ -203,6 +203,20 @@ end end + # GCE will not live-migrate an instance with a GPU attached, so the + # driver turns auto-migrate off whatever the user asked for. + context "with guest accelerators and auto-migrate left on" do + let(:driver_config) do + { guest_accelerators: [{ type: "nvidia-tesla-t4", count: 1 }], auto_migrate: true } + end + + it "warns that auto-migrate is disabled" do + driver.validate! + + expect(log).to include("Auto-migrate disabled for instance with guest accelerators") + end + end + context "with a WinRM transport logging in as the built-in Administrator" do let(:transport_name) { "winrm" } let(:transport_username) { "Administrator" } diff --git a/spec/kitchen/driver/gce/windows_password_spec.rb b/spec/kitchen/driver/gce/windows_password_spec.rb index 3583db2..3f40811 100644 --- a/spec/kitchen/driver/gce/windows_password_spec.rb +++ b/spec/kitchen/driver/gce/windows_password_spec.rb @@ -82,29 +82,70 @@ def serial_port_returns(*lines) expect(winpass.new_password).to eq("pässwörd-✓") end + # The port is scanned newest line first, so the noise that has to be + # survived is the noise *after* the response: those are the lines actually + # parsed on the way back to it. Noise before it is never even read. it "ignores serial port noise surrounding the response" do serial_port_returns( "SeaBIOS (version 1.8.9-google)", + agent_response(password: "found-me").to_json, "not json at all {{{", - "12345", '{"unrelated":"event"}', - agent_response(password: "found-me").to_json, "Booting from Hard Disk..." ) expect(winpass.new_password).to eq("found-me") end + # A bare scalar parses as valid JSON but is not a Hash, so indexing it for + # a modulus raises TypeError rather than simply failing to match. + it "skips a line that is valid JSON but not an object" do + serial_port_returns( + agent_response(password: "found-me").to_json, + "12345", + '"a bare string"', + "null" + ) + + expect(winpass.new_password).to eq("found-me") + end + + # Newest last, so the stale response is the one examined first. Were the + # key check dropped, this would decrypt to the stale entry's password. it "ignores a response generated for a different key" do other_key = OpenSSL::PKey::RSA.new(2048) - stale = agent_response.merge( + stale = agent_response(password: "not-mine").merge( "modulus" => Base64.strict_encode64(other_key.public_key.n.to_s(2)) ) - serial_port_returns(stale.to_json, agent_response(password: "mine").to_json) + serial_port_returns(agent_response(password: "mine").to_json, stale.to_json) expect(winpass.new_password).to eq("mine") end + # Same again for the exponent half of the pair, which is the easier of the + # two to drop by accident because every RSA key here shares one. + it "ignores a response whose exponent does not match" do + stale = agent_response(password: "not-mine").merge( + "exponent" => Base64.strict_encode64(OpenSSL::BN.new(3).to_s(2)) + ) + serial_port_returns(agent_response(password: "mine").to_json, stale.to_json) + + expect(winpass.new_password).to eq("mine") + end + + it "reads the serial port the Windows agent writes to" do + expect(compute).to receive(:get_instance_serial_port_output) + .with("test-project", "test-zone-1a", "tk-win-1", port: described_class::SERIAL_PORT) + .and_return( + instance_double( + Google::Apis::ComputeV1::SerialPortOutput, + contents: agent_response(password: "on-port-4").to_json + ) + ) + + expect(winpass.new_password).to eq("on-port-4") + end + it "raises when the agent reports it could not reset the password" do serial_port_returns(agent_response(found: false).to_json) @@ -189,6 +230,21 @@ def serial_port_returns(*lines) expect(request["userName"]).to eq("Administrator") end + # Google's images ship the built-in Administrator disabled and the agent + # will not enable it, so a configured username reaching the agent + # unchanged is what makes a Windows suite work at all. + context "with an account other than the built-in Administrator" do + subject(:winpass) do + described_class.new( + driver, instance_name: "tk-win-1", email: "user@example.com", username: "kitchen" + ) + end + + it "asks the agent to reset that account instead" do + expect(request["userName"]).to eq("kitchen") + end + end + it "carries the requesting user's email" do expect(request["email"]).to eq("user@example.com") end