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

[Martin Peres]

Hey Jose,

On 18/02/15 17:10, Jose Fonseca wrote:
> FYI, you'll need to rebase on top of 
> http://cgit.freedesktop.org/piglit/commit/?id=83bc6862386b2d465879bcd372d61ec754534970 
> , 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.

>
>
> 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 
http://cgit.freedesktop.org/~mperes/piglit/commit/?h=dsa&id=c08558659f7967410ab68f39c3d768e41e35bc6d

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. I'm not a fan of the global variable
too as what will happen if we have 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?

Martin