[Piglit] [PATCH 1/1] results.py: Print warning when overwriting subtest results

[Ilia Mirkin]

On Wed, Sep 3, 2014 at 8:56 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> On Wed, 2014-09-03 at 19:33 -0400, Ilia Mirkin wrote:
>> On Wed, Sep 3, 2014 at 7:16 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>> > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
>> > ---
>> >
>> > There is none reported for quick.py profile, and tons for cl.py
>> >
>> >  framework/results.py | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/framework/results.py b/framework/results.py
>> > index ddadcc1..7d3a375 100644
>> > --- a/framework/results.py
>> > +++ b/framework/results.py
>> > @@ -465,15 +465,17 @@ class TestResult(dict):
>> >          dictionary -- a dictionary instance to update the TestResult with
>> >
>> >          """
>> > -        def update(d, u):
>> > +        def update(d, u, check):
>> >              for k, v in u.iteritems():
>> >                  if isinstance(v, dict):
>> > -                    d[k] = update(d.get(k, {}), v)
>> > +                    d[k] = update(d.get(k, {}), v, True)
>> >                  else:
>> > +                   if check and d.has_key(k):
>> > +                       print("Warning: duplicate subtest: {} value: {} old value: {}".format(k, v, d[k]))
>>
>> It would appear that you are mixing tabs and spaces. Don't do that,
>> python can get mad at you. (Or gmail is messing up, in which case, my
>> bad.)
>>
>> Also, "k in d" is preferable to "d.has_key(k)".
>
> thanks, I'll fix these
>
>>
>> At a quick glance I don't understand the purpose of "check". Why can't
>> it always just be true (and thus removed)?
>
> that was my original implementation, but something sets the 'result' key
> (probably constructor), so it would always report replacing it.

Hmmmm.... right. In the Test constructor:

        self.result = TestResult({'result': 'fail'})

And in case that wasn't there, right before the
self.interpret_result() call in Test.run():

        self.result['result'] = 'fail'

(which is actually a little wrong because it needs to be one of those
status things, but nothing cares at exec time). It does seem like
that's unnecessary... Dylan? Seems like that could just be replaced
with

self.interpret_result()
if 'result' not in self.result:
  self.result['result'] = status.FAIL

>
>>
>> Printing stuff right there goes against the logging logic... I think
>> TestResults are also used during test execution, not just at summary
>> time... Dylan may have ideas on how to expose the issue properly.
>
> These need to be detected at execution time otherwise only the last
> result is stored in the results file and we loose information (and hide
> possible failures).
> if there is a better way to report these, I don;t mind changing it.
>
> thanks,
> jan
>
>>
>> >                      d[k] = v
>> >              return d
>> >
>> > -        update(self, dictionary)
>> > +        update(self, dictionary, False)
>> >
>> >
>> >  class TestrunResult(object):
>> > --
>> > 1.9.3
>> >
>> > _______________________________________________
>> > Piglit mailing list
>> > Piglit at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/piglit
>
> --
> Jan Vesely <jan.vesely at rutgers.edu>