Question: Current use of filters in requests can result in test code being unable to exercise behaviour

[electrofelix]

A number of the requests support filters that change how libvirt is queried for the various resources:

fog-libvirt/lib/fog/libvirt/requests/compute/list_domains.rb

Lines 4 to 28 in 7e534d1

	class Real
	def list_domains(filter = { })
	data=[]
	if filter.key?(:uuid)
	data << client.lookup_domain_by_uuid(filter[:uuid])
	elsif filter.key?(:name)
	data << client.lookup_domain_by_name(filter[:name])
	else
	client.list_defined_domains.each { |name| data << catchLibvirtExceptions { client.lookup_domain_by_name(name) } } unless filter[:defined] == false
	client.list_domains.each { |id| data << catchLibvirtExceptions { client.lookup_domain_by_id(id) } } unless filter[:active] == false
	end
	data.compact.map { |d| domain_to_attributes d }.compact
	end
	# Catch Libvirt exceptions to avoid race conditions involving
	# concurrent libvirt operations from other processes. For example,
	# domains being undefined while fog-libvirt is trying to work with
	# domain lists.
	def catchLibvirtExceptions
	yield
	rescue ::Libvirt::RetrieveError, ::Libvirt::Error
	nil
	end
	end

Corresponding Mock:

fog-libvirt/lib/fog/libvirt/requests/compute/list_domains.rb

Lines 97 to 103 in 7e534d1

	class Mock
	def list_domains(filter = { })
	dom1 = mock_domain 'fog-dom1'
	dom2 = mock_domain 'fog-dom2'
	dom3 = mock_domain 'a-fog-dom3'
	[dom1, dom2, dom3]
	end

however because the filter is fully processed in the Real object, and the Mock just ignores it, it means that exercising any of the logic to ensure information is being retrieved or handled correctly around the filters, can be quite tricky.

Consequently this appears to result in the need to duplicate the filtering behaviour in the Mock instance that will subsequently need to be kept aligned with the Real instance. e.g.

fog-libvirt/lib/fog/libvirt/requests/compute/list_networks.rb

Lines 46 to 69 in 7e534d1

	class Mock
	def list_networks(filter={ })
	networks = [ {
	:uuid => 'a29146ea-39b2-412d-8f53-239eef117a32',
	:name => 'net1',
	:bridge_name => 'virbr0'
	},
	{
	:uuid => 'fbd4ac68-cbea-4f95-86ed-22953fd92384',
	:name => 'net2',
	:bridge_name => 'virbr1'
	}
	]
	return networks if filter.empty?
	case filter.keys.first
	when :uuid
	[networks.find(:uuid => filter[:uuid]).first]
	when :name
	[networks.find(:name => filter[:name]).first]
	else
	networks
	end
	end

needs to filter on the same attributes as:

fog-libvirt/lib/fog/libvirt/requests/compute/list_networks.rb

Lines 4 to 15 in 7e534d1

	class Real
	def list_networks(filter = { })
	data=[]
	if filter.keys.empty?
	(client.list_networks + client.list_defined_networks).each do |network_name|
	data << network_to_attributes(client.lookup_network_by_name(network_name))
	end
	else
	data = [network_to_attributes(get_network_by_filter(filter))]
	end
	data
	end

Is this the intended usage for Mocks vs Real? Or should the specific requests be wrapped with private functions for both the Mock and Real with the filter code being common for both and subsequently calling the private methods?
e.g.:

module Fog
  module Libvirt
    class Compute
        def self.list_networks(filter = { })
          data=[]
          if filter.keys.empty?
            (list_networks + list_defined_networks).each do |network_name|
              data << network_to_attributes()
            end
          else
            data = [network_to_attributes(get_network_by_filter(filter))]
          end
          data
        end

        def self.get_network_by_filter(filter = { })
          case filter.keys.first
            when :uuid
              lookup_by_uuid(filter[:uuid])
            when :name
              lookup_by_name(filter[:name])
          end
        end
      end

      class Real
        private
        def list_networks
          client.list_networks
        end

        def list_networks_defined
          client.list_networks_defined
        end

        def lookup_by_name(name)
          client.lookup_network_by_name(network_name)
        end

        def lookup_by_uuid(uuid)
          client.lookup_network_by_uuid(uuid)
        end
      end

...

That way the Mock can implement the four calls list_networks, list_networks_defined, lookup_by_name and lookup_by_uuid, retrieving information from an internal stored array, but it still ensures the main logic is exercised and it is only the client request part itself that ends up needing to be mocked and kept in sync.

Note that I'm not expecting the above to be usable as is, just a way of trying to outline how it's a bit difficult to ensure the filtering code in list_networks in the existing code is correct given that any tests won't exercise it, and it's unclear if this is the intended approach or something that has be caused by the fact that for libvirt the filtering results in different API calls being made.