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
_______________________________________________
Piglit mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/piglit
