From dbf6c93107843736d2daee9675d8d0d5e2e97a5f Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Tue, 6 Dec 2022 18:25:23 -0500
Subject: [PATCH 1/4] fix sporadic errors with dispatcher specs [specs only]

We are triggering queue_signal for every flow, both for error and success cases.
So detecting if the queue_signal is called doesn't mean anything.

The positive cases were passing because either the vms were not updated or it was not calling dispatch at all.
The negative cases were passing because an authorization was missing, which is not actually what was under test.

The host now has authentication so we can do the tests
the ems now has authentication so we can do more tests
The vms are updated properly (it was not saving the storage before)
The vms all have an ems (so they have authentications are scannable)
The storages and repo_storage have an ems
The tests are stating the signal they are expecting

(not necessary)
removed constants from specs (constants are defined in the global, not just in this spec)
reference ems instead of ems id in factory
---
 .../infra_manager/metrics_capture_spec.rb     |  2 +-
 .../vm_scan/dispatcher_embedded_scan_spec.rb  | 25 +++++-----
 spec/models/vm_scan/dispatcher_spec.rb        | 49 ++++++++-----------
 .../dispatcher_vm_storage2proxies_spec.rb     |  7 +--
 spec/support/job_proxy_dispatcher_helper.rb   | 30 +++---------
 5 files changed, 45 insertions(+), 68 deletions(-)

diff --git a/spec/models/manageiq/providers/infra_manager/metrics_capture_spec.rb b/spec/models/manageiq/providers/infra_manager/metrics_capture_spec.rb
index eb48f80a45b..70b4aecc883 100644
--- a/spec/models/manageiq/providers/infra_manager/metrics_capture_spec.rb
+++ b/spec/models/manageiq/providers/infra_manager/metrics_capture_spec.rb
@@ -2,7 +2,7 @@
   include Spec::Support::MetricHelper
 
   let(:miq_server) { EvmSpecHelper.local_miq_server }
-  let(:ems) { FactoryBot.create(:ems_vmware, :zone => miq_server.zone) }
+  let(:ems) { FactoryBot.create(:ems_vmware_with_authentication, :zone => miq_server.zone) }
 
   before do
     storages = FactoryBot.create_list(:storage_vmware, 2)
diff --git a/spec/models/vm_scan/dispatcher_embedded_scan_spec.rb b/spec/models/vm_scan/dispatcher_embedded_scan_spec.rb
index f5ae412dd51..d71987e2c0d 100644
--- a/spec/models/vm_scan/dispatcher_embedded_scan_spec.rb
+++ b/spec/models/vm_scan/dispatcher_embedded_scan_spec.rb
@@ -2,11 +2,11 @@
   describe "dispatch embedded" do
     include Spec::Support::JobProxyDispatcherHelper
 
-    NUM_OF_VMS = 5
-    NUM_OF_REPO_VMS = 0
-    NUM_OF_HOSTS = 3
-    NUM_OF_SERVERS = 3
-    NUM_OF_STORAGES = 3
+    let(:vm_count)      { 5 }
+    let(:repo_vm_count) { 0 }
+    let(:host_count)    { 3 }
+    let(:server_count)  { 3 }
+    let(:storage_count) { 3 }
 
     def assert_at_most_x_scan_jobs_per_y_resource(x_scans, y_resource)
       vms_in_embedded_scanning = Job.where(["dispatch_status = ? AND state != ? AND target_class = ?", "active", "finished", "VmOrTemplate"])
@@ -42,22 +42,18 @@ def assert_at_most_x_scan_jobs_per_y_resource(x_scans, y_resource)
       let(:zone) { FactoryBot.create(:zone) }
       before do
         server = EvmSpecHelper.local_miq_server(:is_master => true, :name => "test_server_main_server", :zone => zone)
-        (NUM_OF_SERVERS - 1).times do |i|
-          FactoryBot.create(:miq_server, :zone => server.zone, :name => "test_server_#{i}")
-        end
+        FactoryBot.create_list(:miq_server, server_count - 1, :zone => server.zone)
 
         # TODO: We should be able to set values so we don't need to stub behavior
         allow_any_instance_of(MiqServer).to receive_messages(:is_vix_disk? => true)
         allow_any_instance_of(MiqServer).to receive_messages(:is_a_proxy? => true)
         allow_any_instance_of(MiqServer).to receive_messages(:has_active_role? => true)
