Use a filter to allow listing of inactive pools

[electrofelix]

Allow for an :include_inactive filter attribute to determine whether
to include or exclude inactive pools and handle correctly setting the
number of volumes in the case it is inactive.

Current testing approach requires modifying the internal function in
order to allow the Mock to call it to ensure it is exercised correctly.

Fixes: #12

[geemus]

I'm not really very familiar with the libvirt portions of this, so a little unclear on specifically where the mock behavior is in question. That being said, fog-local might be a good example of the approach we've used elsewhere (since it is local file writes in real mode, it should hopefully be easy to follow and understand vs needing to know more about a specific provider). You can see how the mock data is stored/used here: https://github.com/fog/fog-local/blob/master/lib/fog/local/storage.rb. Happy to discuss more too, but hopefully that should give you some foundation to learn from.

[electrofelix]

@geemus the problem is in testing the current behaviour of the following code

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

Lines 19 to 46 in 64b4337

	private
	def pool_to_attributes(pool)
	states=[:inactive, :building, :running, :degrated, :inaccessible]
	{
	:uuid => pool.uuid,
	:persistent => pool.persistent?,
	:autostart => pool.autostart?,
	:active => pool.active?,
	:name => pool.name,
	:allocation => pool.info.allocation,
	:capacity => pool.info.capacity,
	:num_of_volumes => pool.num_of_volumes,
	:state => states[pool.info.state]
	}
	end
	def find_pool_by_name name
	pool_to_attributes(client.lookup_storage_pool_by_name(name))
	rescue ::Libvirt::RetrieveError
	nil
	end
	def find_pool_by_uuid uuid
	pool_to_attributes(client.lookup_storage_pool_by_uuid(uuid))
	rescue ::Libvirt::RetrieveError
	nil
	end
	end

The mock behaviour essentially by-passes testing this by returning a list of mock pools in

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

Lines 49 to 53 in 64b4337

	def list_pools(filter = { })
	pool1 = mock_pool 'pool1'
	pool2 = mock_pool 'pool1'
	[pool1, pool2]
	end

So when writing tests it's not clear how to test changes to the functionality of the pool_to_attributes function.

The result of the pool_to_attributes function looks to fit into the fog model of a pool https://github.com/fog/fog-libvirt/blob/64b43373722539f89458a36c02f01feb057b2a39/lib/fog/libvirt/models/compute/pool.rb, which is fine, except what really needs to be tested is how the function handles converting a libvirt pool to a fog pool model and then how they are presented when listing all pools.

I see from the local storage that one possible solution is to duplicate the pool_to_attributes function in both the mock and real object:
Mock
https://github.com/fog/fog-local/blob/bf609ca9ad618dc2bf2e14013e9bb06a322e153d/lib/fog/local/storage.rb#L57-L62

Real
https://github.com/fog/fog-local/blob/bf609ca9ad618dc2bf2e14013e9bb06a322e153d/lib/fog/local/storage.rb#L89-L94

I was looking to avoid this as it requires making sure you keep them in sync, additionally the only way I could think of testing the conversion to a fog model was to construct a fake pool object that would look like what is returned by ruby-libvirt for a pool but with different internal data controlled by the test.

It feels like the mock class in https://github.com/fog/fog-libvirt/blob/master/lib/fog/libvirt/requests/compute/list_pools.rb is intended to be used by everything else that needs to call list_pools, and tests for the list pools request should be somehow using the mocked client at

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

Lines 55 to 73 in 64b4337

	class Mock
	include Shared
	def initialize(options={})
	# libvirt is part of the gem => ruby-libvirt
	require 'libvirt'
	end
	private
	def client
	return @client if defined?(@client)
	end
	#read mocks xml
	def read_xml(file_name)
	file_path = File.join(File.dirname(__FILE__),"requests","compute","mock_files",file_name)
	File.read(file_path)
	end
	end

but I don't see any examples of how to accomplish this as calls to list_pools in the requests test appear to go to the mock request rather than the real request with a mocked client:
https://github.com/fog/fog-libvirt/pull/95/files#diff-be4f38ac759613aedbdee5156ed3d603cb91ab112054e6aa817da3baa2d08237R5-R9

