[Piglit] [Patch v7 11/13] framework: consolidate boilerplate in JSNOWriter class

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 7:38 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> This moves all of the code for writing the name, the options, etc out of
> run and resume and into a single method of JSONWriter. This should
> reduce errors, code duplication, and help abstract a lot of problems
> with future changes to the json away.
>
> v7: - add close_json() to JSONWriter
> TODO: add tests for close_json()
>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
>  framework/programs/run.py | 43 +++++++++---------------------------
>  framework/results.py      | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 33 deletions(-)
>
> diff --git a/framework/programs/run.py b/framework/programs/run.py
> index 15b2f53..4a500bd 100644
> --- a/framework/programs/run.py
> +++ b/framework/programs/run.py
> @@ -161,28 +161,18 @@ def run(input_):
>      result_filepath = path.join(args.results_path, 'main')
>      result_file = open(result_filepath, 'w')
>      json_writer = framework.results.JSONWriter(result_file)
> -    json_writer.open_dict()
>
> -    # Write out command line options for use in resuming.
> -    json_writer.write_dict_key('options')
> -    json_writer.open_dict()
> -    json_writer.write_dict_item('profile', args.test_profile)
> -    for key, value in opts:
> -        json_writer.write_dict_item(key, value)
> +    # Create a dictionary to pass to initialize json, it needs the contents of
> +    # the env dictionary and profile and platform information
> +    options = {'profile': args.test_profile}
>      if args.platform:
> -        json_writer.write_dict_item('platform', args.platform)
> -    json_writer.close_dict()
> -
> -    json_writer.write_dict_item('name', results.name)
> -
> -    for key, value in core.collect_system_info().iteritems():
> -        json_writer.write_dict_item(key, value)
> +        options['platform'] = args.platform
> +    json_writer.initialize_json(options, results.name,
> +                                core.collect_system_info())
>
>      profile = framework.profile.merge_test_profiles(args.test_profile)
>      profile.results_dir = args.results_path
>
> -    json_writer.write_dict_key('tests')
> -    json_writer.open_dict()
>      time_start = time.time()
>      # Set the dmesg type
>      if args.dmesg:
> @@ -196,8 +186,7 @@ def run(input_):
>      json_writer.write_dict_item('time_elapsed', results.time_elapsed)
>
>      # End json.
> -    json_writer.close_dict()
> -    json_writer.file.close()
> +    json_writer.close_json()
>
>      print('Thank you for running Piglit!\n'
>            'Results have been written to ' + result_filepath)
> @@ -225,19 +214,9 @@ def resume(input_):
>
>      results_path = path.join(args.results_path, "main")
>      json_writer = framework.results.JSONWriter(open(results_path, 'w+'))
> -    json_writer.open_dict()
> -    json_writer.write_dict_key("options")
> -    json_writer.open_dict()
> -    for key, value in results.options.iteritems():
> -        json_writer.write_dict_item(key, value)
> -    json_writer.close_dict()
> -
> -    json_writer.write_dict_item('name', results.name)
> -    for key, value in core.collect_system_info().iteritems():
> -        json_writer.write_dict_item(key, value)
> +    json_writer.initialize_json(results.options, results.name,
> +                                core.collect_system_info())
>
> -    json_writer.write_dict_key('tests')
> -    json_writer.open_dict()
>      for key, value in results.tests.iteritems():
>          json_writer.write_dict_item(key, value)
>          opts.exclude_tests.add(key)
> @@ -250,9 +229,7 @@ def resume(input_):
>      # This is resumed, don't bother with time since it wont be accurate anyway
>      profile.run(opts, json_writer)
>
> -    json_writer.close_dict()
> -    json_writer.close_dict()
> -    json_writer.file.close()
> +    json_writer.close_json()
>
>      print("Thank you for running Piglit!\n"
>            "Results have ben wrriten to {0}".format(results_path))
> diff --git a/framework/results.py b/framework/results.py
> index 2321a89..5f0316a 100644
> --- a/framework/results.py
> +++ b/framework/results.py
> @@ -118,6 +118,59 @@ class JSONWriter(object):
>          #
>          self.__is_collection_empty = []
>
> +        # self._open_containers
> +        #
> +        # A FILO stack that stores container information, each time
> +        # self.open_dict() 'dict' is added to the stack, (other elements like
> +        # 'list' could be added if support was added to JSONWriter for handling
> +        # them), each to time self.close_dict() is called an element is
> +        # removed. When self.close_json() is called each element of the stack
> +        # is popped and written into the json
> +        self._open_containers = []
> +
> +    def initialize_json(self, options, name, env):
> +        """ Write boilerplate json code
> +
> +        This writes all of the json except the actual tests.
> +
> +        Arguments:
> +        options -- any values to be put in the options dictionary, must be a
> +                   dict-like object
> +        name -- the name of the test
> +        env -- any environment information to be written into the results, must
> +               be a dict-like object
> +
> +        """
> +        self.open_dict()
> +        self.write_dict_item('name', name)
> +
> +        self.write_dict_key('options')
> +        self.open_dict()
> +        for key, value in options.iteritems():
> +            # Loading a NoneType will break resume, and are a bug
> +            assert value is not None, "Value {} is NoneType".format(key)
> +            self.write_dict_item(key, value)
> +        self.close_dict()
> +
> +        for key, value in env.iteritems():
> +            self.write_dict_item(key, value)
> +
> +        self.write_dict_key('tests')
> +        self.open_dict()
> +
> +    def close_json(self):
> +        """ End json serialization and cleanup
> +
> +        This method is called after all of tests are written, it closes any
> +        containers that are still open and closes the file
> +
> +        """
> +        for elem in reversed(self._open_containers):
> +            if elem == 'dict':
> +                self.close_dict()

I had thought something more like

self.close_dict()
self.close_dict()
assert len(self._open_containers) == 0

Which makes sure that everything between initialize_json and
close_json closes itself. Perhaps you can just use the indent level
though.

> +
> +        self.file.close()
> +
>      @synchronized_self
>      def __write_indent(self):
>          if self.__inhibit_next_indent:
> @@ -144,6 +197,7 @@ class JSONWriter(object):
>
>          self.__indent_level += 1
>          self.__is_collection_empty.append(True)
> +        self._open_containers.append('dict')
>
>      @synchronized_self
>      def close_dict(self):
> @@ -153,6 +207,8 @@ class JSONWriter(object):
>          self.file.write('\n')
>          self.__write_indent()
>          self.file.write('}')
> +        assert self._open_containers[-1] == 'dict'
> +        self._open_containers.pop()
>
>      @synchronized_self
>      def write_dict_item(self, key, value):
> --
> 2.0.0
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit