Merge lp:~roadmr/checkbox/cdts-report-2-exporter-non-boolean-options-xml-client-name-option into lp:checkbox

[Daniel Manrique]

 plainbox:exporter:xml: Add client-name option. Add support to exporters for non-boolean options using "=".

Adds support for non-boolean options in the option-string for exporters. This is leveraged to add the client-name setting to the xml exporter, which in turn results in this information showing in the HTML report. I did this in three small steps:

16f8fa4 plainbox:exporter:xml: Add client-name option.
763b4ab plainbox:exporter: Add support for non-boolean options using "=".
1606e15 plainbox:exporter: Change _option_list into a property.

Other than the test suite, this can be verified manually quite easily. Set up a working venv, then run:

plainbox run -w plainbox-provider-checkbox/provider_whitelists/hwsubmit.whitelist -f html -p client-name=CUSTOM_CLIENT_NAME -o /tmp/test.html

Even if no tests are run (due to running from source) the html report should show "This report was created using CUSTOM_CLIENT_NAME 0.6.0" at the top.

[lp:~roadmr/checkbox/cdts-report-2-exporter-non-boolean-options-xml-client-name-option]

2864. By Daniel Manrique on 2014-04-01

plainbox:exporter: Change _option_list into a property.

Added tests to ensure current behavior is maintained

2865. By Daniel Manrique on 2014-04-01

plainbox:exporter: Add support for non-boolean options using "=".

The old "API" of directly querying and setting an exporter's
_option_list is maintained for backwards compatibility.

Options that have "=" are parsed into option=value by the constructor.

New methods to query those values, as well as set an option to either
just True or a string value, were added.

2866. By Daniel Manrique on 2014-04-01

plainbox:exporter:xml: Add client-name option.

A non-boolean option (client-name=something) meant to pass the client
name to put in the report to the xml exporter. If set, this will take
precedence over the client_name parameter to the constructor.

Tests were updated accordingly.

[Zygmunt Krynicki (zyga)]

+1

review: Approve

=== modified file 'plainbox/plainbox/impl/commands/test_run.py'
--- plainbox/plainbox/impl/commands/test_run.py	2014-02-20 21:43:54 +0000
+++ plainbox/plainbox/impl/commands/test_run.py	2014-04-01 23:36:47 +0000
@@ -158,7 +158,7 @@
         json: with-io-log, squash-io-log, flatten-io-log, with-run-list, with-job-list, with-resource-map, with-job-defs, with-attachments, with-comments, with-job-via, with-job-hash, machine-json
         rfc822: with-io-log, squash-io-log, flatten-io-log, with-run-list, with-job-list, with-resource-map, with-job-defs, with-attachments, with-comments, with-job-via, with-job-hash
         text: with-io-log, squash-io-log, flatten-io-log, with-run-list, with-job-list, with-resource-map, with-job-defs, with-attachments, with-comments, with-job-via, with-job-hash
-        xml: 
+        xml: client-name
         """
         self.assertEqual(io.combined, cleandoc(expected) + "\n")
 

=== modified file 'plainbox/plainbox/impl/exporter/__init__.py'
--- plainbox/plainbox/impl/exporter/__init__.py	2014-03-26 19:36:44 +0000
+++ plainbox/plainbox/impl/exporter/__init__.py	2014-04-01 23:36:47 +0000
@@ -61,9 +61,14 @@
     preserve everything that the session may hold but instead to present it to
     the user in the best format possible.
 
-    Each exporter can support a set of options (currently boolean flags) that
-    can alter the way it operates. It's best to keep the list of exporter
-    options under control to keep the user interface from becoming annoying.
+    Each exporter can support a set of options that can alter the way it
+    operates. Options can either be set boolean-like, or they can be assigned a
+    value (a string). If an option contains a "=", the part of the string on
+    the right of the equal sign will be assigned as the option's value;
+    otherwise they operate in a boolean fashion.
+
+    It's best to keep the list of exporter options under control to keep the
+    user interface from becoming annoying.
     """
 
     OPTION_WITH_IO_LOG = 'with-io-log'
@@ -96,10 +101,52 @@
     def __init__(self, option_list=None):
         if option_list is None:
             option_list = []
-        for option in option_list:
+        self._option_dict = {}
+        for option_string in option_list:
+            # An option can look like "foo" or like "foo=bar". In the first case
+            # we assign "True" to that key in the dict, in the second we assign "bar".
+            key_value = option_string.split("=", 1)
+            option = key_value[0]
             if option not in self.supported_option_list:
                 raise ValueError("Unsupported option: {}".format(option))
