[Piglit] [PATCH] texwrap: do no short circuit remaining tests if one fails

[Ian Romanick]

On 11/23/2015 03:07 PM, Emil Velikov wrote:
> On 23 November 2015 at 20:48, Ian Romanick <idr at freedesktop.org> wrote:
>> On 11/22/2015 03:43 AM, Emil Velikov wrote:
>>> On 11 November 2015 at 18:07, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>
>>>> Noticed as some of these have been intermittently failing on llvmpipe,
>>>> resulting in a few "not run" test across mesa release checks.
>>>>
>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>> ---
>>>>
>>>> XXX:
>>>> At some point we'd want to do a tree-wide:
>>>>  - s/GLboolean pass/bool pass/
>>>>  - s/pass = pass && foo/pass &= foo/
>>>>  - s/pass = foo && pass/pass &= foo/
>>
>> Yes, please... but see below.
>>
>>>> We might want to convert the test to use the piglit_probe_pixels over
>>>> it's custom ones.
>>>>
>>>> -Emil
>>>>
>>>>  tests/texturing/texwrap.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/texturing/texwrap.c b/tests/texturing/texwrap.c
>>>> index fbe9068..60ffa73 100644
>>>> --- a/tests/texturing/texwrap.c
>>>> +++ b/tests/texturing/texwrap.c
>>>> @@ -1134,7 +1134,7 @@ static GLboolean test_format_npot(const struct format_desc *format, GLboolean np
>>>>                  * It has to be enabled on the command line.
>>>>                  */
>>>>                 if (!texture_swizzle && !npot && !test_border_color && has_texture_swizzle) {
>>>> -                       pass = pass && test_format_npot_swizzle(format, npot, 1);
>>>> +                       pass = test_format_npot_swizzle(format, npot, 1) && pass;
>>>>                 }
>>>>         }
>>>>         return pass;
>>>> @@ -1149,7 +1149,7 @@ static GLboolean test_format(const struct format_desc *format)
>>>>         } else {
>>>>                 pass = test_format_npot(format, 0);
>>>>                 if (has_npot && !test_border_color) {
>>>> -                       pass = pass && test_format_npot(format, 1);
>>>> +                       pass = test_format_npot(format, 1) && pass;
>>>>                 }
>>>>         }
>>>>         return pass;
>>>> @@ -1163,7 +1163,7 @@ enum piglit_result piglit_display()
>>>>                 pass = test_format(init_format ? init_format : &test->format[0]);
>>>>         } else {
>>>>                 if (init_format) {
>>>> -                       pass = pass && test_format(init_format);
>>>> +                       pass = test_format(init_format) && pass;
>>>>                 } else {
>>>>                         int i;
>>>>                         for (i = 0; i < test->num_formats; i++) {
>>>> --
>>> Any takers on this trivial patch ? I guess we can bikeshed the "pass =
>>
>> I don't think we should use the term bikeshed.  While it didn't start
>> that way, it has become a purely pejorative term used to dismiss
>> someone's feedback.  It's only purpose these days seems to be to make
>> people mad and start arguments.
>>
> "bikeshedding" wasn't used to belittle or stray anyone's input or
> review about the patch. It was aimed to distinguish between the patch
> from the comment after the --- line. As the former is (imho) dead
> obvious, while the latter than provide an lengthy discussion.

I didn't think you were... that's why I said "we" instead of "you." :)
You did right by keeping the separate concerns separate.

>>> foo && pass" vs "pass &= foo" at a later stage.
>>
>> The problem with &= is that it doesn't extend to more than two
>> predicates.  As a result, there will always be place in piglit that do
>>
> Admittedly I haven't looked that closely into piglits, but I have yet
> to see a case like that.
> Perhaps we can just add that is a big must/no-go and crack on ? We
> might even want to check in-tree a coccinelle script to handle such
> cases ?

As Ilia pointed out (about the short circuiting), there may not be any.
 I remember that when Eric, Ken, and I started doing it this way, we
collectively had some good reasons.  It was 5+ years ago, and I guess I
don't actually remember what they were.  I know we considered both &=
and &&.

>>     pass = foo &&
>>            bar &&
>>            asdf &&
>>            pass;
>>
>> We don't want to have two different idioms for essentially the same
>> thing.  That means that test authors and reviews have to stop and think
>> about which idiom should be used in which places.  It also means that if
>> a place that used &= is extended with another predicate, you have to
>> change more of the code. So,
>>
>>     pass &= foo;
>>
>> would become
>>
>>     pass = foo &&
>>            bar &&
>>            pass;
>>
>> And everyone has to review it more carefully to be sure it's right.
>>
>> Moreover, "pass = foo && pass;" has been the piglit idiom for *years*.
>> A few new-comers came along and started using the other idiom because
>> they had used it on other projects.  Piglit's slack review requirement
>> allowed a bunch of that to slip in.
>>
> True, some of that can be attributed to slack review. Although there
> was the argument "I saw this as the new format/used in mesa" which
> albeit adequate, combined with people being busy on other things
> doesn't make things any easier.
> 
>> I really don't want to see it spread any further, and I'd be happy to
>> review patches that change the existing uses of &=.
>>
> Fwiw I'm in no position to enforce either option, I'm just pointing
> out that the issues in current patch, are not &= ones.
> 
> Perhaps regardless of the approach we choose, we should write some
> coccinelle (other?) scripts to ensure/enforce things. Sort of like the
> kernel's check-patch - always run your patches through it and fix the
> issues before sending upstream. Perhaps we can also use check-patch
> with a few piglit specific tweaks ?
> 
> Shame Coccinelle skills are non existent :-\
> 
> -Emil