Refactor Libvirt Firmware Handling for Better Testability

- Moved firmware logic into dedicated methods.
- Updated tests for improved coverage and workflow alignment.

app/models/compute_resources/foreman/model/libvirt.rb

@@ -147,26 +147,8 @@ def new_vm(attr = { })
       opts[:boot_order] = %w[hd]
       opts[:boot_order].unshift 'network' unless attr[:image_id]
 
-      # This value comes from PxeLoaderSupport#firmware_type
-      firmware_type = opts.delete(:firmware_type).to_s
-
-      # automatic firmware type is determined by the PXE Loader
-      # if no PXE Loader is set, we will set it to bios by default
-      if opts[:firmware] == 'automatic'
-        opts[:firmware] = (firmware_type == 'none' || firmware_type.empty?) ? 'bios' : firmware_type
-      end
-
-      # Adjust firmware and secure_boot values for Libvirt compatibility
-      if opts[:firmware] == 'uefi_secure_boot'
-        opts[:firmware_features] = { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' }
-        opts[:loader] = { 'secure' => 'yes' }
-      end
-      # Libvirt expects the firmware type to be 'efi' instead of 'uefi'
-      if opts[:firmware]&.start_with?('uefi')
-        opts[:firmware] = 'efi'
-      else
-        opts[:firmware] = 'bios'
-      end
+      handle_automatic_firmware(opts)
+      configure_firmware_settings(opts)
 
       vm = client.servers.new opts
       vm.memory = opts[:memory] if opts[:memory]
@@ -348,5 +330,26 @@ def validate_volume_capacity(vol)
         raise Foreman::Exception.new(N_("Please specify volume size. You may optionally use suffix 'G' to specify volume size in gigabytes."))
       end
     end
+
+    def handle_automatic_firmware(opts)
+      # This value comes from PxeLoaderSupport#firmware_type
+      firmware_type = opts.delete(:firmware_type).to_s
+
+      # Handle automatic firmware setting based on PXE Loader
+      # if no PXE Loader is set, we will set it to bios by default
+      if opts[:firmware] == 'automatic'
+        opts[:firmware] = firmware_type.presence || 'bios'
+      end
+    end
+
+    def configure_firmware_settings(opts)
+      # Adjust firmware and secure boot settings for Libvirt compatibility
+      if opts[:firmware] == 'uefi_secure_boot'
+        opts[:firmware_features] = { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' }
+        opts[:loader] = { 'secure' => 'yes' }
+      end
+
+      opts[:firmware] = opts[:firmware]&.start_with?('uefi') ? 'efi' : 'bios'
+    end
   end
 end

test/models/compute_resources/libvirt_test.rb

@@ -124,6 +124,57 @@ class Foreman::Model::LibvirtTest < ActiveSupport::TestCase
     end
   end
 
+  describe '#handle_automatic_firmware' do
+    setup do
+      @cr = FactoryBot.build_stubbed(:libvirt_cr)
+    end
+
+    test 'sets firmware based on automatic and firmware_type scenarios' do
+      scenarios = [
+        [{ firmware: 'automatic' }, { firmware: 'bios' }],
+        [{ firmware: 'automatic', firmware_type: :bios }, { firmware: 'bios' }],
+        [{ firmware: 'automatic', firmware_type: :uefi }, { firmware: 'uefi' }],
+        [{ firmware: 'automatic', firmware_type: :uefi_secure_boot }, { firmware: 'uefi_secure_boot' }],
+      ]
+
+      scenarios.each do |input, expected|
+        @cr.send(:handle_automatic_firmware, input)
+        assert_equal(expected, input)
+      end
+    end
+
+    test 'does not change firmware if it is not automatic' do
+      attrs_in = { firmware_type: :uefi_secure_boot, firmware: 'uefi' }
+      @cr.send(:handle_automatic_firmware, attrs_in)
+      assert_equal({ firmware: 'uefi' }, attrs_in)
+    end
+  end
+
+  describe '#configure_firmware_settings' do
+    setup do
+      @cr = FactoryBot.build_stubbed(:libvirt_cr)
+    end
+
+    test 'sets firmware to BIOS when no firmware is provided' do
+      attrs_in = { firmware: '' }
+      @cr.send(:configure_firmware_settings, attrs_in)
+      assert_equal({ firmware: 'bios' }, attrs_in)
+    end
+
+    test 'sets firmware to EFI when firmware_type is uefi' do
+      attrs_in = { firmware: 'uefi' }
+      @cr.send(:configure_firmware_settings, attrs_in)
+      assert_equal({ firmware: 'efi' }, attrs_in)
+    end
+
+    test 'sets EFI firmware with SecureBoot enabled and secure loader when firmware type is uefi_secure_boot' do
+      attrs_in = { firmware: 'uefi_secure_boot' }
+      attrs_out = { firmware: 'efi', firmware_features: { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' }, loader: { 'secure' => 'yes' } }
+      @cr.send(:configure_firmware_settings, attrs_in)
+      assert_equal attrs_out, attrs_in
+    end
+  end
+
   describe '#new_volume' do
     let(:cr) { FactoryBot.build_stubbed(:libvirt_cr) }