Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[David Hildenbrand <david@redhat.com>]

On 04.11.19 15:24, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 14:47:54 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.11.19 13:06, Claudio Imbrenda wrote:
>>> On Mon, 4 Nov 2019 12:55:48 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>    
>>>> On 04.11.19 12:49, Claudio Imbrenda wrote:
>>>>> On Mon, 4 Nov 2019 12:31:32 +0100
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>       
>>>>>> On 04.11.19 12:29, Claudio Imbrenda wrote:
>>>>>>> On Mon, 4 Nov 2019 11:58:20 +0100
>>>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>          
>>>>>>>> Can we just please rename all "cx" into something like "len"?
>>>>>>>> Or is there a real need to have "cx" in there?
>>>>>>>
>>>>>>> if cx is such a nuisance to you, sure, I can rename it to i
>>>>>>
>>>>>> better than random characters :)
>>>>>
>>>>> will be in v3
>>>>>       
>>>>>>>          
>>>>>>>> Also, I still dislike "test_one_sccb". Can't we just just do
>>>>>>>> something like
>>>>>>>>
>>>>>>>> expect_pgm_int();
>>>>>>>> rc = test_one_sccb(...)
>>>>>>>> report("whatever pgm", rc == WHATEVER);
>>>>>>>> report("whatever rc", lc->pgm_int_code == WHATEVER);
>>>>>>>>
>>>>>>>> In the callers to make these tests readable and cleanup
>>>>>>>> test_one_sccb(). I don't care if that produces more LOC as long
>>>>>>>> as I can actually read and understand the test cases.
>>>>>>>
>>>>>>> if you think that makes it more readable, ok I guess...
>>>>>>>
>>>>>>> consider that the output will be unreadable, though
>>>>>>>          
>>>>>>
>>>>>> I think his will turn out more readable.
>>>>>
>>>>> two output lines per SCLP call? I  don't think so
>>>>
>>>> To clarify, we don't always need two checks. E.g., I would like to
>>>> see instead of
>>>>
>>>> +static void test_sccb_too_short(void)
>>>> +{
>>>> +	int cx;
>>>> +
>>>> +	for (cx = 0; cx < 8; cx++)
>>>> +		if (!test_one_run(valid_code, pagebuf, cx, 8,
>>>> PGM_BIT_SPEC, 0))
>>>> +			break;
>>>> +
>>>> +	report("SCCB too short", cx == 8);
>>>> +}
>>>>
>>>> Something like
>>>>
>>>> static void test_sccb_too_short(void)
>>>> {
>>>> 	int i;
>>>>
>>>> 	for (i = 0; i < 8; i++) {
>>>> 		expect_pgm_int();
>>>> 		test_one_sccb(...); // or however that will be
>>>> called check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>>> 	}
>>>> }
>>>>
>>>> If possible.
>>>>   
>>>
>>> so, thousands of output lines for the whole test, ok
>>>    
>>
>> A couple of things to note
>>
>> a) You perform 8 checks, so report the result of 8 checks
>> b) We really don't care about the number of lines in a log file as
>> long as we can roughly identify what went wrong (e.g., push/pop a
>> prefix here) c) We really *don't* need full coverage here. The same
>> applies to other tests. Simply testing against the boundary
>> conditions is good enough.
>>
>>
>> expect_pgm_int();
>> test_one_sccb(..., 0, ...);
>> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>
>> expect_pgm_int();
>> test_one_sccb(..., 7, ...);
>> check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>
>> Just as we handle it in other tests.
> 
> the fact that the other test are not as extensive as they should be
> doesn't mean this test should cover less.

All I'm saying is that you might test too much and I am not sure if that 
is really needed everywhere in this patch. But I'll leave that up to you.

-- 

Thanks,

David / dhildenb