-        allow_any_instance_of(ManageIQ::Providers::Vmware::InfraManager).to receive_messages(:authentication_status_ok? => true)
-        allow_any_instance_of(Host).to receive_messages(:authentication_status_ok? => true)
 
         @hosts, @proxies, @storages, @vms, @repo_vms = build_entities(
-          :hosts    => NUM_OF_HOSTS,
-          :storages => NUM_OF_STORAGES,
-          :vms      => NUM_OF_VMS,
-          :repo_vms => NUM_OF_REPO_VMS,
+          :hosts    => host_count,
+          :storages => storage_count,
+          :vms      => vm_count,
+          :repo_vms => repo_vm_count,
           :zone     => zone,
         )
       end
@@ -78,6 +74,7 @@ def assert_at_most_x_scan_jobs_per_y_resource(x_scans, y_resource)
             it "should signal 2 jobs to start" do
               stub_settings(:coresident_miqproxy => {:concurrent_per_ems => 2},
                             :ems                 => {:ems_amazon => {}})
+              MiqQueue.truncate
               VmScan::Dispatcher.dispatch
               expect(MiqQueue.count).to eq(2)
             end
diff --git a/spec/models/vm_scan/dispatcher_spec.rb b/spec/models/vm_scan/dispatcher_spec.rb
index 6552ee6fc7f..44e580a4d91 100644
--- a/spec/models/vm_scan/dispatcher_spec.rb
+++ b/spec/models/vm_scan/dispatcher_spec.rb
@@ -71,11 +71,9 @@
 
       context "with a vm without a storage" do
         before do
-          # Test a vm without a storage (ie, removed from VC but retained in the VMDB)
-          @vm = @vms.first
-          @vm.storage = nil
-          @vm.save
-          @vm.raw_scan
+          vm = @vms.first
+          vm.update(:storage => nil)
+          vm.raw_scan
         end
 
         it "should expect queue_signal and dispatch without errors" do
@@ -86,47 +84,42 @@
 
       context "with a Microsoft vm without a storage" do
         before do
-          # Test a Microsoft vm without a storage
-          @vm = @vms.first
-          @vm.storage = nil
-          @vm.vendor = "microsoft"
-          @vm.save
-          @vm.raw_scan
+          vm = @vms.first
+          vm.update(:storage => nil, :vendor => "microsoft")
+          vm.raw_scan
         end
 
-        it "should run dispatch without calling queue_signal" do
-          expect(dispatcher).not_to receive(:queue_signal)
+        it "triggers an error" do
+          expect(dispatcher).to receive(:queue_signal).with(anything, hash_including(:args => [:abort, /not located on a storage/, "error"]))
+          dispatcher.dispatch
         end
       end
 
       context "with a Microsoft vm with a Microsoft storage" do
         before do
-          # Test a Microsoft vm without a storage
-          @vm = @vms.first
-          @vm.storage.store_type = "CSVFS"
-          @vm.vendor = "microsoft"
-          @vm.save
-          @vm.raw_scan
+          vm = @vms.first
+          vm.storage.update(:store_type => "CSVFS")
+          vm.update(:vendor => "microsoft")
+          vm.raw_scan
         end
 
-        it "should run dispatch without calling queue_signal" do
-          expect(dispatcher).not_to receive(:queue_signal)
+        it "start collecting" do
+          expect(dispatcher).to receive(:queue_signal).with(anything, hash_including(:args => ["start"]))
+          dispatcher.dispatch
         end
       end
 
       context "with a Microsoft vm with an invalid storage" do
         before do
-          # Test a Microsoft vm without a storage
           @vm = @vms.first
-          @vm.storage.store_type = "XFS"
-          @vm.vendor = "microsoft"
-          @vm.save
+          @vm.storage.update(:store_type => "ABC")
+          @vm.update(:vendor => "microsoft")
           @vm.raw_scan
         end
 
-        it "should expect queue_signal and dispatch without errors" do
-          expect(dispatcher).to receive(:queue_signal)
-          expect { dispatcher.dispatch }.not_to raise_error
+        it "triggers an error" do
+          expect(dispatcher).to receive(:queue_signal).with(anything, hash_including(:args => [:abort, /unsupported/, "error"]))
+          dispatcher.dispatch
         end
       end
 
