[Piglit] [PATCH] util: keep track of failures in subtests

[Martin Peres]

On 04/03/15 20:38, Dylan Baker wrote:
> On Wed, Mar 04, 2015 at 10:00:03AM +0200, Martin Peres wrote:
>> On 03/03/15 20:32, Dylan Baker wrote:
>>> On Tue, Mar 03, 2015 at 03:12:51PM +0200, Martin Peres wrote:
>>>> On 03/03/15 14:55, Jose Fonseca wrote:
>>>>> On 26/02/15 16:06, Martin Peres wrote:
>>>>>> ---
>>>>>>     tests/util/piglit-util.c | 17 +++++++++++++++++
>>>>>>     1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/util/piglit-util.c b/tests/util/piglit-util.c
>>>>>> index b094625..b5bd89f 100644
>>>>>> --- a/tests/util/piglit-util.c
>>>>>> +++ b/tests/util/piglit-util.c
>>>>>> @@ -273,6 +273,20 @@ piglit_set_timeout(double seconds, enum
>>>>>> piglit_result timeout_result)
>>>>>>     #endif
>>>>>>     }
>>>>>>
>>>>>> +static bool _piglit_subtests_all_passed = true;
>>>>>> +
>>>>>> +bool piglit_subtests_all_passed()
>>>>>> +{
>>>>>> +    return _piglit_subtests_all_passed;
>>>>>> +}
>>>>>> +
>>>>>> +bool piglit_subtests_all_passed_reset()
>>>>>> +{
>>>>>> +    bool ret = _piglit_subtests_all_passed;
>>>>>> +    _piglit_subtests_all_passed = true;
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>>     void
>>>>>>     piglit_report_subtest_result(enum piglit_result result, const char
>>>>>> *format, ...)
>>>>>>     {
>>>>>> @@ -287,6 +301,9 @@ piglit_report_subtest_result(enum piglit_result
>>>>>> result, const char *format, ...)
>>>>>>         fflush(stdout);
>>>>>>
>>>>>>         va_end(ap);
>>>>>> +
>>>>>> +    if (result == PIGLIT_FAIL)
>>>>>> +        _piglit_subtests_all_passed = false;
>>>>>>     }
>>>>>>
>>>>>>     #ifdef _WIN32
>>>>>>
>>>>> Sorry for the delay.  This looks good overall, though it might be
>>>>> better to change the interface to use enum piglit_result instead of
>>>>> bool, so that it eventually it handles WARP/SKIP better (eg., if all
>>>>> subtests SKIP, then report SKIP).
>>>> No probs for the delay. That is indeed a good proposition.
>>>>> BTW, is it really true that if one subtest fails, the overall test
>>>>> should be false?  For example, when I was looking at
>>>>> iglit_report_subtest_result callers, I noticed that
>>>>> tests/spec/amd_performance_monitor/api.c will not report false
>>>>> overall, even though may report true.
>>>> That is a matter of conventions I guess but to me it does not make sense
>>>> to say that a test passed if some parts of it failed. Has anyone a test
>>>> in mind where it would make sense? Worst case, we could make a function
>>>> that does not update the global state.
>>>>
>>> I agree with this, and it is what piglit currently does. Right now the
>>> framework disregards the overall status of the test and return the
>>> 'worst' subtest status as the overall test status.
>> Really? That sounds great! How can I use this behaviour?
> Should be done automatically by the summary generator. I'm not sure if
> that's being done in the raw result files, but it certainly could be
> changed easily enough
Yeah, it is not done in the raw result files. I guess it would make sense
to do the work there and not in the summary generator.