deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: re-send: [PATCH core 4/7] Core: Added initial RHEV-M unit tests
Date Thu, 26 Jul 2012 08:37:12 GMT


---
Michal Fojtik, Sr. Soft. Engineer
Deltacloud API / CloudForms
mfojtik@redhat.com

On Jul 26, 2012, at 12:39 AM, David Lutterkort wrote:

> On Wed, 2012-07-25 at 16:42 +0200, Michal Fojtik wrote:
>> Hi,
>> 
>> Seems like this patch is big for the list, so here you can grab it:
>> 
>> http://omicron.mifo.sk/0004-Core-Added-initial-RHEV-M-unit-tests.patch.gz
> 
> NAK because
> 
>      * this has user/password/provider hardwired in the code. It should
>        get these from ~/.deltacloud/config (or complain if that doesn't
>        exist)

I though that the server/tests/unit will not use this config (mean the tests should
require nothing from user to configure). The user/password/provider is there just to
record the VCR fixtures, then we're just replay-ing them without touching the provider.
I don't expect somebody will try to run these unit tests against 'real' RHEV-M, I though
Marios tests will be used in this case.

>      * Why doesn't tests/drivers/rhevm/common.rb require test_helper ?

Hmm. That looks wrong, the 'test_helper' is required by Rake task even before
the tests are started. Will correct that one.

>        That would also be the right place for TestPoller

I agree, TestPooler need to be moved to test_helper, since the same funcionality will
be used by EC2 and RHEV-M (and I believe in other driver tests too).


>      * Similarly, shouldn't the VCR magic go into test_helper ?

I don't think so, since the test_helper is also being used by 'base' tests (collections),
where VCR is not needed. 

I meant test_helper to be very small and contain just code that is shared across all tests,
like SimpleCov or 'require_relative' fix. More about VCR in next email :-)

> 
> There's also a couple of typos in comments: s/pool/poll/,
> s/TestPooler/TestPoller/ and s/new_epizode/new_episode/

Yeah, sorry for that :-) And thanks for spotting this. I'll fix them all in new revision.

> 
> David
> 
> 


Mime
View raw message