diff --git a/spec/models/vm_scan/dispatcher_vm_storage2proxies_spec.rb b/spec/models/vm_scan/dispatcher_vm_storage2proxies_spec.rb
index 770cfae3485..4f0d20c2c32 100644
--- a/spec/models/vm_scan/dispatcher_vm_storage2proxies_spec.rb
+++ b/spec/models/vm_scan/dispatcher_vm_storage2proxies_spec.rb
@@ -83,12 +83,13 @@
 
             context "a vm template and invalid VC authentication" do
               before do
-                allow_any_instance_of(ManageIQ::Providers::Vmware::InfraManager).to receive_messages(:missing_credentials? => true)
-                allow(@vm).to receive_messages(:template? => true)
+                @vm.template = true
+                @hosts.each { |host| host.authentications.destroy_all }
                 @ems1 = FactoryBot.create(:ems_vmware, :name => "Ems1")
                 @vm.ext_management_system = @ems1
                 @vm.save
               end
+
               it "Vm#storage2active_proxies will return an empty list" do
                 expect(@vm.storage2active_proxies).to be_empty
               end
@@ -96,7 +97,7 @@
 
             context "a vm and invalid host authentication" do
               before do
-                allow_any_instance_of(Host).to receive_messages(:missing_credentials? => true)
+                @hosts.each { |host| host.authentications.destroy_all }
                 allow(@vm).to receive_messages(:template? => false)
               end
               it "Vm#storage2active_proxies will return an empty list" do
diff --git a/spec/support/job_proxy_dispatcher_helper.rb b/spec/support/job_proxy_dispatcher_helper.rb
index 17dcade0bd5..e416f3fbe61 100644
--- a/spec/support/job_proxy_dispatcher_helper.rb
+++ b/spec/support/job_proxy_dispatcher_helper.rb
@@ -5,16 +5,13 @@ def build_entities(options = {})
         options = {:hosts => 2, :storages => 2, :vms => 3, :repo_vms => 3, :container_providers => [1, 2], :zone => FactoryBot.create(:zone)}.merge(options)
 
         proxies = []
-        storages = []
-        options[:storages].times do |i|
-          storage = FactoryBot.create(:storage, :name => "test_storage_#{i}", :store_type => "VMFS")
-          storages << storage
-        end
 
-        ems = FactoryBot.create(:ems_vmware, :name => "ems1", :zone => options[:zone])
+        ems = FactoryBot.create(:ems_vmware, :name => "ems1", :zone => options[:zone], :authtype => :default)
+        storages = FactoryBot.create_list(:storage, options[:storages], :store_type => "VMFS", :ext_management_system => ems)
+
         hosts = []
         options[:hosts].times do |i|
-          host = FactoryBot.create(:host, :name => "test_host_#{i}", :hostname => "test_host_#{i}")
+          host = FactoryBot.create(:host_with_authentication, :name => "test_host_#{i}", :hostname => "test_host_#{i}")
           max = i > storages.length ? storages.length : i
           host.storages = storages[0..max]
           host.ext_management_system = ems
@@ -32,30 +29,19 @@ def build_entities(options = {})
           vms << vm
         end
 
-        repo_vms = []
-
-        repo_storage = FactoryBot.create(:storage, :name => "test_repo_storage", :store_type => "VMFS")
-        repo_storage.hosts = []
-        repo_storage.save
-
-        options[:repo_vms].times do |i|
-          vm = FactoryBot.create(:vm_vmware, :name => "test_repo_vm_#{i}", :location => "abc/abc.vmx")
-          vm.storage = repo_storage
-          vm.host = nil
-          vm.save
-          repo_vms << vm
-        end
+        repo_storage = FactoryBot.create(:storage, :name => "test_repo_storage", :store_type => "VMFS", :hosts => [], :ext_management_system => ems)
+        repo_vms = FactoryBot.create_list(:vm_vmware, options[:repo_vms], :location => "abc/abc.vmx", :ext_management_system => ems, :host => nil, :storage => repo_storage)
 
         container_providers = []
         options[:container_providers].each_with_index do |images_count, i|
-          ems_openshift = FactoryBot.create(:ems_openshift, :name => "test_container_provider_#{i}", :zone => options[:zone])
+          ems_openshift = FactoryBot.create(:ems_openshift, :name => "test_container_provider_#{i}", :zone => options[:zone], :authtype => :default)
           container_providers << ems_openshift
           container_image_classes = ContainerImage.descendants.append(ContainerImage)
           images_count.times do |idx|
             container_image_classes.each do |cic|
               FactoryBot.create(:container_image,
                                  :name   => "test_container_images_#{idx}",
-                                 :ems_id => ems_openshift.id,
+                                 :ext_management_system => ems_openshift,
                                  :type   => cic.name)
             end
           end

