Changes based on code review.

library/network/src/lib/network/firewall_chooser.rb

@@ -46,11 +46,19 @@ class FirewallChooser
       fwd: "firewalld"
     }.freeze
 
+    # @!attribute [rw] selected
+    #   @return [Symbol] backend selected
+    attr_accessor :selected
+
+    def initialize(selected = nil)
+      @selected = selected
+    end
+
     # Check if backend is installed on the system.
     #
     # @param backend_sym [Symbol] Firewall backend
     # @return [Boolean] True if backend is installed.
-    def self.backend_available?(backend_sym)
+    def backend_available?(backend_sym)
       # SF2 has it's own method of checking if it's installed. This is
       # used internally by SF2. Here, we simply care if the package
       # is present on the system.
@@ -60,14 +68,14 @@ def self.backend_available?(backend_sym)
     # Obtain backends which are installed on the system
     #
     # @return [Array<Symbol>] List of installed backends.
-    def self.installed_backends
+    def installed_backends
       FIREWALL_BACKENDS.select { |b, _p| backend_available?(b) }.keys
     end
 
     # Obtain list of enabled backends.
     #
     # @return [Array<Symbol>] List of enabled backends.
-    def self.enabled_backends
+    def enabled_backends
       installed_backends.select { |b| Service.Enabled(FIREWALL_BACKENDS[b]) }
     end
 
@@ -77,24 +85,19 @@ def self.enabled_backends
     #   since FirewallD and SF2 systemd service files conflict with each other.
     #
     # @return [Array<Symbol>] List of running backends.
-    def self.running_backends
+    def running_backends
       installed_backends.select { |b| Service.Active(FIREWALL_BACKENDS[b]) }
     end
 
-    # Return an instance of the firewall given or detected
+    # Return an instance of the firewall selected or find the preferred one
     #
+    # @see find_preferred
     # @return [SuSEFirewall2Class, SuSEFirewalldClass]
-    def self.choose(backend_sym = nil)
-      backend = backend_sym || detect
-      # For the old testsuite, always generate SF2 instance. FirewallD tests
-      # will be committed later on but they will only affect the new
-      # testsuite
-
+    def choose
       # If backend is specificed, go ahead and create an instance. Otherwise, try
       # to detect which backend is enabled and create the appropriate instance.
-      backend = detect unless backend
-
-      backend == :sf2 ? SuSEFirewall2Class.new : SuSEFirewalldClass.new
+      @selected ||= find_preferred
+      selected == :sf2 ? SuSEFirewall2Class.new : SuSEFirewalldClass.new
     end
 
     # Determine which firewall should be selected as the backend depending on
@@ -104,8 +107,10 @@ def self.choose(backend_sym = nil)
     # @raise [SuSEFirewallMultipleBackends] if firewalld and SuSEfirewalld2 are
     #   running
     # @return [Symbol] the backend that should be used
-    def self.detect
-      # Old testsuite
+    def find_preferred
+      # For the old testsuite, always generate SF2 instance. FirewallD tests
+      # will be committed later on but they will only affect the new
+      # testsuite
       return :sf2 if Mode.testsuite
 
       # Only one running backend is permitted.
@@ -116,11 +121,11 @@ def self.detect
       if running_backends.empty? && enabled_backends.size > 1
         Builtins.y2warning("Both SuSEfirewall2 and firewalld services are enabled. " \
                             "Defaulting to SuSEfirewall2")
-        enabled_backends[0] = :sf2
+        return :sf2
       end
 
       # Set a good default. The running one takes precedence over the enabled one.
-      selected_backend = running_backends[0] ? running_backends[0] : enabled_backends[0]
+      selected_backend = (running_backends.empty? ? enabled_backends : running_backends).first
       # Fallback to the first installed backend or to SuSEfirewall2 if not
       selected_backend = (installed_backends.first || :sf2) if selected_backend.to_s.empty?
 

library/network/test/firewall_chooser_test.rb

@@ -4,33 +4,33 @@
 require "network/firewall_chooser"
 
 describe Yast::FirewallChooser do
-  describe ".installed_backeds" do
-
-  end
-
-  describe ".choose" do
-    context "when :sf2 is specified" do
+  describe "#choose" do
+    context "when :sf2 is pre-selected" do
+      subject { described_class.new(:sf2) }
       it "returns a SuSEFirewall2Class object" do
-        expect(described_class.choose(:sf2)).to be_a(Yast::SuSEFirewall2Class)
+        expect(subject.choose).to be_a(Yast::SuSEFirewall2Class)
       end
     end
 
-    context "when :fwd is specified" do
+    context "when :fwd is pre-selected" do
+      subject { described_class.new(:fwd) }
       it "returns a SuSEFirewalldClass object" do
