[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise
Tomas Babej
tbabej at redhat.com
Tue Feb 24 18:03:17 UTC 2015
Hi Gabe,
sorry for the delay. Here comes the review!
1.) All the tests fail, since the IPA master is not installed at all:
def test_advice(self):
# Obtain the advice from the server
> tasks.kinit_admin(self.master)
test_integration/test_advise.py:37:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test_integration/tasks.py:484: in kinit_admin
stdin_text=host.config.admin_password)
../pytest_multihost/host.py:222: in run_command
command.wait(raiseonerr=raiseonerr)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pytest_multihost.transport.SSHCommand object at 0x7f09c0530c90>
raiseonerr = True
def wait(self, raiseonerr=True):
"""Wait for the remote process to exit
Raises an excption if the exit code is not 0, unless raiseonerr is
true.
"""
if self._done:
return self.returncode
self._end_process()
self._done = True
if raiseonerr and self.returncode:
self.log.error('Exit code: %s', self.returncode)
> raise subprocess.CalledProcessError(self.returncode, self.argv)
E CalledProcessError: Command '['kinit', 'admin']' returned non-zero exit status 1
Similiarly for other tests. This is caused by the fact that you did not
set topology in the BaseTestAdvise class, like this:
--- a/ipatests/test_integration/test_advise.py
+++ b/ipatests/test_integration/test_advise.py
@@ -31,6 +31,7 @@ class BaseTestAdvise(IntegrationTest, object):
advice_id = None
raiseerr = None
advice_regex = ''
+ topology = 'line'
2.) BaseTestAdvise inherits from IntegrationTest and from object.
Explicitly specifying object as superclass is not needed,
IntegrationTest already inherits from it.
3.) I think there is no good incentive to separate the test cases into
mutliple classes. Each test class adds overhead of installing and
uninstalling IPA server, to guarantee a clean and sane environment.
However, it seems to be an overkill for testing ipa-advise command,
which should be read-only anyway. By squashing the tests into one test
class, we will decrease the run time of this test more than 8-fold.
4.) The patch adds a whitespace error.
The test cases themselves are looking fine, and when I fixed the missing
topology, they all passed. So this is a question of fixing the above
issues, and we should be ready to push.
Tomas
On 02/17/2015 03:29 PM, Gabe Alford wrote:
> Hello,
>
> I was wondering if I could get a review of this patch.
>
> Thanks,
>
> Gabe
>
> On Thursday, January 29, 2015, Gabe Alford <redhatrises at gmail.com
> <mailto:redhatrises at gmail.com>> wrote:
>
> Hello,
>
> Here is a patch for
> https://fedorahosted.org/freeipa/ticket/4029 I added test cases
> for valid and invalid advice.
>
> Thanks,
>
> Gabe
>
> On Wed, Jan 14, 2015 at 10:23 AM, Tomas Babej <tbabej at redhat.com
> <javascript:_e(%7B%7D,'cvml','tbabej at redhat.com');>> wrote:
>
>
> On 01/14/2015 06:13 PM, Gabe Alford wrote:
>> On Wed, Jan 14, 2015 at 10:05 AM, Tomas Babej
>> <tbabej at redhat.com
>> <javascript:_e(%7B%7D,'cvml','tbabej at redhat.com');>> wrote:
>>
>>
>> On 01/14/2015 06:00 PM, Tomas Babej wrote:
>>>
>>> On 01/14/2015 05:37 PM, Tomas Babej wrote:
>>>>
>>>> On 01/14/2015 02:55 PM, Gabe Alford wrote:
>>>>> Hello,
>>>>>
>>>>> In looking into
>>>>> https://fedorahosted.org/freeipa/ticket/4029 I am
>>>>> wondering if there should be separate ipa-advise test,
>>>>> Yes/No? Could be handy in the future to test more
>>>>> ipa-advise output? Or should this test be added to the
>>>>> test_legacy_clients.py?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Gabe
>>>>>
>>>>> On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford
>>>>> <redhatrises at gmail.com
>>>>> <javascript:_e(%7B%7D,'cvml','redhatrises at gmail.com');>>
>>>>> wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> I was going to try my hand at attempting a patch
>>>>> for ipa-tests. However in wanting to test my
>>>>> patch, I am not sure how to run ipa-tests to check
>>>>> if it works or not. Documentation is not really
>>>>> clear on what needs to be done to start a test and
>>>>> run a test. This is for
>>>>> https://fedorahosted.org/freeipa/ticket/4029
>>>>>
>>>>> I have attached the patch that I have yet to
>>>>> really test with ipa-test. Any help on how to test
>>>>> the patch running ipa-tests would be great. Of
>>>>> course, if one of the reviewers looks at the patch
>>>>> and looks good, then I would be happy with that as
>>>>> well.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Gabe
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Freeipa-devel mailing list
>>>>> Freeipa-devel at redhat.com <javascript:_e(%7B%7D,'cvml','Freeipa-devel at redhat.com');>
>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>
>>>> Hello,
>>>>
>>>> TL;DR: feel free to create a separate ipa-advise test
>>>> file. Test requested in this ticket really does not
>>>> belong to the legacy clients feature test.
>>>>
>>>> As for the any new tests that might come: I think tests
>>>> for ipa-advise that are specific to that particular
>>>> feature should be tested with that feature, more so, if
>>>> they contain parts that are supposed to work
>>>> copy-pasted. If a tests, however, tests a general
>>>> behaviour of ipa-advise, it should live in the
>>>> ipa-advise namespace, hence separate test file.
>>>>
>>>> HTH,
>>>>
>>>> --
>>>> Tomas Babej
>>>> Associate Software Engineer | Red Hat | Identity Management
>>>> RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
>>>
>>> The attached patch looks fine, although, please also
>>> test for a non-zero return code number.
>>>
>>
>> Upon hitting send I noticed you did not include
>> raiseonerr=False into the run_command call. You need to
>> do that, otherwise a exception will be raised, since
>> ipa-advise exited with non-zero return code.
>>
>> Thanks Tomas.
>>
>> Which do you prefer: a test_advise.py or an update to the
>> existing patch?
>
> A new test file, as I pointed out in the second email :) sorry
> for splitting.
>
> However, it would be the best if you could spin up a positive
> test as well (maybe listing out available advices), not just
> this negative one, to justify the overhead reinstalling IPA
> for testing this feature.
>
>
>>>
>>> --
>>> Tomas Babej
>>> Associate Software Engineer | Red Hat | Identity Management
>>> RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
>>
>> --
>> Tomas Babej
>> Associate Software Engineer | Red Hat | Identity Management
>> RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
>>
>>
>
> --
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150224/d87a2658/attachment.htm>
More information about the Freeipa-devel
mailing list