From 57aead5c1306ccf4b0a3c3758afcd0e6e5c0fc15 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Thu, 1 Dec 2022 16:05:59 -0500
Subject: [PATCH 2/4] rename method supports? to storage_type_supported? to
 avoid SupportsFeatureMixin conflict

BTW/ This method name is a little tricky.

The field is store_type
but the constant is SUPPORTED_STORAGE_TYPES
---
 .../providers/infra_manager/metrics_capture.rb       |  2 +-
 app/models/storage.rb                                |  7 +++----
 spec/models/storage_spec.rb                          | 12 ++++++++++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/app/models/manageiq/providers/infra_manager/metrics_capture.rb b/app/models/manageiq/providers/infra_manager/metrics_capture.rb
index 8d6838e7d98..7dc3583d068 100644
--- a/app/models/manageiq/providers/infra_manager/metrics_capture.rb
+++ b/app/models/manageiq/providers/infra_manager/metrics_capture.rb
@@ -78,7 +78,7 @@ def capture_host_targets(ems)
   # @return [Array<Storage>] supported storages
   # hosts preloaded storages and tags
   def capture_storage_targets(hosts)
-    hosts.flat_map(&:storages).uniq.select { |s| Storage.supports?(s.store_type) & s.perf_capture_enabled? }
+    hosts.flat_map(&:storages).uniq.select { |s| s.storage_type_supported_for_ssa? & s.perf_capture_enabled? }
   end
 
   # @param [ExtManagementSystem] ems
diff --git a/app/models/storage.rb b/app/models/storage.rb
index 6b93907baa8..5d9f5e15fee 100644
--- a/app/models/storage.rb
+++ b/app/models/storage.rb
@@ -343,7 +343,7 @@ def self.scan_timer(zone_name = nil)
   end
 
   def scan(userid = "system", _role = "ems_operations")
-    unless SUPPORTED_STORAGE_TYPES.include?(store_type.upcase)
+    unless storage_type_supported_for_ssa?
       raise(MiqException::MiqUnsupportedStorage,
             _("Action not supported for Datastore type [%{store_type}], [%{name}] with id: [%{id}]") %
               {:store_type => store_type, :name => name, :id => id})
@@ -815,9 +815,8 @@ def vm_scan_affinity
     with_relationship_type("vm_scan_storage_affinity") { parents }
   end
 
-  # @param [String, Storage] store_type upcased version of the storage type
-  def self.supports?(store_type)
-    Storage::SUPPORTED_STORAGE_TYPES.include?(store_type)
+  def storage_type_supported_for_ssa?
+    SUPPORTED_STORAGE_TYPES.include?(store_type.upcase)
   end
 
   def self.display_name(number = 1)
diff --git a/spec/models/storage_spec.rb b/spec/models/storage_spec.rb
index 50f2f10f070..85c82b37348 100644
--- a/spec/models/storage_spec.rb
+++ b/spec/models/storage_spec.rb
@@ -506,4 +506,16 @@
       expect(storage.tenant_identity.current_tenant).to eq(Tenant.root_tenant)
     end
   end
+
+  describe "#storage_type_supported_for_ssa?" do
+    it "detects bad storage type" do
+      storage = FactoryBot.build(:storage, :store_type => "XYZ")
+      expect(storage.storage_type_supported_for_ssa?).to eq(false)
+    end
+
+    it "detects good storage type" do
+      storage = FactoryBot.build(:storage, :store_type => "NFS")
+      expect(storage.storage_type_supported_for_ssa?).to eq(true)
+    end
+  end
 end

From 2fde639b74b2f52210b52aa6839d1369d5669ed1 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Thu, 1 Dec 2022 13:52:23 -0500
Subject: [PATCH 3/4] make supported storage types consistent

The listed of supported storage types was different between metrics capture, vm_scan, and vm path
This made them consistent.
---
 app/models/storage.rb            |  2 +-
 app/models/vm_or_template.rb     | 12 ++++--------
 app/models/vm_scan/dispatcher.rb |  2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/app/models/storage.rb b/app/models/storage.rb
index 5d9f5e15fee..ee4b56ef2c3 100644
--- a/app/models/storage.rb
+++ b/app/models/storage.rb
@@ -67,7 +67,7 @@ class Storage < ApplicationRecord
 
   delegate :queue_name_for_ems_operations, :to => :ext_management_system, :allow_nil => true
 
