[Piglit] [PATCH 1/2] framework/results.py: Copy 'tests' internally to OrderedDict

[Dylan Baker]

Quoting Dylan Baker (2017-02-09 08:34:47)
> First, thanks for doing this it's a nice improvement.
> 
> I have a few comments below, and a few on the next patch as well.
> 
> Quoting Tomi Sarvela (2017-02-09 03:07:04)
> > To preserve the original test running order, use OrderedDict
> > when copying 'tests' values over.
> > ---
> >  framework/results.py | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/framework/results.py b/framework/results.py
> > index 1a73c11..45a5727 100644
> > --- a/framework/results.py
> > +++ b/framework/results.py
> > @@ -24,7 +24,9 @@
> >  from __future__ import (
> >      absolute_import, division, print_function, unicode_literals
> >  )
> > -import collections
> 
> First, this is an unrelated change since we already have collections imported,
> and creates a lot of churn in the patch. Second, I don't think this is an
> improvement, since it puts more things in the root namespace which are not local
> functions or frequently used.
> 
> > +from collections import defaultdict
> > +from collections import OrderedDict
> > +from collections import MutableMapping
> >  import copy
> >  import datetime
> >  
> > @@ -38,7 +40,7 @@ __all__ = [
> >  ]
> >  
> >  
> > -class Subtests(collections.MutableMapping):
> > +class Subtests(MutableMapping):
> >      """A dict-like object that stores Statuses as values."""
> >      def __init__(self, dict_=None):
> >          self.__container = {}
> > @@ -301,8 +303,8 @@ class TestrunResult(object):
> >          self.clinfo = None
> >          self.lspci = None
> >          self.time_elapsed = TimeAttribute()
> > -        self.tests = collections.OrderedDict()
> > -        self.totals = collections.defaultdict(Totals)
> > +        self.tests = OrderedDict()
> > +        self.totals = defaultdict(Totals)
> >  
> >      def get_result(self, key):
> >          """Get the result of a test or subtest.
> > @@ -350,7 +352,7 @@ class TestrunResult(object):
> >          if not self.totals:
> >              self.calculate_group_totals()
> >          rep = copy.copy(self.__dict__)
> > -        rep['tests'] = {k: t.to_json() for k, t in six.iteritems(self.tests)}
> > +        rep['tests'] = OrderedDict([ ( k, t.to_json() ) for k, t in six.iteritems(self.tests)] )
> 
> This is not an "OrderedDict comprehension", what this actually does is create a
> list in memeory, and then copy that into an OrderedDict. It should be sufficient
> to do:
> OrderedDict(k, t.to_json() for k, t in six.iteritems(self.tests))

I should probably explain why this is better. Without the '[]' around the
comprehension this becomes a "generator expression", which produces new values
as it is iterated over. The advantage is that only one concrete instance is built,
the OrderedDict.

> Please also note that we use PEP8 style in the python portion of the code, there
> are no spaces around braces or parens.
> 
> >          rep['__type__'] = 'TestrunResult'
> >          return rep
> >  
> > @@ -378,8 +380,8 @@ class TestrunResult(object):
> >          if 'time_elapsed' in dict_:
> >              setattr(res, 'time_elapsed',
> >                      TimeAttribute.from_dict(dict_['time_elapsed']))
> > -        res.tests = {n: TestResult.from_dict(t)
> > -                     for n, t in six.iteritems(dict_['tests'])}
> > +        res.tests = OrderedDict([ (n, TestResult.from_dict(t))
> > +                     for n, t in six.iteritems(dict_['tests']) ])
> 
> The same here as above
> 
> >  
> >          if not 'totals' in dict_ and not _no_totals:
> >              res.calculate_group_totals()
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/piglit
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20170209/b51cdb0c/attachment.sig>