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

[Martin Babinsky]

On 12/09/2015 11:29 AM, Lenka Doudova wrote:
>
>
> On 12/09/2015 10:13 AM, Martin Basti wrote:
>>
>>
>> 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
>
> Great idea, thanks. Fixed patches attached.
>
> Lenka
>
>

Tests pass and code looks good, ACK.

-- 
Martin^3 Babinsky