[Piglit] [PATCH] util: migrate the SUBTEST* macros to piglit-util.sh

[Jose Fonseca]

On 18/02/15 16:21, Martin Peres wrote:
> Hey Jose,
>
> On 18/02/15 17:10, Jose Fonseca wrote:
>> FYI, you'll need to rebase on top of
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__cgit.freedesktop.org_piglit_commit_-3Fid-3D83bc6862386b2d465879bcd372d61ec754534970&d=AwICaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=sVUvrnV7t1PNeIQxUMSOqpqrBVurla-YZBTVE6zrfak&s=Ab2Wxn8llpFWmmedbauWhqjyxSffQWMDqv8amVB9WcI&e=
>> , so that we use the standard C99 syntax for variadic macros.  You
>> might need to manually update the moved code, as git might just mark
>> it as a conflict.
>
> Sorry for breaking the windows build...  No probs, I'll rebase it.

No prob. Thanks.

>
>>
>>
>> Concerning the actual change, just a general remark:  these macros
>> save typing but they make the test code harder to understand
>>
>>
>> I think there are other approaches that achieve the same amount of
>> type-saving, without compromising readbility.
>>
>> For example, we could have a
>>
>>
>>   // global contaning the current substest result
>>   // TODO: Make it a enum piglit_result
>>   bool current_subtest_result;
>>   char current_subtest_message[4096];
>>
>>   piglit_subtest_begin() {
>>     current_subtest_result = true;
>>     current_subtest_message[0] = 0;
>>   }
>>
>>   void
>>   piglit_subtest_check(bool condition, format, ...) {
>>     bool pass = piglit_check_gl_error((error));
>>     if (!pass) {
>>         // FIXME: append format+va_args to current_subtest_message.
>>     }
>>     current_subtest_result = current_subtest_result && pass;
>>   }
>>
>>   void
>>   piglit_subtest_check_gl_error(error) {
>>      ...
>>   }
>>
>>  piglit_subtest_end()
>>    piglit_report_subtest_result(current_subtest_result ? PIGLIT_PASS :
>> PIGLIT_FAIL, current_subtest_message);
>>   }
>>
>>
>> And the test would do
>>
>>     piglit_subtest_begin();
>>     piglit_subtest_check_gl_error(error, etc);
>>     piglit_subtest_check_gl_error(error, etc);
>>     piglit_subtest_check(cond, etc);
>>     piglit_subtest_end();
>
> Not sure this form saves anything since one sub-test would be one line
> in my case instead of 3 in this case.
> Have a look at
> https://urldefense.proofpoint.com/v2/url?u=http-3A__cgit.freedesktop.org_-7Emperes_piglit_commit_-3Fh-3Ddsa-26id-3Dc08558659f7967410ab68f39c3d768e41e35bc6d&d=AwICaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=sVUvrnV7t1PNeIQxUMSOqpqrBVurla-YZBTVE6zrfak&s=eN7ni-8KESSon5ovNFgLvaqSxs_IHB9Wp20IEwtz5YU&e=

Right. I did misunderstood how this was supposed to be used.  I saw 
piglit_report_subtest_result callers that had many partial sub-conditions.

But I now see that there's the test pass/fail, and then the subtest 
pass/fail, and each can be aggregated of multiple sub-conditions, and 
test writers usually need to do "foo = foo && condition" at both levels. 
  You macros target the case where each subtest is one condition/error 
exactly.


> What you proposed is a good idea for the whole test though.
> piglit_test_begin() would set current_test_result
> to keep track of the state and the other functions would follow the
> state.

Right.


 > I'm not a fan of the global variable
> too as what will happen if we have interleaved tests?

True.  But maybe the benefit of not having to keep track of overall 
pass/fail would trump over the inability to write interleaved tests.

> In any case, I'm sure some tests will not work with this model. So,
> instead of sharing those function and making them
> very confusing, I would argue that outside of trivial cases (like the
> macros I proposed), you should re-write your own
> way of checking. As for the "confusing" argument, not sure how much
> simpler can this code be (*no side effect*). It is
> also less prone to copy/paste errors.
>
> In my opinion, we won't reach an agreement that would work in every case
> or, if we do, people will not use the code
> added because it will be too complex. Instead, let's just let devs
> handle their test code as it pleases them. If there are
> some trivial cases common-enough, then we can add them to
> piglit-util.sh. I guess the question now is, are those
> common-enough outside of DSA?

Devs are certainly free to stick with their style.  And I'm not active 
enough in piglit to oppose anything.


So I just wondered if it wouldn't be worthwhile to devise some scheme 
that would completely eliminate the need for test writers to keep track 
of these aggregated pass/fail variables.

Jose