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

[Martin Basti]

On 09.12.2015 12:40, Martin Babinsky wrote:
> 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.
>
Pushed to master: a66a2c5160dbc23cdeec55d17422812028939e16
Pushed to ipa-4-2: 75675fc71a148dcc17b025a80123aa01644f5297