[Intel-gfx] [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests

[Gore, Tim]

Some comments on Daniels' comments inline

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, July 07, 2014 5:10 PM
> To: Gore, Tim
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init
> for simple tests
> 
> On Fri, Jun 27, 2014 at 03:15:37PM +0100, tim.gore at intel.com wrote:
> > From: Tim Gore <tim.gore at intel.com>
> >
> > igt_subtest_init mainly does stuff that we also want for simple/single
> > tests, such as looking for --list-subtests and --help options and
> > calling common_init. So just call this from igt_simple_init and then
> > set tests_with_subtests to false. NOTE that this means that
> > check_igt_exit is now registered as an exit handler for single tests,
> > so need to make sure that ALL tests exit via igt_exit.
> >
> > Signed-off-by: Tim Gore <tim.gore at intel.com>
> > ---
> >  lib/igt_core.c         | 32 ++++++++++++++++----------------
> >  lib/igt_core.h         |  4 ++--
> >  tests/gem_gtt_hog.c    |  2 +-
> >  tests/igt_simulation.c |  4 ++--
> >  4 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c index 7ac7ebe..aaeaa3b
> > 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -458,13 +458,15 @@ void igt_subtest_init(int argc, char **argv)
> >   * #igt_simple_main block instead of stitching the tests's main() function
> together
> >   * manually.
> >   */
> > -void igt_simple_init(void)
> > +void igt_simple_init(int argc, char **argv)
> >  {
> > -	print_version();
> > -
> > -	oom_adjust_for_doom();
> > -
> > -	common_init();
> > +	/* Use the same init function as is used with subtests - we want most
> of its functionality */
> > +	/* Note that this will install the igt_exit_handler so you need to exit
> via igt_exit(),    */
> > +	/* Dont call exit()                                                                        */
> > +	igt_subtest_init(argc, argv);
> 
> Not sure whether forcing the reuse of igt_subtest_init makes a lot of sense
> since it requires a lot of follow-up changes all over. I'd just add a bit of
> argparsing here with the required special cases, i.e.
> - exit without doing anything for --list-subtests
> - exit with 77 when anything is specified with --run-subtests
> 
      What I wanted to do here was start removing the distinction between single
     Tests and multiple test; this distinction seems somewhat artificial and doesn't
      seem to add much. 
     igt_subtest_init does pretty much everything we want for single tests
     so I thought it made sense to re-use it. Perhaps the name should change,
     although this would lead to more knock on changes.
         Tim

> Also I prefer if the piglit changes come together with this patch so that we
> can roll it all out together.

    What piglet changes do you refer to? I have not made any piglit changes.
        Tim


> -Daniel
> 
> > +	test_with_subtests = false;
> > +	if (list_subtests)
> > +		igt_exit();
> >  }
> >
> >  /*
> > @@ -565,7 +567,7 @@ void igt_skip(const char *f, ...)
> >  		assert(in_fixture);
> >  		__igt_fixture_end();
> >  	} else {
> > -		exit(IGT_EXIT_SKIP);
> > +		igt_exit();
> >  	}
> >  }
> >
> > @@ -655,7 +657,7 @@ void igt_fail(int exitcode)
> >  			__igt_fixture_end();
> >  		}
> >
> > -		exit(exitcode);
> > +		igt_exit();
> >  	}
> >  }
> >
> > @@ -713,18 +715,16 @@ void igt_exit(void)
> >  	if (igt_only_list_subtests())
> >  		exit(IGT_EXIT_SUCCESS);
> >
> > -	if (!test_with_subtests)
> > -		exit(IGT_EXIT_SUCCESS);
> > -
> > -	/* Calling this without calling one of the above is a failure */
> > -	assert(skipped_one || succeeded_one || failed_one);
> > +	if (test_with_subtests)
> > +		/* Calling this without calling one of the above is a failure */
> > +		assert(skipped_one || succeeded_one || failed_one);
> >
> >  	if (failed_one)
> >  		exit(igt_exitcode);
> > -	else if (succeeded_one)
> > -		exit(IGT_EXIT_SUCCESS);
> > -	else
> > +	else if (skipped_one)
> >  		exit(IGT_EXIT_SKIP);
> > +	else
> > +		exit(IGT_EXIT_SUCCESS);
> >  }
> >
> >  /* fork support code */
> > diff --git a/lib/igt_core.h b/lib/igt_core.h index 9853e6b..cabbc3b
> > 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -166,7 +166,7 @@ bool igt_only_list_subtests(void);
> >   *
> >   * Init for simple tests without subtests
> >   */
> > -void igt_simple_init(void);
> > +void igt_simple_init(int argc, char **argv);
> >
> >  /**
> >   * igt_simple_main:
> > @@ -178,7 +178,7 @@ void igt_simple_init(void);  #define
> > igt_simple_main \
> >  	static void igt_tokencat(__real_main, __LINE__)(int argc, char
> **argv); \
> >  	int main(int argc, char **argv) { \
> > -		igt_simple_init(); \
> > +		igt_simple_init(argc, argv); \
> >  		igt_tokencat(__real_main, __LINE__)(argc, argv); \
> >  		igt_exit(); \
> >  	} \
> > diff --git a/tests/gem_gtt_hog.c b/tests/gem_gtt_hog.c index
> > 5d47540..f607ea0 100644
> > --- a/tests/gem_gtt_hog.c
> > +++ b/tests/gem_gtt_hog.c
> > @@ -150,7 +150,7 @@ static void run(data_t *data, int child)
> >  	munmap(ptr, size);
> >
> >  	igt_assert(x == canary);
> > -	exit(0);
> > +	_exit(0);
> >  }
> >
> >  igt_simple_main
> > diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c index
> > 15cbe64..3048c9e 100644
> > --- a/tests/igt_simulation.c
> > +++ b/tests/igt_simulation.c
> > @@ -53,11 +53,11 @@ static int do_fork(void)
> >  		assert(0);
> >  	case 0:
> >  		if (simple) {
> > -			igt_simple_init();
> > +			igt_simple_init(1, argv_run);
> >
> >  			igt_skip_on_simulation();
> >
> > -			exit(0);
> > +			igt_exit();
> >  		} else {
> >  			if (list_subtests)
> >  				igt_subtest_init(2, argv_list);
> > --
> > 1.9.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch