[Piglit] [PATCH 2/2] framework/backends/json.py: preserve 'tests' order

[Dylan Baker]

Quoting Tomi Sarvela (2017-02-09 03:07:05)
> New helper function numericlistdir() arranges the os.listdir()
> in numerical order and deduplicates the code: reading in is done
> in two places, finalize and _resume.
> 
> Use OrderedDict when reading data in.
> ---
>  framework/backends/json.py | 42 +++++++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/framework/backends/json.py b/framework/backends/json.py
> index 174c0ca..6e67462 100644
> --- a/framework/backends/json.py
> +++ b/framework/backends/json.py
> @@ -29,6 +29,8 @@ import os
>  import posixpath
>  import shutil
>  import sys
> +import re

Please insert these alphabetically sorted.

> +from collections import OrderedDict

collections is already imported, please just use 'collections.OrderedDict'

>  
>  try:
>      import simplejson as json
> @@ -62,6 +64,16 @@ MINIMUM_SUPPORTED_VERSION = 7
>  INDENT = 4
>  
>  
> +def numericlistdir(path):

This name isn't very descriptive. how about: numeric_sorted_listdir?

> +    """Returns os.listdir with items sorted by their numerical value (if any)
> +
> +    Pass possible errors (FileNotFoundError, PermissionError) upwards
> +    """
> +    return sorted(os.listdir(path), key =

No spaces around operators in function signatures, please.

> +        lambda x: (-1, x) if not re.match(r'^\d+', x)
> +        else (int(re.findall(r'^\d+', x)[0]), x) )

This seems a little complex for what you're actually trying to do, I think this
does the same thing (but in less code):
lambda p: int(os.path.splitext(p)[0])

If that does work, for the two places it's used it's probably just easier to
call sorted inline.

> +
> +
>  def piglit_encoder(obj):
>      """ Encoder for piglit that can transform additional classes into json
>  
> @@ -125,12 +137,15 @@ class JSONBackend(FileBackend):
>          containers that are still open and closes the file
>  
>          """
> +        tests_dir = os.path.join(self._dest, 'tests')
> +        file_list = numericlistdir(tests_dir)
> +
>          # If jsonstreams is not present then build a complete tree of all of
>          # the data and write it with json.dump
>          if not _STREAMS:
>              # Create a dictionary that is full of data to be written to a
>              # single file
> -            data = collections.OrderedDict()
> +            data = OrderedDict()
>  
>              # Load the metadata and put it into a dictionary
>              with open(os.path.join(self._dest, 'metadata.json'), 'r') as f:
> @@ -141,11 +156,10 @@ class JSONBackend(FileBackend):
>                  data.update(metadata)
>  
>              # Add the tests to the dictionary
> -            data['tests'] = collections.OrderedDict()
> +            data['tests'] = OrderedDict()
>  
> -            test_dir = os.path.join(self._dest, 'tests')
> -            for test in os.listdir(test_dir):
> -                test = os.path.join(test_dir, test)
> +            for test in file_list:
> +                test = os.path.join(tests_dir, test)
>                  if os.path.isfile(test):
>                      # Try to open the json snippets. If we fail to open a test
>                      # then throw the whole thing out. This gives us atomic
> @@ -177,15 +191,14 @@ class JSONBackend(FileBackend):
>                      s.write('__type__', 'TestrunResult')
>                      with open(os.path.join(self._dest, 'metadata.json'),
>                                'r') as n:
> -                        s.iterwrite(six.iteritems(json.load(n)))
> +                        s.iterwrite(six.iteritems(json.load(n), object_pairs_hook=OrderedDict))

you've passed object_pairs_hook to the wrong function, it needs to be passed to
json.load

this specifically causes piglit with jsonstreams to fail. You can test it using
tox:
tox -e 'py{27,36}-streams'

>  
>                      if metadata:
>                          s.iterwrite(six.iteritems(metadata))
>  
> -                    test_dir = os.path.join(self._dest, 'tests')
>                      with s.subobject('tests') as t:
> -                        for test in os.listdir(test_dir):
> -                            test = os.path.join(test_dir, test)
> +                        for test in file_list:
> +                            test = os.path.join(tests_dir, test)
>                              if os.path.isfile(test):
>                                  try:
>                                      with open(test, 'r') as f:
> @@ -259,7 +272,7 @@ def _load(results_file):
>  
>      """
>      try:
> -        result = json.load(results_file)
> +        result = json.load(results_file, object_pairs_hook=OrderedDict)
>      except ValueError as e:
>          raise exceptions.PiglitFatalError(
>              'While loading json results file: "{}",\n'
> @@ -283,11 +296,14 @@ def _resume(results_dir):
>      assert meta['results_version'] == CURRENT_JSON_VERSION, \
>          "Old results version, resume impossible"
>  
> -    meta['tests'] = {}
> +    meta['tests'] = OrderedDict()
>  
>      # Load all of the test names and added them to the test list
> -    for file_ in os.listdir(os.path.join(results_dir, 'tests')):
> -        with open(os.path.join(results_dir, 'tests', file_), 'r') as f:
> +    tests_dir = os.path.join(results_dir, 'tests')
> +    file_list = numericlistdir(tests_dir)
> +
> +    for file_ in file_list:
> +        with open(os.path.join(tests_dir, file_), 'r') as f:
>              try:
>                  meta['tests'].update(json.load(f))
>              except ValueError:
> -- 
> 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/e0964b45/attachment.sig>