Re: [PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 3/23/21 11:44 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>>> Member name 'u' and names starting with 'has-' or 'has_' are reserved
>>>> for the generator.  check_type() enforces this, covered by tests
>>>> reserved-member-u and reserved-member-has.
>>>> These tests neglect to cover optional members, where the name starts
>>>> with '*'.  Tweak reserved-member-u to fix that.
>>>> This demonstrates the reserved member name check is broken for
>>>> optional members.  The next commit will fix it.
>>>>
>>>
>>> The test without an optional member goes away. Do we lose coverage?
>>> (Do we care?)
>> Up to a point :)  We do try to cover all failure modes, just not in
>> all
>> contexts.
>> The test is about this error:
>>           if c_name(key, False) == 'u' or c_name(key,
>> False).startswith('has_'):
>>               raise QAPISemError(info, "%s uses reserved name" % key_source)
>> Full matrix: (is "u", starts with "has_") x (optional, not
>> optional).
>> Instead of covering all four cases, we cover two: non-optional "u"
>> (reserved-member-u) and non-optional "has-" (reserved-member-has).
>> The patch flips the former to optional.  The latter still covers
>> non-optional.
>> Good enough, I think.
>> 
>
> Relies a tiny bit on knowing these two reserved name checks are
> implemented in the same place. Doubt it'll matter
> practically. Coverage has increased overall.
>
>> Do you feel I should point to reserved-member-has in the commit message?
>> 
>
> It'd be for my benefit, but you also already just explained it to me.

Amending the second paragraph:

    These tests neglect to cover optional members, where the name starts
    with '*'.  Tweak reserved-member-u to fix that.  Test
    reserved-member-has still covers non-optional members.


> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!