-        expect(described_class.choose(:fwd)).to be_a(Yast::SuSEFirewalldClass)
+        expect(subject.choose).to be_a(Yast::SuSEFirewalldClass)
       end
     end
 
-    context "when no backend is specified" do
-      it "returns and object of the detected one" do
-        expect(described_class).to receive(:detect).and_return(:sf2, :fwd)
-        expect(described_class.choose).to be_a(Yast::SuSEFirewall2Class)
-        expect(described_class.choose).to be_a(Yast::SuSEFirewalldClass)
+    context "when no backend has been pre-selected" do
+      it "returns and object of the find_preferreded one" do
+        expect(subject).to receive(:find_preferred).and_return(:sf2, :fwd)
+        subject.selected = nil
+        expect(subject.choose).to be_a(Yast::SuSEFirewall2Class)
+        subject.selected = nil
+        expect(subject.choose).to be_a(Yast::SuSEFirewalldClass)
       end
     end
   end
 
-  describe ".detect" do
+  describe "#find_preferred" do
     let(:fwd_installed) { false }
     let(:fwd_enabled) { false }
     let(:fwd_running) { false }
@@ -50,21 +50,21 @@
 
     context "when neither firewalld nor SuSEfirewall2 are running" do
       it "returns :sf2 as the fallback" do
-        expect(Yast::FirewallChooser.detect).to eql(:sf2)
+        expect(subject.find_preferred).to eql(:sf2)
       end
     end
 
     context "when only firewalld is installed" do
       let(:fwd_installed) { true }
       it "returns :fwd" do
-        expect(Yast::FirewallChooser.detect).to eql(:fwd)
+        expect(subject.find_preferred).to eql(:fwd)
       end
     end
 
     context "when only SuSEFirewall2 is installed" do
       let(:sf2_installed) { true }
       it "returns :sf2" do
-        expect(Yast::FirewallChooser.detect).to eql(:sf2)
+        expect(subject.find_preferred).to eql(:sf2)
       end
     end
 
@@ -75,7 +75,7 @@
       context "and neither firewalld nor SuSEfirewall2 are running" do
         context "and no one is enabled" do
           it "returns :sf2 as the fallback" do
-            expect(Yast::FirewallChooser.detect).to eql(:sf2)
+            expect(subject.find_preferred).to eql(:sf2)
           end
         end
 
@@ -84,31 +84,31 @@
           let(:sf2_enabled) { true }
 
           it "returns :sf2" do
-            expect(Yast::FirewallChooser.detect).to eql(:sf2)
+            expect(subject.find_preferred).to eql(:sf2)
           end
         end
 
         context "and firewalld is enabled" do
           let(:fwd_enabled) { true }
 
           it "returns :fwd" do
-            expect(Yast::FirewallChooser.detect).to eql(:fwd)
+            expect(subject.find_preferred).to eql(:fwd)
           end
         end
 
         context "and SuSEfirewall2 is enabled" do
           let(:sf2_enabled) { true }
 
           it "returns :sf2" do
-            expect(Yast::FirewallChooser.detect).to eql(:sf2)
+            expect(subject.find_preferred).to eql(:sf2)
           end
         end
       end
 
       context "and firewalld is running" do
         let(:fwd_running) { true }
         it "returns :fwd" do
-          expect(Yast::FirewallChooser.detect).to eql(:fwd)
+          expect(subject.find_preferred).to eql(:fwd)
         end
       end
 
@@ -117,7 +117,7 @@
         let(:fwd_installed) { true }
         let(:sf2_running) { true }
         it "returns :sf2" do
-          expect(Yast::FirewallChooser.detect).to eql(:sf2)
+          expect(subject.find_preferred).to eql(:sf2)
         end
       end
 
@@ -128,7 +128,7 @@
         let(:fwd_running) { true }
 
         it "raises and exception" do
-          expect { Yast::FirewallChooser.detect }
+          expect { subject.find_preferred }
             .to raise_error(Yast::SuSEFirewallMultipleBackends)
         end
       end

library/network/test/susefirewall2_test.rb

@@ -14,7 +14,7 @@ def reset_SuSEFirewallIsInstalled_cache
 end
 
 # Instantiate an SF2 object
-FakeFirewall = Yast::FirewallChooser.choose(:sf2)
+FakeFirewall = Yast::FirewallChooser.new(:sf2).choose
 FakeFirewall.main
 
 describe FakeFirewall do

library/network/test/susefirewalld_test.rb

@@ -25,7 +25,7 @@ def reset_FirewallDIsInstalled_cache
 need_API_mock = true
 
 # Re-instansiate our objects
-FakeFirewallD = Yast::FirewallChooser.choose(:fwd)
+FakeFirewallD = Yast::FirewallChooser.new(:fwd).choose
 
 describe FakeFirewallD do