-  SUPPORTED_STORAGE_TYPES = %w( VMFS NFS NFS41 FCP ISCSI GLUSTERFS )
+  SUPPORTED_STORAGE_TYPES = %w[VSAN VMFS NAS NFS NFS41 ISCSI DIR FCP CSVFS NTFS GLUSTERFS].freeze
 
   supports :smartstate_analysis do
     if !ext_management_system&.class&.supports?(:smartstate_analysis)
diff --git a/app/models/vm_or_template.rb b/app/models/vm_or_template.rb
index a8c2d4c64d8..6017f6777ab 100644
--- a/app/models/vm_or_template.rb
+++ b/app/models/vm_or_template.rb
@@ -1143,14 +1143,10 @@ def path
     # Return location for RHEV-M VMs
     return rhevm_config_path if vendor.to_s.downcase == 'redhat'
 
-    case storage.store_type
-    when "VMFS"  then "[#{storage.name}] #{location}"
-    when "VSAN"  then "[#{storage.name}] #{location}"
-    when "NFS"   then "[#{storage.name}] #{location}"
-    when "NFS41" then "[#{storage.name}] #{location}"
-    when "NTFS"  then "[#{storage.name}] #{location}"
-    when "CSVFS" then "[#{storage.name}] #{location}"
-    when "NAS"   then File.join(storage.name, location)
+    if storage.store_type == "NAS"
+      File.join(storage.name, location)
+    elsif storage.storage_type_supported_for_ssa?
+      "[#{storage.name}] #{location}"
     else
       _log.warn("VM [#{name}] storage type [#{storage.store_type}] not supported")
       @path = location
diff --git a/app/models/vm_scan/dispatcher.rb b/app/models/vm_scan/dispatcher.rb
index 3204aee977c..e4ce4d23321 100644
--- a/app/models/vm_scan/dispatcher.rb
+++ b/app/models/vm_scan/dispatcher.rb
@@ -252,7 +252,7 @@ def get_eligible_proxies_for_job(job)
           queue_signal(job, {:args => [:abort, msg, "error"]})
           return []
         else
-          unless %w[VSAN VMFS NAS NFS NFS41 ISCSI DIR FCP CSVFS NTFS GLUSTERFS].include?(@vm.storage.store_type)
+          unless @vm.storage.storage_type_supported_for_ssa?
             msg = "Vm storage type [#{@vm.storage.store_type}] unsupported [#{job.target_id}], aborting job [#{job.guid}]."
             queue_signal(job, {:args => [:abort, msg, "error"]})
             return []

From ac06484a1804583ddd0dceb5e6a64053e34ea077 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Tue, 6 Dec 2022 17:53:25 -0500
Subject: [PATCH 4/4] fix storage.supports(:smartstate) to take store_type into
 account

I had replaced storage.storage_type_supports_ssa? calls to just use supports(:smartstate_analysis)
but I wasn't sure if that was correct for a few.

But I did keep ems authentication changes - they seemed like they may make introduce some false negatives
in some areas of the code
---
 .../infra_manager/metrics_capture.rb          |  2 +-
 app/models/storage.rb                         | 13 +++++-------
 app/models/vm_scan/dispatcher.rb              |  4 ++--
 spec/models/storage_spec.rb                   | 21 +++++++++++++++----
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/models/manageiq/providers/infra_manager/metrics_capture.rb b/app/models/manageiq/providers/infra_manager/metrics_capture.rb
index 7dc3583d068..b0f7f2d85f6 100644
--- a/app/models/manageiq/providers/infra_manager/metrics_capture.rb
+++ b/app/models/manageiq/providers/infra_manager/metrics_capture.rb
@@ -78,7 +78,7 @@ def capture_host_targets(ems)
   # @return [Array<Storage>] supported storages
   # hosts preloaded storages and tags
   def capture_storage_targets(hosts)
-    hosts.flat_map(&:storages).uniq.select { |s| s.storage_type_supported_for_ssa? & s.perf_capture_enabled? }
+    hosts.flat_map(&:storages).uniq.select { |s| s.supports?(:smartstate_analysis) & s.perf_capture_enabled? }
   end
 
   # @param [ExtManagementSystem] ems
