[Freeipa-devel] [TESTS][PATCH 0006] Add comments to stageuser plugin tests

[Martin Basti]

On 09.12.2015 09:41, Lenka Doudova wrote:
> Hi,
>
> attaching fixed patches for master and ipa-4-2 branch.
> Also fixes test accordingly to 
> https://fedorahosted.org/freeipa/ticket/5387.
>
> Lenka
>
> On 11/20/2015 12:13 PM, Martin Babinsky wrote:
>> On 11/19/2015 10:34 AM, Petr Viktorin wrote:
>>> On 11/19/2015 09:30 AM, Lenka Doudova wrote:
>>>> On 11/18/2015 04:51 PM, Martin Babinsky wrote:
>>>>> On 11/18/2015 02:16 PM, Lenka Doudova wrote:
>>>>>> Hi,
>>>>>>
>>>>>> here's a patch that adds a few comments to stageuser tests in 
>>>>>> order to
>>>>>> allow easier determining of a problem when tests fail.
>>>>>>
>>>>>> Lenka
>>>>>>
>>>>>>
>>>>>
>>>>> Hi Lenka,
>>>>>
>>>>> Firstly a technical detail: Python indexes lists from 0, so the
>>>>> comments in 'options_ok' do not correctly map to the test names 
>>>>> anyway.
>>>>>
>>>>> I am also not sure if this patch is worth reviewing and pushing as it
>>>>> IMHO doesn't help in the identification of failed tests at all.
>>>>>
>>>>> This should be solved at more fundamental level.
>>>>>
>>>> Ouch, good point, I didn't realize. Sorry.
>>>>
>>>> Anyway, the issue is that even if tests are run in verbose mode, 
>>>> you get
>>>> output like this:
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser27] 
>>>>
>>>> PASSED
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser28] 
>>>>
>>>> PASSED
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser29] 
>>>>
>>>> PASSED
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser210] 
>>>>
>>>> PASSED
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser211] 
>>>>
>>>> PASSED
>>>>
>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser212] 
>>>>
>>>> PASSED
>>>>
>>>>
>>>> If some test fails, you can't really tell which command was the one
>>>> responsible for the fail. This should be solved by
>>>> https://fedorahosted.org/freeipa/ticket/5449. Until that's done, 
>>>> though,
>>>> the only way to find out which command failed is looking at the 
>>>> code and
>>>> finding which parameters were put into the command, which was not much
>>>> possible without better commenting the test case (as I realized last
>>>> week when David Kupka asked me to help him find the reason for failed
>>>> tests).
>>>> Obviously I can rewrite the tests so there's 27 separate test 
>>>> cases, one
>>>> for each parameter, instead of one method that 'unwraps' into 27 test
>>>> cases, which would entirely eliminate the confusion. So far I don't 
>>>> know
>>>> of a way to put 27 similar test cases in one method which would allow
>>>> easy recognition of the test cases.
>>>> I'll wait with fixing the patch until further discussion.
>>>
>>> Hello,
>>> Pytest wants you to be a bit more explicit about how to name the
>>> parameters. (It "hides" dicts by default, because large dicts would 
>>> make
>>> the output even more confusing than the numbers.)
>>>
>>> Please try the attached patch.
>>> Docs are at 
>>> https://pytest.org/latest/fixture.html#parametrizing-a-fixture
>>>
>>>
>>>
>> This looks like a better approach IMHO, you can then see which 
>> attribute/value was being checked.
>>
>> I would very much favor more descriptive test/fixture names in the 
>> beginning.
>>
>
>
>
Hello,

we use usually bottom posting on freeipa-devel please try to keep reply 
in this way.

Patch:

I do not like the idea of separated lists, IMO it is hard to manage and 
is easy to create mistakes.

How about this (untested, just from top of my head): 
http://fpaste.org/298994/65184014/

Martin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151209/7374f54b/attachment.htm>