| Message ID | 1403878537-29020-3-git-send-email-tim.gore@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27 June 2014 15:15, <tim.gore@intel.com> wrote: > From: Tim Gore <tim.gore@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@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() */ This note needs to be added to the documentation comment for igt_simple_init so that it appears in the documentation. > + igt_subtest_init(argc, argv); > + test_with_subtests = false; > + if (list_subtests) > + igt_exit(); This should also exit with an error code if --run-subtest is used, since there are no subtests. > } > > /* > @@ -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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jun 27, 2014 at 03:15:37PM +0100, tim.gore@intel.com wrote: > From: Tim Gore <tim.gore@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@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 Also I prefer if the piglit changes come together with this patch so that we can roll it all out together. -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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Some comments on Daniels' comments inline > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Monday, July 07, 2014 5:10 PM > To: Gore, Tim > Cc: intel-gfx@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@intel.com wrote: > > From: Tim Gore <tim.gore@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@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@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
On Wed, Jul 09, 2014 at 01:23:46PM +0000, Gore, Tim wrote: > Some comments on Daniels' comments inline > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > 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. Well under the hood subtests work with longjmps when skipping/failing tests while simple tests directly call exit. The reason for that is that historically we've started with simple binaries and slowly grew the igt infrastructure on top of them. I don't really see much benefits in converting the last stragglers. > > 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. For me the only trouble with this disdinction is that people consistently place tests in the wrong Makefile target. Hence I'd like to see those two Makefile targets being unified (which requires adjustements in piglit, too) to validate this work. Iirc Thomas is also working on this. Cheers, Daniel
On Wed, Jul 09, 2014 at 04:23:50PM +0200, Daniel Vetter wrote: > For me the only trouble with this disdinction is that people consistently > place tests in the wrong Makefile target. Hence I'd like to see those two > Makefile targets being unified (which requires adjustements in piglit, > too) to validate this work. Iirc Thomas is also working on this. In all generalities, I think it makes things a lot simpler if we only one case to care about. In this instance: "A test is a set of one or more subtests" Seems a good invariant to me. Not sure I understand what the objections are.
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); + 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);