-        self._option_list = option_list
+            if len(key_value) == 2:
+                value = key_value[1]
+            else:
+                value = True
+            self._option_dict[option] = value
+
+    @property
+    def _option_list(self):
+        """
+        Returns a list of set options.
+        Users who only are about whether an option is set, regardless of
+        the value assigned to it, can use this API.
+        """
+        return sorted([option for option in self._option_dict.keys()
+                if self._option_dict[option]])
+
+    @_option_list.setter
+    def _option_list(self, value):
+        """
+        Sets the option list to exactly what is sent as the parameter.
+        Note that this will obliterate any prior settings in the list.
+        """
+        self._option_dict = {}
+        for key in value:
+            self._option_dict[key] = True
+
+    def get_option_value(self, option):
+        """
+        Returns the value assigned to an option.
+        """
+        return self._option_dict.get(option, False)
+
+    def set_option_value(self, option, value=True):
+        """
+        assigns a value to an option. If no value is given, it just
+        "sets" the option to True
+        """
+        self._option_dict[option] = value
 
     @classproperty
     def supported_option_list(cls):

=== modified file 'plainbox/plainbox/impl/exporter/test_init.py'
--- plainbox/plainbox/impl/exporter/test_init.py	2013-11-27 20:40:19 +0000
+++ plainbox/plainbox/impl/exporter/test_init.py	2014-04-01 23:36:47 +0000
@@ -83,6 +83,46 @@
         session.update_job_result(job_b, result_b)
         return session
 
+    def test_option_list_setting_boolean(self):
+        exporter = self.TestSessionStateExporter()
+        exporter._option_list = [
+            SessionStateExporterBase.OPTION_WITH_IO_LOG,
+            SessionStateExporterBase.OPTION_FLATTEN_IO_LOG]
+        self.assertEqual(exporter._option_list, sorted([
+            SessionStateExporterBase.OPTION_WITH_IO_LOG,
+            SessionStateExporterBase.OPTION_FLATTEN_IO_LOG]))
+
+    def test_option_list_setting_boolean_all_at_once(self):
+        # Test every option set, all at once
+        # Just to be paranoid, ensure the options I set are the ones the
+        # exporter actually thinks it has
+        exporter = self.TestSessionStateExporter(
+            self.TestSessionStateExporter.supported_option_list)
+        self.assertEqual(exporter._option_list,
+                sorted(self.TestSessionStateExporter.supported_option_list))
+
+    def test_option_list_init_non_boolean(self):
+        option = SessionStateExporterBase.OPTION_WITH_COMMENTS
+        exporter = self.TestSessionStateExporter(["{}=detailed".format(option)])
+        self.assertEqual(exporter.get_option_value(option),
+                "detailed")
+
+    def test_option_list_non_duplicated_options(self):
+        # Setting the same option twice makes no sense, check it gets squashed
+        # into only one item in the option_list.
+        option = SessionStateExporterBase.OPTION_WITH_COMMENTS
+        exporter = self.TestSessionStateExporter([option, option])
+        self.assertEqual(exporter._option_list, [option])
+
+    def test_option_list_setting_api(self):
+        exporter = self.TestSessionStateExporter(
+                [SessionStateExporterBase.OPTION_WITH_IO_LOG])
+        exporter.set_option_value("with-comments")
+        self.assertEqual(exporter.get_option_value('with-comments'), True)
+        exporter.set_option_value("with-comments", "detailed")
+        self.assertEqual(exporter.get_option_value('with-comments'),
+                "detailed")
+
     def test_defaults(self):
         # Test all defaults, with all options unset
         exporter = self.TestSessionStateExporter()

=== modified file 'plainbox/plainbox/impl/exporter/test_xml.py'
--- plainbox/plainbox/impl/exporter/test_xml.py	2013-11-27 20:40:19 +0000
+++ plainbox/plainbox/impl/exporter/test_xml.py	2014-04-01 23:36:47 +0000
@@ -87,18 +87,35 @@
 class XMLExporterTests(TestCase):
 
     def setUp(self):
+        self.prepare_test_exporter()
+
+    def prepare_test_exporter(self, client_name="plainbox",
+                              system_id="",
+                              option_list=None,
+                              timestamp="2012-12-21T12:00:00",
+                              client_version="1.0"):
         data = resource_json(
             "plainbox", "test-data/xml-exporter/example-data.json",
             exact=True)
