Use a filter to allow listing of inactive pools

electrofelix commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@geemus the problem is in testing the current behaviour of the following code https://github.com/fog/fog-libvirt/blob/64b43373722539f89458a36c02f01feb057b2a39/lib/fog/libvirt/requests/compute/list_pools.rb#L19-L46

The mock behaviour essentially by-passes testing this by returning a list of mock pools in https://github.com/fog/fog-libvirt/blob/64b43373722539f89458a36c02f01feb057b2a39/lib/fog/libvirt/requests/compute/list_pools.rb#L49-L53 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 https://github.com/fog/fog-libvirt/blob/64b43373722539f89458a36c02f01feb057b2a39/lib/fog/libvirt/compute.rb#L55-L73

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 commented 3 years ago

@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).