[Intel-gfx] [PATCH] lib: allow igt_skip_on_simulation outside of fixtures.

[Daniel Vetter]

On Sat, Mar 22, 2014 at 10:14:41AM -0700, Ben Widawsky wrote:
> On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote:
> > Thomas noticed that in simulation mode a lot of the tests fall over
> > instead of skipping properly. This is due to recently added
> > self-checks which ensure that any call to igt_skip happens either
> > within a fixture or subtest block (or it's a simple test without
> > subtests). This is to catch bugs since pretty much always not wrapping
> > up hardware setup and checks into these blocks is a bug.
> > 
> > Bug simulation skipping is a bit different, so allow that exception.
>   But
> 
> > Otherwise we'd need to fix up piles of tests (and likely need to play
> > a game of whack-a-mole).
> > 
> > Also add a library testcase for all the different variants to make
> > sure it really works.
> > 
> > Cc: Thomas Wood <thomas.wood at intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> You do realize you've just written a test for your test suite? Yo dawg,
> I heard you like tests....

You do realize that this is about number 6 in my set of tests for my test
suite?

I think the proper conclusion is simply that at least occasionally I
should hand in my "C programmer" lincense due to accute incompetence ;-)

~/xorg/intel-gpu-tools$ ls tests/igt_*c
tests/igt_fork_helper.c  tests/igt_no_exit.c            tests/igt_no_subtest.c
tests/igt_list_only.c    tests/igt_no_exit_list_only.c  tests/igt_simulation.c

Fyi you can run the with

$ make check

which is always recommended when you touch some of the library code.

> I honestly don't know enough about the internals here to do a proper
> review in a minute, but sounds fine from the commit msg. Just one
> comment below and it's:
> Acked-by: Ben Widawsky <ben at bwidawsk.net>

Already pushed the patch ...

> > +int main(int argc, char **argv)
> > +{
> > +	/* simple tests */
> > +	simple = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	/* subtests, list mode */
> > +	simple = false;
> > +	list_subtests = true;
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	/* subtest, run mode */
> > +	simple = false;
> > +	list_subtests = false;
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> 
> You could have saved some setenvs here and above by doing all the "trues",
> then "falses." If you want to keep the same pattern though, it probably
> makes sense to dump this into a helper
> 
> test(bool fixture, char *sim_env, int retval);
> 
> OR
> 	in_fixture = false;
> 	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> 	assert(do_fork() == 77);
> 	in_fixture = true;
> 	assert(do_fork() == 77);
> 
> 	in_fixture = false;
> 	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> 	assert(do_fork() == 0);
> 	in_fixture = true;
> 	assert(do_fork() == 0);
> 
> 
> you can even get down to just 2 setenvs by permuting in_fixture, simple, and
> list_subtests.

Yeah, it's not pretty. If I'd go totally ocd I'd do a for_boolean macro
for simple, list_subtests and in_fixture and for the simulation flag. But
my thinking here was that I want to explicitly list all the case, since
it's no totally obvious which should return 77 and which 0. So I've left
it at the very verbose but explicit version above.

After all this is a testcase for a now fairly established (and documented)
piece of igt infrastructure, so I don't think anyone needs to look at this
any time soon (i.e. hopefully not in the next few years).

Also, alread pushed ;-)

Thanks for the comments anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch