[PATCH aeolus-image-rubygem 2/2] Tests for Aeolus::Image::Katello code

Martyn Taylor mtaylor at redhat.com
Fri Jun 1 11:44:26 UTC 2012


On 05/31/2012 06:55 PM, Matt Wagner wrote:
> On Thu, May 31, 2012 at 03:46:09PM +0100, Martyn Taylor wrote:
>> Matt,
>>
>> It's difficult to review without any tests.
>>
>> I understand that you decided to implement a full client since it was
>> relatively straight forward and adding tests for the whole thing
>> would likely take some time and stray away from your original goal.
>> So can you at least add spec tests for the parts of the API that you
>> are trying to consume.  This will make it easy for reviewer to figure
>> out what is going on / how to use the client and also make sure that
>> ActiveResource and the API are compliant in each area.
> It's not a question of time, as much as that I started to question
> whether I was really adding any value with the tests.
>
> I actually started out writing tests for each model, testing each
> association I added. But when I found myself copying-and-pasting tests
> and just changing association names, I realized that something was
> wrong. Ultimately all of the tests I wrote just verified that has_many
> generated the correct URL. So I did away with those tests and just
> wrote a test for has_many itself. The tests do feel kind of bare, but
> ultimately I only added a couple of methods. The other classes don't add
> any methods, so I'm not sure it makes sense to try to write tests for
> them. Ultimately, though this is a large patch, there's not a ton of
> code in it.
Sure, I understand, your thinking here.

What I was really getting at is that we should be testing that what you 
have defined actually works with the Katello API.  So your not testing 
the ActiveResource as such; but you are checking that ActiveResource and 
the way you have structured you models works with the Katello API.  
Also, if we write the tests properly it means that we should be able to 
drop all the mock responses and update with new ones each time Katello 
has a new release.

One way to do this would be to use VCR to test against mock responses

We can talk when you get online mate.
>> Some other notes:
>>
>> Be careful when lifting code from other projects.  The base.rb file
>> in aeolus-image is bespoke to imagefactory and so turns off
>> certificate verfication since factory did not support this at the
>> time of writing the client.  I don't think this is necessary in
>> Katello since they support certificates.  It's probably worth while
>> looking over the rest of this class and remove anything that doesn't
>> make sense in respect to conductor.
> I was working against a server using a self-signed SSL cert, so that was
> actually there intentionally. But now that you mention it, this should
> really be a configurable option, not something permanently disabled.
>
>> Lastly, as we discussed in IRC we should probably pull this out into
>> a separate gem and contribute it back to Katello, its fine to live
>> here for now, but I'dhope we can pull this out later.
> Yes, I think breaking this out on its own would be slick.
>
>> I''l have to nack this for now until we have some proper tests.
> Thanks for the review, Martyn. I'll connect with you tomorrow to figure
> out what it makes sense to add for tests.
>
> -- Matt




More information about the aeolus-devel mailing list