| Message ID | 20180912093306.23537-2-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATH,i-g-t,1/2] intel: Be consistent with test results on simulation | expand |
On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > igt_skip_on_simulation is called both directly from tests but also from > library helpers. In the latter case especially the logged caller name is > useless since it is always the helper itself. What we instead want to know > is who is the caller. > > Trivial approach would be to move the helper to a header as static inline, > but due the longjmp in it it can never be inlined. Alternative option is > to print a backtrace from it. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > --- > lib/igt_core.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 23bb858fd886..990abc5a36b3 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) > */ > void igt_skip_on_simulation(void) > { > + bool in_simulation; > + > if (igt_only_list_subtests()) > return; > > + in_simulation = igt_run_in_simulation(); > + > if (!igt_can_fail()) { > - igt_fixture > - igt_require(!igt_run_in_simulation()); > - } else > - igt_require(!igt_run_in_simulation()); > + igt_fixture { > + if (in_simulation) { > + print_backtrace(); > + igt_require(!in_simulation); > + } > + } > + } else { > + if (in_simulation) { > + print_backtrace(); > + igt_require(!in_simulation); Hm, why don't we go right ahead and push this into igt_skip()? There's a pile of other igt_require, and we tend to push a lot of these into the library. So they have all the same problem. -Daniel > + } > + } > } > > /* structured logging */ > -- > 2.17.1 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
On 14/09/2018 10:12, Daniel Vetter wrote: > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> igt_skip_on_simulation is called both directly from tests but also from >> library helpers. In the latter case especially the logged caller name is >> useless since it is always the helper itself. What we instead want to know >> is who is the caller. >> >> Trivial approach would be to move the helper to a header as static inline, >> but due the longjmp in it it can never be inlined. Alternative option is >> to print a backtrace from it. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> >> --- >> lib/igt_core.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/lib/igt_core.c b/lib/igt_core.c >> index 23bb858fd886..990abc5a36b3 100644 >> --- a/lib/igt_core.c >> +++ b/lib/igt_core.c >> @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) >> */ >> void igt_skip_on_simulation(void) >> { >> + bool in_simulation; >> + >> if (igt_only_list_subtests()) >> return; >> >> + in_simulation = igt_run_in_simulation(); >> + >> if (!igt_can_fail()) { >> - igt_fixture >> - igt_require(!igt_run_in_simulation()); >> - } else >> - igt_require(!igt_run_in_simulation()); >> + igt_fixture { >> + if (in_simulation) { >> + print_backtrace(); >> + igt_require(!in_simulation); >> + } >> + } >> + } else { >> + if (in_simulation) { >> + print_backtrace(); >> + igt_require(!in_simulation); > > Hm, why don't we go right ahead and push this into igt_skip()? There's a > pile of other igt_require, and we tend to push a lot of these into the > library. So they have all the same problem. Maybe.. I wasn't 100% this was a good idea to start with, or in another words, that other people would consider it a problem. Since the downside is test output gets more verbose on skips, or some could say more noisy. So I basically floated the patch to see if it will provoke some responses. :) Regards, Tvrtko
On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote: > > On 14/09/2018 10:12, Daniel Vetter wrote: > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > igt_skip_on_simulation is called both directly from tests but also from > > > library helpers. In the latter case especially the logged caller name is > > > useless since it is always the helper itself. What we instead want to know > > > is who is the caller. > > > > > > Trivial approach would be to move the helper to a header as static inline, > > > but due the longjmp in it it can never be inlined. Alternative option is > > > to print a backtrace from it. > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > > --- > > > lib/igt_core.c | 20 ++++++++++++++++---- > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > > index 23bb858fd886..990abc5a36b3 100644 > > > --- a/lib/igt_core.c > > > +++ b/lib/igt_core.c > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) > > > */ > > > void igt_skip_on_simulation(void) > > > { > > > + bool in_simulation; > > > + > > > if (igt_only_list_subtests()) > > > return; > > > + in_simulation = igt_run_in_simulation(); > > > + > > > if (!igt_can_fail()) { > > > - igt_fixture > > > - igt_require(!igt_run_in_simulation()); > > > - } else > > > - igt_require(!igt_run_in_simulation()); > > > + igt_fixture { > > > + if (in_simulation) { > > > + print_backtrace(); > > > + igt_require(!in_simulation); > > > + } > > > + } > > > + } else { > > > + if (in_simulation) { > > > + print_backtrace(); > > > + igt_require(!in_simulation); > > > > Hm, why don't we go right ahead and push this into igt_skip()? There's a > > pile of other igt_require, and we tend to push a lot of these into the > > library. So they have all the same problem. > > Maybe.. I wasn't 100% this was a good idea to start with, or in another > words, that other people would consider it a problem. Since the downside is > test output gets more verbose on skips, or some could say more noisy. > > So I basically floated the patch to see if it will provoke some responses. > :) We have the backtrace already in igt_fail, makes total sense to add it to igt_skip too. Has my ack at least. Aside: I kinda wonder whether we need an __igt_require, which embeds the if (igt_can_fail) igt_require() else igt_fixture igt_require() thing. But that's just an aside. Cheers, Daniel
Quoting Daniel Vetter (2018-09-14 10:46:25) > On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote: > > > > On 14/09/2018 10:12, Daniel Vetter wrote: > > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote: > > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > igt_skip_on_simulation is called both directly from tests but also from > > > > library helpers. In the latter case especially the logged caller name is > > > > useless since it is always the helper itself. What we instead want to know > > > > is who is the caller. > > > > > > > > Trivial approach would be to move the helper to a header as static inline, > > > > but due the longjmp in it it can never be inlined. Alternative option is > > > > to print a backtrace from it. > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > > > --- > > > > lib/igt_core.c | 20 ++++++++++++++++---- > > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > > > index 23bb858fd886..990abc5a36b3 100644 > > > > --- a/lib/igt_core.c > > > > +++ b/lib/igt_core.c > > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) > > > > */ > > > > void igt_skip_on_simulation(void) > > > > { > > > > + bool in_simulation; > > > > + > > > > if (igt_only_list_subtests()) > > > > return; > > > > + in_simulation = igt_run_in_simulation(); > > > > + > > > > if (!igt_can_fail()) { > > > > - igt_fixture > > > > - igt_require(!igt_run_in_simulation()); > > > > - } else > > > > - igt_require(!igt_run_in_simulation()); > > > > + igt_fixture { > > > > + if (in_simulation) { > > > > + print_backtrace(); > > > > + igt_require(!in_simulation); > > > > + } > > > > + } > > > > + } else { > > > > + if (in_simulation) { > > > > + print_backtrace(); > > > > + igt_require(!in_simulation); > > > > > > Hm, why don't we go right ahead and push this into igt_skip()? There's a > > > pile of other igt_require, and we tend to push a lot of these into the > > > library. So they have all the same problem. > > > > Maybe.. I wasn't 100% this was a good idea to start with, or in another > > words, that other people would consider it a problem. Since the downside is > > test output gets more verbose on skips, or some could say more noisy. > > > > So I basically floated the patch to see if it will provoke some responses. > > :) > > We have the backtrace already in igt_fail, makes total sense to add it to > igt_skip too. Has my ack at least. Skip is rather more frequent than fail, and more often than not, expected. So I am not entirely enjoying the prospect of a lot more noise, the requirement message was supposed to be sufficient to understand why we skipped. Maybe enforce that we have no igt_skip without a message? -Chris
On Fri, Sep 14, 2018 at 10:49:47AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2018-09-14 10:46:25) > > On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote: > > > > > > On 14/09/2018 10:12, Daniel Vetter wrote: > > > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote: > > > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > > > igt_skip_on_simulation is called both directly from tests but also from > > > > > library helpers. In the latter case especially the logged caller name is > > > > > useless since it is always the helper itself. What we instead want to know > > > > > is who is the caller. > > > > > > > > > > Trivial approach would be to move the helper to a header as static inline, > > > > > but due the longjmp in it it can never be inlined. Alternative option is > > > > > to print a backtrace from it. > > > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > > > > > --- > > > > > lib/igt_core.c | 20 ++++++++++++++++---- > > > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > > > > index 23bb858fd886..990abc5a36b3 100644 > > > > > --- a/lib/igt_core.c > > > > > +++ b/lib/igt_core.c > > > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) > > > > > */ > > > > > void igt_skip_on_simulation(void) > > > > > { > > > > > + bool in_simulation; > > > > > + > > > > > if (igt_only_list_subtests()) > > > > > return; > > > > > + in_simulation = igt_run_in_simulation(); > > > > > + > > > > > if (!igt_can_fail()) { > > > > > - igt_fixture > > > > > - igt_require(!igt_run_in_simulation()); > > > > > - } else > > > > > - igt_require(!igt_run_in_simulation()); > > > > > + igt_fixture { > > > > > + if (in_simulation) { > > > > > + print_backtrace(); > > > > > + igt_require(!in_simulation); > > > > > + } > > > > > + } > > > > > + } else { > > > > > + if (in_simulation) { > > > > > + print_backtrace(); > > > > > + igt_require(!in_simulation); > > > > > > > > Hm, why don't we go right ahead and push this into igt_skip()? There's a > > > > pile of other igt_require, and we tend to push a lot of these into the > > > > library. So they have all the same problem. > > > > > > Maybe.. I wasn't 100% this was a good idea to start with, or in another > > > words, that other people would consider it a problem. Since the downside is > > > test output gets more verbose on skips, or some could say more noisy. > > > > > > So I basically floated the patch to see if it will provoke some responses. > > > :) > > > > We have the backtrace already in igt_fail, makes total sense to add it to > > igt_skip too. Has my ack at least. > > Skip is rather more frequent than fail, and more often than not, > expected. So I am not entirely enjoying the prospect of a lot more noise, > the requirement message was supposed to be sufficient to understand why > we skipped. Maybe enforce that we have no igt_skip without a message? Hm yeah, adding a const char * to igt_skip_on_simulation that explains why we're skipping could be the pretty solution here. Would also serve as some self-documentation. -Daniel
diff --git a/lib/igt_core.c b/lib/igt_core.c index 23bb858fd886..990abc5a36b3 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void) */ void igt_skip_on_simulation(void) { + bool in_simulation; + if (igt_only_list_subtests()) return; + in_simulation = igt_run_in_simulation(); + if (!igt_can_fail()) { - igt_fixture - igt_require(!igt_run_in_simulation()); - } else - igt_require(!igt_run_in_simulation()); + igt_fixture { + if (in_simulation) { + print_backtrace(); + igt_require(!in_simulation); + } + } + } else { + if (in_simulation) { + print_backtrace(); + igt_require(!in_simulation); + } + } } /* structured logging */