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

Matt Wagner matt.wagner at redhat.com
Thu May 31 17:55:33 UTC 2012


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.

> 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