diff --git a/app/models/storage.rb b/app/models/storage.rb
index ee4b56ef2c3..2cf8b277d8f 100644
--- a/app/models/storage.rb
+++ b/app/models/storage.rb
@@ -74,6 +74,8 @@ class Storage < ApplicationRecord
       _("Smartstate Analysis cannot be performed on selected Datastore")
     elsif !ext_management_system&.authentication_status_ok?
       _("There are no EMSs with valid credentials for this Datastore")
+    elsif !storage_type_supported_for_ssa?
+      _("Smartstate Analysis unsupported for storage type %{store_type}" % {:store_type => store_type})
     end
   end
 
@@ -343,17 +345,12 @@ def self.scan_timer(zone_name = nil)
   end
 
   def scan(userid = "system", _role = "ems_operations")
-    unless storage_type_supported_for_ssa?
+    unless supports?(:smartstate_analysis)
       raise(MiqException::MiqUnsupportedStorage,
-            _("Action not supported for Datastore type [%{store_type}], [%{name}] with id: [%{id}]") %
-              {:store_type => store_type, :name => name, :id => id})
+            _("Action not supported for Datastore type [%{store_type}], [%{name}] with id: [%{id}] %{error}") %
+              {:store_type => store_type, :name => name, :id => id, :error => unsupported_reason(:smartstate_analysis)})
     end
 
-    unless ext_management_system&.authentication_status_ok?
-      raise(MiqException::MiqStorageError,
-            _("Check that an EMS has valid credentials for Datastore [%{name}] with id: [%{id}]") %
-              {:name => name, :id => id})
-    end
     task_name = "SmartState Analysis for [#{name}]"
     self.class.create_scan_task(task_name, userid, [self])
   end
diff --git a/app/models/vm_scan/dispatcher.rb b/app/models/vm_scan/dispatcher.rb
index e4ce4d23321..8f5ee293056 100644
--- a/app/models/vm_scan/dispatcher.rb
+++ b/app/models/vm_scan/dispatcher.rb
@@ -252,8 +252,8 @@ def get_eligible_proxies_for_job(job)
           queue_signal(job, {:args => [:abort, msg, "error"]})
           return []
         else
-          unless @vm.storage.storage_type_supported_for_ssa?
-            msg = "Vm storage type [#{@vm.storage.store_type}] unsupported [#{job.target_id}], aborting job [#{job.guid}]."
+          unless @vm.storage.supports?(:smartstate_analysis)
+            msg = @vm.storage.unsupported_reason(:smartstate_analysis)
             queue_signal(job, {:args => [:abort, msg, "error"]})
             return []
           end
diff --git a/spec/models/storage_spec.rb b/spec/models/storage_spec.rb
index 85c82b37348..18149ede6a0 100644
--- a/spec/models/storage_spec.rb
+++ b/spec/models/storage_spec.rb
@@ -395,13 +395,26 @@
   end
 
   context "#supports?(:smartstate_analysis)" do
-    it "returns true for VMware Storage when queried whether it supports smartstate analysis" do
-      storage = FactoryBot.create(:storage, :ext_management_system => FactoryBot.create(:ems_vmware_with_authentication))
+    it "supports smartstate if the ems supports smartstate analysis" do
+      ems = FactoryBot.create(:ems_vmware_with_authentication)
+      expect(ems.supports?(:smartstate_analysis)).to eq(true)
+      storage = FactoryBot.create(:storage_nfs, :ext_management_system => ems)
+
       expect(storage.supports?(:smartstate_analysis)).to eq(true)
     end
 
-    it "returns false for non-vmware Storage when queried whether it supports smartstate analysis" do
-      storage = FactoryBot.create(:storage, :ext_management_system => FactoryBot.create(:ems_microsoft_with_authentication))
+    it "doesn't support smartstate for an unknown store_type" do
+      ems = FactoryBot.create(:ems_vmware_with_authentication)
+      storage = FactoryBot.create(:storage_unknown, :ext_management_system => ems)
+
+      expect(storage.supports?(:smartstate_analysis)).to eq(false)
+    end
+
+    it "doesn't support smartstate for an ems that doesn't support smartstate analysis" do
+      ems = FactoryBot.create(:ems_microsoft_with_authentication)
+      expect(ems.supports?(:smartstate_analysis)).not_to eq(true)
+      storage = FactoryBot.create(:storage_nfs, :ext_management_system => ems)
+
       expect(storage.supports?(:smartstate_analysis)).to_not eq(true)
     end
   end