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

[Emil Velikov]

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.

>> 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 ?

>     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