Open Stack

On 25 July 2016 at 13:56, Bhor, Dinesh <Dinesh.Bhor at nttdata.com> wrote:
>
>
> -----Original Message-----
> From: Sean Dague [mailto:sean at dague.net]
> Sent: Monday, July 25, 2016 5:53 PM
> To: openstack-dev at lists.openstack.org
> Subject: Re: [openstack-dev] [Nova] Remove duplicate code using Data Driven Tests (DDT)
>
> On 07/25/2016 08:05 AM, Daniel P. Berrange wrote:
>> On Mon, Jul 25, 2016 at 07:57:08AM -0400, Sean Dague wrote:
>>> On 07/22/2016 11:30 AM, Daniel P. Berrange wrote:
>>>> On Thu, Jul 21, 2016 at 07:03:53AM -0700, Matt Riedemann wrote:
>>>>> On 7/21/2016 2:03 AM, Bhor, Dinesh wrote:
>>>>>
>>>>> I agree that it's not a bug. I also agree that it helps in some
>>>>> specific types of tests which are doing some kind of input
>>>>> validation (like the patch you've proposed) or are simply iterating
>>>>> over some list of values (status values on a server instance for example).
>>>>>
>>>>> Using DDT in Nova has come up before and one of the concerns was
>>>>> hiding details in how the tests are run with a library, and if
>>>>> there would be a learning curve. Depending on the usage, I
>>>>> personally don't have a problem with it. When I used it in manila
>>>>> it took a little getting used to but I was basically just looking
>>>>> at existing tests and figuring out what they were doing when adding new ones.
>>>>
>>>> I don't think there's significant learning curve there - the way it
>>>> lets you annotate the test methods is pretty easy to understand and
>>>> the ddt docs spell it out clearly for newbies. We've far worse
>>>> things in our code that create a hard learning curve which people
>>>> will hit first :-)
>>>>
>>>> People have essentially been re-inventing ddt in nova tests already
>>>> by defining one helper method and them having multiple tests methods
>>>> all calling the same helper with a different dataset. So ddt is just
>>>> formalizing what we're already doing in many places, with less code
>>>> and greater clarity.
>>>>
>>>>> I definitely think DDT is easier to use/understand than something
>>>>> like testscenarios, which we're already using in Nova.
>>>>
>>>> Yeah, testscenarios feels little over-engineered for what we want
>>>> most of the time.
>>>
>>> Except, DDT is way less clear (and deterministic) about what's going
>>> on with the test name munging. Which means failures are harder to
>>> track back to individual tests and data load. So debugging the failures is harder.
>>
>> I'm not sure what you think is unclear - given an annotated test:
>>
>>    @ddt.data({"foo": "test", "availability_zone": "nova1"},
>>               {"name": "  test  ", "availability_zone": "nova1"},
>>               {"name": "", "availability_zone": "nova1"},
>>               {"name": "x" * 256, "availability_zone": "nova1"},
>>               {"name": "test", "availability_zone": "x" * 256},
>>               {"name": "test", "availability_zone": "  nova1  "},
>>               {"name": "test", "availability_zone": ""},
>>               {"name": "test", "availability_zone": "nova1", "foo": "bar"})
>>     def test_create_invalid_create_aggregate_data(self, value):
>>
>> It is generated one test for each data item:
>>
>>      test_create_invalid_create_aggregate_data_1
>>      test_create_invalid_create_aggregate_data_2
>>      test_create_invalid_create_aggregate_data_3
>>      test_create_invalid_create_aggregate_data_4
>>      test_create_invalid_create_aggregate_data_5
>>      test_create_invalid_create_aggregate_data_6
>>      test_create_invalid_create_aggregate_data_7
>>      test_create_invalid_create_aggregate_data_8
>>
>> This seems about as obvious as you can possibly get
>
> At least when this was attempted to be introduced into Tempest, the naming was a lot less clear, maybe it got better. But I still think milestone 3 isn't the time to start a thing like this.
>
>         -Sean
>
> Hi Sean,
>
> IMO it is possible to have a descriptive name to test cases using DDT.
>
> For ex.,
>
> @ddt.data(annotated('missing_name', {"foo": "test", "availability_zone": "nova1"}),
>           annotated('name_greater_than_255_characters', {"name": "x" * 256, "availability_zone": "nova1"}))
>     def test_create_invalid_aggregate_data(self, value):
>
>
> it generates following test names:
>
>         test_create_invalid_aggregate_data_2_name_greater_than_255_characters
>         test_create_invalid_aggregate_data_1_missing_name
>
> I think with this it is easy to identify which test scenario has failed.
>
> Same is implemented in openstack/zaqar.
> https://github.com/openstack/zaqar/blob/master/zaqar/tests/functional/wsgi/v1/test_queues.py#L87

That descriptive name does help with some of my fears (although I wish
it were simpler, maybe just using kwargs in data).

I am +1 sdague's not now. I know we normally allow unit test things,
but we have such a huge backlog, full of really useful changes we
should merge instead, I would rather we all looked at those.

Thanks,
johnthetubaguy