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

[David Hildenbrand <david@redhat.com>]

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.

-- 

Thanks,

David / dhildenb