[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Gabe Alford]

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> 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 listFreeipa-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
>>>
>>>
>>>  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
>>>
>>>
>>> --
>>> Tomas Babej
>>> Associate Software Engineer | Red Hat | Identity Management
>>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>>
>>>
>>
>> --
>> Tomas Babej
>> Associate Software Engineer | Red Hat | Identity Management
>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150217/9e7f6b2f/attachment.htm>