-        exporter = XMLSessionStateExporter(
-            system_id="",
-            timestamp="2012-12-21T12:00:00",
-            client_version="1.0")
+        self.exporter = XMLSessionStateExporter(
+            client_name=client_name,
+            option_list=option_list,
+            system_id=system_id,
+            timestamp=timestamp,
+            client_version=client_version)
         stream = io.BytesIO()
-        exporter.dump(data, stream)
+        self.exporter.dump(data, stream)
         self.actual_result = stream.getvalue()  # This is bytes
         self.assertIsInstance(self.actual_result, bytes)
 
+    def test_exporter_option(self):
+        """
+        Ensure that the previously-optionless xml exporter can have its
+        single accepted 'client-name' option set properly.
+        """
+        self.prepare_test_exporter(option_list=['client-name=verifythis'])
+        self.assertEqual(self.exporter.get_option_value('client-name'), "verifythis")
+
     def test_perfect_match(self):
         expected_result = resource_string(
             "plainbox", "test-data/xml-exporter/example-data.xml"
@@ -113,3 +130,17 @@
         self.assertTrue(
             validator.validate_text(
                 self.actual_result))
+
+    def test_client_name_option_takes_precedence(self):
+        # We use trickery to verify the xml final report has the client name
+        # sent in the option string, rather than the constructor parameter.
+        # We pass a bogus client-name in the constructor, then the correct expected
+        # name in the option, and just check as usual.
+        self.prepare_test_exporter(client_name="bogus",
+                                   option_list=['client-name=plainbox'])
+        expected_result = resource_string(
+            "plainbox", "test-data/xml-exporter/example-data.xml"
+        )  # unintuitively, resource_string returns bytes
+        self.assertEqual(self.actual_result, expected_result)
+
+

=== modified file 'plainbox/plainbox/impl/exporter/xml.py'
--- plainbox/plainbox/impl/exporter/xml.py	2014-04-01 15:30:07 +0000
+++ plainbox/plainbox/impl/exporter/xml.py	2014-04-01 23:36:47 +0000
@@ -98,7 +98,8 @@
 
     NS = '2013.com.canonical.certification::'
 
-    SUPPORTED_OPTION_LIST = ()
+    OPTION_CLIENT_NAME = 'client-name'
+    SUPPORTED_OPTION_LIST = (OPTION_CLIENT_NAME, )
 
     # These are the job statuses allowed by the checkbox parser.
     # This is a limitation of the certification website, so we
@@ -148,16 +149,18 @@
             The name of the exporter to report. Defaults to "plainbox".
         """
         # Super-call with empty option list
-        super(XMLSessionStateExporter, self).__init__(())
+        super(XMLSessionStateExporter, self).__init__((option_list))
         # All the "options" are simply a required configuration element and are
         # not optional in any way. There is no way to opt-out.
-        self._option_list = (
-            SessionStateExporterBase.OPTION_WITH_IO_LOG,
-            SessionStateExporterBase.OPTION_FLATTEN_IO_LOG,
-            SessionStateExporterBase.OPTION_WITH_JOB_DEFS,
-            SessionStateExporterBase.OPTION_WITH_RESOURCE_MAP,
-            SessionStateExporterBase.OPTION_WITH_COMMENTS,
-            SessionStateExporterBase.OPTION_WITH_ATTACHMENTS)
+        xml_options = (SessionStateExporterBase.OPTION_WITH_IO_LOG,
+                       SessionStateExporterBase.OPTION_FLATTEN_IO_LOG,
+                       SessionStateExporterBase.OPTION_WITH_JOB_DEFS,
+                       SessionStateExporterBase.OPTION_WITH_RESOURCE_MAP,
+                       SessionStateExporterBase.OPTION_WITH_COMMENTS,
+                       SessionStateExporterBase.OPTION_WITH_ATTACHMENTS)
+        for option in xml_options:
+                self.set_option_value(option)
+
         # Generate a dummy system hash if needed
         if system_id is None:
             # FIXME: Compute an real system_id for submission to
@@ -174,6 +177,9 @@
         self._client_version = client_version
         # Remember client name
         self._client_name = client_name
+        # If a client name was specified as an option, prefer that.
+        if self.get_option_value('client-name'):
+            self._client_name = self.get_option_value('client-name')
 
     def dump(self, data, stream):
         """