If I was to use the fog-local as a comparison, how does the copy_object code get tested which is called at:
https://github.com/fog/fog-local/blob/8832480bd0400df6208c0aa5a1ae1cc1c640bf45/lib/fog/local/models/file.rb#L37-L42

and appears to be defined at https://github.com/fog/fog-local/blob/master/lib/fog/local/storage.rb#L82-L87

It's not clear to me that it is currently exercised by the tests?

[geemus]

@electrofelix Thanks for the clarification, I think I have a much better idea of the question now.

I think there might actually be two different questions here we can tease apart. My hope is that by splitting this out a bit it will maybe make it easier to discuss (and implement), but definitely let me know if I'm still confusing things.

First, the core of your question, the pool_to_attributes stuff. I think this actually is more like some other fog tests that start with a known dataset in the format returned from the provider and use a method such as this and check that the output matches what we would expect. I think for this it probably makes sense to just do this directly with at least one sample dataset (but you could do more if there are particular edge cases or other things you felt were worth testing.

Second, the somewhat separate question of the list pool mock implementation. It definitely doesn't provide much flexibility as it stands. In some of the other fog providers we get around this by having things like list_pool look up in the stored mock data (and other calls can then update that data, so for instance creating pools would change the list). Implementing mock in this way is certainly more complex, but also brings it more inline with real usage (which makes the mocks more useful).

[github-actions]

This pr has been marked inactive and will be closed if no further activity occurs.

[electrofelix]

hoping to get back to this next month

[electrofelix]

Still haven't gotten to it, but hopefully soon, still on my radar though

[electrofelix]

@geemus I finally got back to tweaking this, I think the main stumbling block though is the need to test the behaviour of something that is private but without having any easy way to access it via a public interface.

I've put up an adjusted patch based on my original approach, and then added a commit on top that limits the changes to having the predefined pools in the main mock and calling the private method from the tests to check it's behaviour. I'm not sure it's actually cleaner since it means knowledge of how the private method works is now directly referenced from the tests.

Please let me know if the subsequent commit is better or worse

[geemus]

@electrofelix Thanks for taking another look. As you suggest, there is some stumbling bits due to how all of this fits together. I think given that, your approach seems reasonable. Just let me know if you have further questions or would like to talk through it more.

[electrofelix]

@geemus which approach do you prefer? With or without the second commit?

[geemus]

@electrofelix - In other fog providers for things like this, I would tend to expect one to call something like "create_pool" and have that populate mocks. Am I correct in guessing that pools are not something you can directly control from libvirt? I don't see related commands, so I'm guessing this may be something that is handled in some other more direct way? Beyond that, I guess I lean a bit toward providing some way within mocks to manipulate pools if possible, because this seems more versatile. Other people working with/on things using this might also want to change pool settings within their own mocked tests and this could allow both uses (vs only doing in tests). Does that make sense?

[electrofelix]

The libvirt provider only accepts the raw XML for creating pools

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

Lines 10 to 13 in 085a7c8

	class Mock
	def define_pool(xml)
	end
	end

Probably a little out of scope of this bugfix to implement something to convert that call to storing pool data that can be used by mocks.

Are you just looking for the mock to have an add_pool method and its own pool data along with a reset function?
Should I leave all the fakepool handling code with the mock then and not with the test?

[electrofelix]

@geemus I just realised that with the subsequent change to move the FakePool to the tests code there is no need to modify the pools for the tests. It's not clear whether if additional functionality that would not be used by the test should be added in the same PR?

For this bug really the only thing that is of interest is to validate that pool_to_attributes correctly handles converting inactive pools to attributes to be subsequently returned by list_pools. Ideally it would also be nice to validate the pass thru in the list_pools method on the Real class since it contains more logic than just processing a request. Possibly there is some restructuring needed here but it's unclear to me what it should look like.

It would be possible to support additional FakePool objects being added based on a list of attributes being provide, which would then be passed through the pool_to_attributes method when list_pools is called on the mock. However it's also possibly less complex to have a method add a pool that contains the resolved attributes instead, in which case it wouldn't be used to validate the bug fix.

I can still add this, I just want to be sure that it's non usage by the test to validate the functional changes to pool_to_attributes makes sense and isn't going to be a cause for concern for adding excess code changes.

[geemus]

@electrofelix Yeah, I think that makes sense. Lets keep it simpler and more focused as you suggest in the current iteration. Thanks for working through this with me.