Merge lp:~javier.collado/utah/battery_measurements_config into lp:utah

[Javier Collado]

This branch adds a new boolean field `battery_measurements` that can be added
to the master runlist. When this field is set to `true` battery measurements
are taken if battery information is available. When this field is `false`,
which is the default value, no information will be gathered at all.

The initial design was to make it possible to specify `battery_measurements`
also in the test case control file. However, I think the additional complexity
needed to handle this wasn't really paying off. Anyway, if you think otherwise,
please let me know.

I'd like to add some documentation strings to the code that has been added
and/or updated, but if could let review the code as it is right now, it would
be great. Thanks.

[Andy Doan (doanac)]

102 + kkkk"""

I you must be a VIM user :)

[lp:~javier.collado/utah/battery_measurements_config]

832. By Javier Collado on 2013-02-12

Removed vim side effect

[Javier Collado (javier.collado)]

@Andy

Indeed, I am. Thanks for pointing that out.

[Max Brustkern (nuclearbob)]

Now that line 93 exists, is line 90 still needed?

The rest of it looks good to me, but I haven't tested it yet.

[lp:~javier.collado/utah/battery_measurements_config]

833. By Javier Collado on 2013-02-15

Removed unused code

[Javier Collado (javier.collado)]

@Max

Fixed, I believe that was never really needed since it was set by the subclass
as an instance variable as it happens after these changes.

review: Needs Resubmitting

[Max Brustkern (nuclearbob)]

Cool. I'll try to get my laptop fired up today to test it, but if you've already tested it, it's probably good to go in.

[Joe Talbott (joetalbott)]

Looks good to me.

review: Approve

[Max Brustkern (nuclearbob)]

This looks good to me.

review: Approve

=== modified file 'utah/client/common.py'
--- utah/client/common.py	2013-02-06 14:16:20 +0000
+++ utah/client/common.py	2013-02-15 08:57:19 +0000
@@ -83,7 +83,8 @@
 
 # Inspired by:
 # http://stackoverflow.com/a/3326559
-def run_cmd(command, cwd=None, timeout=0, cmd_type=CMD_TC_TEST, run_as=None):
+def run_cmd(command, cwd=None, timeout=0, cmd_type=CMD_TC_TEST, run_as=None,
+            battery_measurements=False):
 
     if run_as is not None:
         # add -n so sudo will not prompt for a password on the tty
@@ -103,7 +104,9 @@
     def alarm_handler(_signum, _frame):
         raise TimeoutAlarm
 
-    start_battery = battery.get_data()
+    if battery_measurements:
+        start_battery = battery.get_data()
+
     start_time = datetime.datetime.now()
     p = subprocess.Popen("{} {}".format(cmd_prefix, command),
                          shell=True, cwd=cwd,
@@ -144,19 +147,23 @@
                            )
 
     time_delta = datetime.datetime.now() - start_time
-    end_battery = battery.get_data()
-
-    return make_result(command=command,
-                       retcode=p.returncode,
-                       stdout=stdout,
-                       stderr=stderr,
-                       start_time=start_time.strftime(DATE_FORMAT),
-                       time_delta=str(time_delta),
-                       cmd_type=cmd_type,
-                       user=run_as,
-                       start_battery=start_battery,
-                       end_battery=end_battery,
-                       )
+
+    kwargs = {'command': command,
+              'retcode': p.returncode,
+              'stdout': stdout,
+              'stderr': stderr,
+              'start_time': start_time.strftime(DATE_FORMAT),
+              'time_delta': str(time_delta),
+              'cmd_type': cmd_type,
+              'user': run_as,
+              }
+
+    if battery_measurements:
+        end_battery = battery.get_data()
+        kwargs['start_battery'] = start_battery
+        kwargs['end_battery'] = end_battery
+
+    return make_result(**kwargs)
 
 
 def normalize_encoding(value, encoding='utf-8'):
@@ -237,12 +244,24 @@
     return data
 
 
+# Validator that sets values to defaults as explained in:
+# https://github.com/Julian/jsonschema/issues/4
+class DefaultValidator(jsonschema.Validator):
+    def validate_properties(self, properties, instance, schema):
+        (super(DefaultValidator, self)
+         .validate_properties(properties, instance, schema))
+        instance.update((k, v['default'])
+                        for k, v in properties.iteritems()
+                        if k not in instance and 'default' in v)
+
+
 def parse_control_file(filename, schema):
     """
     Parse a control file and check against the schema
     """
     control_data = parse_yaml_file(filename)
-    jsonschema.validate(control_data, schema)
+    validator = DefaultValidator()
+    validator.validate(control_data, schema)
     return control_data
 
 
@@ -453,19 +472,29 @@
     """
     Base class for Version Ccontrol System support.
     """
-    repo = ""
+
+    def __init__(self, repo, destination='', battery_measurements=False):
+        self.repo = repo
+        self.destination = destination
+        self.battery_measurements = battery_measurements
 
     def get(self, directory=REPO_DEFAULT_DIR):
         """
         Method to retrieve a copy of the repository
         """
-        return run_cmd(self.get_command, cwd=directory, cmd_type=CMD_TS_FETCH)
+        return run_cmd(self.get_command,
+                       cwd=directory,
+                       cmd_type=CMD_TS_FETCH,
+                       battery_measurements=self.battery_measurements)
 
     def revision(self, directory=REPO_DEFAULT_DIR):
         """
         Return the revision identifier.
         """
-        return run_cmd(self.rev_command, cwd=directory, cmd_type=CMD_TS_FETCH)
+        return run_cmd(self.rev_command,
+                       cwd=directory,
+                       cmd_type=CMD_TS_FETCH,
+                       battery_measurements=self.battery_measurements)
 
     def get_revision(self, directory=REPO_DEFAULT_DIR):
         """
@@ -487,23 +516,23 @@
     bazaar handler.
     """
 
-    def __init__(self, repo, branch=True, options="", destination=""):
-        self.repo = repo
+    def __init__(self, repo, branch=True, options="", destination="",
+                 **kwargs):
+        super(BzrHandler, self).__init__(repo, destination, **kwargs)
         self.options = options
-        self.destination = destination
 
         if branch:
             self.get_command = "bzr branch {} {} {}".format(
                 options,
                 self.repo,
-                destination,
+                self.destination,
             )
 
             self.rev_command = "bzr revno"
         else:
             self.get_command = "bzr export {} {} {}".format(
                 options,
-                destination,
+                self.destination,
                 self.repo,
             )
 
@@ -515,15 +544,14 @@
     git handler.
     """
 
-    def __init__(self, repo, options="", destination=""):
-        self.repo = repo
+    def __init__(self, repo, options="", destination="", **kwargs):
+        super(GitHandler, self).__init__(repo, destination, **kwargs)
         self.options = options
-        self.destination = destination
 
         self.get_command = "git clone {} {} {}".format(
             options,
             self.repo,
-            destination,
+            self.destination,
         )
 
         self.rev_command = "git rev-parse HEAD"
@@ -532,13 +560,12 @@
 class DevHandler(VCSHandler):
     """ Copy from a local directory for development. """
 
-    def __init__(self, repo, destination="."):
-        self.repo = repo
-        self.destination = destination
+    def __init__(self, repo, destination=".", **kwargs):
+        super(DevHandler, self).__init__(repo, destination, **kwargs)
 
         self.get_command = "cp -r {} {}".format(
             self.repo,
-            destination,
+            self.destination,
         )
 
         self.rev_command = "echo 'DEVELOPMENT'"

=== modified file 'utah/client/runner.py'
--- utah/client/runner.py	2013-02-13 19:36:40 +0000
+++ utah/client/runner.py	2013-02-15 08:57:19 +0000
@@ -13,7 +13,10 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from utah.client.common import DEFAULT_TSLIST
+from utah.client.common import (
+    DEFAULT_TSLIST,
+    DefaultValidator,
+)
 from utah.client.result import Result
 from utah.client.testsuite import TestSuite
 from utah.client.state_agent import StateAgentYAML
@@ -130,6 +133,10 @@
                 "type": [MASTER_RUNLIST_SCHEMA_ORIG],
                 "required": True,
             },
+            'battery_measurements': {
+                'type': 'boolean',
+                'default': False,
+            }
         }
     }
 
@@ -256,7 +263,8 @@
         """
         Run the test suites we've parsed.
         """
-        self.result.start_battery = battery.get_data()
+        if self.battery_measurements:
+            self.result.start_battery = battery.get_data()
 
         # Return value to indicate whether processing of a Runner should
         # continue.  This is to avoid a shutdown race on reboot cases.
@@ -279,7 +287,8 @@
         self.status = "DONE"
         self.save_state()
 
-        self.result.end_battery = battery.get_data()
+        if self.battery_measurements:
+            self.result.end_battery = battery.get_data()
         return self.process_results()
 
     def add_suite(self, suite):
@@ -378,8 +387,9 @@
             raise exceptions.MissingFile(runlist)
 
         data = parse_yaml_file(local_filename)
+        validator = DefaultValidator()
         try:
-            jsonschema.validate(data, self.MASTER_RUNLIST_SCHEMA)
+            validator.validate(data, self.MASTER_RUNLIST_SCHEMA)
         except jsonschema.ValidationError as exception:
             raise exceptions.ValidationError(
                 'Master runlist failed to validate: {!r}\n'
@@ -398,6 +408,8 @@
         if 'publish' in data:
             self.result.publish = data['publish']
 
+        self.battery_measurements = data['battery_measurements']
+
         seen = []
 
         orig_dir = os.getcwd()
@@ -421,15 +433,20 @@
             fetch_location = suite['fetch_location']
 
             name = suite['name']
+            kwargs = {'repo': fetch_location,
+                      'destination': name,
+                      'battery_measurements': self.battery_measurements,
+                      }
             if fetch_method == 'bzr':
-                vcs_handler = BzrHandler(repo=fetch_location, destination=name)
+                vcs_class = BzrHandler
             elif fetch_method == 'bzr-export':
-                vcs_handler = BzrHandler(branch=False,
-                                         repo=fetch_location, destination=name)
+                vcs_class = BzrHandler
+                kwargs['branch'] = False
             elif fetch_method == 'dev':
-                vcs_handler = DevHandler(repo=fetch_location, destination=name)
+                vcs_class = DevHandler
             else:
-                vcs_handler = GitHandler(repo=fetch_location, destination=name)
+                vcs_class = GitHandler
+            vcs_handler = vcs_class(**kwargs)
 
             includes = suite.get('include_tests')
             excludes = suite.get('exclude_tests')
@@ -483,10 +500,14 @@
                 self.fetched_suites.append(name)
 
                 # Create a TestSuite
-                s = TestSuite(name=name, runlist_file=suite_runlist,
-                              includes=includes, excludes=excludes,
-                              result=self.result, path=self.testsuitedir,
+                s = TestSuite(name=name,
+                              runlist_file=suite_runlist,
+                              includes=includes,
+                              excludes=excludes,
+                              result=self.result,
+                              path=self.testsuitedir,
                               timeout=self.timeout,
+                              battery_measurements=self.battery_measurements,
                               _save_state_callback=self.save_state,
                               _reboot_callback=self.reboot)
                 self.add_suite(s)

=== modified file 'utah/client/testcase.py'
--- utah/client/testcase.py	2012-12-08 02:10:12 +0000
+++ utah/client/testcase.py	2013-02-15 08:57:19 +0000
@@ -94,6 +94,7 @@
     }
 
     def __init__(self, name, path, result, command=None, timeout=None,
+                 battery_measurements=False,
                  _control_data=None, _save_state_callback=None,
                  _reboot_callback=None):
         """
@@ -108,6 +109,7 @@
         self.results = []
         self.result = result
         self.timeout = timeout
+        self.battery_measurements = battery_measurements
         self.command = command
         self.reboot_callback = _reboot_callback
         self.run_as = None
@@ -170,8 +172,12 @@
         """
         if self.status == 'NOTRUN':
             self.set_status('BUILD')
-            result.add_result(run_cmd(self.build_cmd, cwd=self.working_dir,
-                                      cmd_type=CMD_TC_BUILD))
+            cmd_result = run_cmd(
+                self.build_cmd,
+                cwd=self.working_dir,
+                cmd_type=CMD_TC_BUILD,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def setup(self, result):
         """
@@ -179,8 +185,12 @@
         """
         if self.status == 'BUILD' and result.status == 'PASS':
             self.set_status('SETUP')
-            result.add_result(run_cmd(self.tc_setup, cwd=self.working_dir,
-                                      cmd_type=CMD_TC_SETUP))
+            cmd_result = run_cmd(
+                self.tc_setup,
+                cwd=self.working_dir,
+                cmd_type=CMD_TC_SETUP,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def cleanup(self, result):
         """
@@ -188,8 +198,12 @@
         """
         if self.status == 'RUN':
             self.set_status('CLEANUP')
-            result.add_result(run_cmd(self.tc_cleanup, cwd=self.working_dir,
-                                      cmd_type=CMD_TC_CLEANUP))
+            cmd_result = run_cmd(
+                self.tc_cleanup,
+                cwd=self.working_dir,
+                cmd_type=CMD_TC_CLEANUP,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def run(self):
         """
@@ -244,13 +258,15 @@
                 'action': self.action,
                 'expected_results': self.expected_results,
             }
-            result.add_result(run_cmd(self.command,
-                                      timeout=timeout,
-                                      cwd=self.working_dir,
-                                      cmd_type=CMD_TC_TEST,
-                                      run_as=self.run_as,
-                                      ),
-                              extra_info=extra_info)
+            cmd_result = run_cmd(
+                self.command,
+                timeout=timeout,
+                cwd=self.working_dir,
+                cmd_type=CMD_TC_TEST,
+                run_as=self.run_as,
+                battery_measurements=self.battery_measurements,
+            )
+            result.add_result(cmd_result, extra_info=extra_info)
 
             # only if we haven't failed or errored so far do
             # we set the summary for the testcase.

=== modified file 'utah/client/tests/test_testcase.py'
--- utah/client/tests/test_testcase.py	2012-12-11 09:47:51 +0000
+++ utah/client/tests/test_testcase.py	2013-02-15 08:57:19 +0000
@@ -118,6 +118,7 @@
             'dependencies': 'n/a',
             'action': 'n/a',
             'expected_results': 'n/a',
+            'battery_measurements': False,
         }
 
         result = ResultYAML()

=== modified file 'utah/client/testsuite.py'
--- utah/client/testsuite.py	2012-12-08 02:10:12 +0000
+++ utah/client/testsuite.py	2013-02-15 08:57:19 +0000
@@ -82,7 +82,7 @@
 
     def __init__(self, name, path, result, runlist_file=DEFAULT_TSLIST,
                  control_file=DEFAULT_TSCONTROL, includes=None, excludes=None,
-                 timeout=None,
+                 timeout=None, battery_measurements=False,
                  _control_data=None, _runlist_data=None,
                  _save_state_callback=None, _reboot_callback=None):
 
@@ -92,6 +92,7 @@
         self.failures = 0
         self.errors = 0
         self.timeout = timeout
+        self.battery_measurements = battery_measurements
 
         # work from within the testsuite's directory.
         # eg. /var/lib/utah/examples
@@ -164,9 +165,12 @@
 
             command = test.get('command')
 
-            tc = TestCase(name=name, path=test_path,
-                          command=command, timeout=self.timeout,
+            tc = TestCase(name=name,
+                          path=test_path,
+                          command=command,
+                          timeout=self.timeout,
                           result=self.result,
+                          battery_measurements=self.battery_measurements,
                           _save_state_callback=self.save_state_callback,
                           _reboot_callback=self.reboot_callback)
             if 'overrides' in test:
@@ -193,8 +197,12 @@
         """
         if self.status == 'NOTRUN':
             self.set_status('BUILD')
-            result.add_result(run_cmd(self.build_cmd, cwd=self.name,
-                                      cmd_type=CMD_TS_BUILD))
+            cmd_result = run_cmd(
+                self.build_cmd,
+                cwd=self.name,
+                cmd_type=CMD_TS_BUILD,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def setup(self, result):
         """
@@ -202,8 +210,12 @@
         """
         if self.status == 'BUILD' and result.status == 'PASS':
             self.set_status('SETUP')
-            result.add_result(run_cmd(self.ts_setup, cwd=self.name,
-                                      cmd_type=CMD_TS_SETUP))
+            cmd_result = run_cmd(
+                self.ts_setup,
+                cwd=self.name,
+                cmd_type=CMD_TS_SETUP,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def cleanup(self, result):
         """
@@ -211,8 +223,12 @@
         """
         if self.status == 'INPROGRESS':
             self.set_status('CLEANUP')
-            result.add_result(run_cmd(self.ts_cleanup, cwd=self.name,
-                                      cmd_type=CMD_TS_CLEANUP))
+            cmd_result = run_cmd(
+                self.ts_cleanup,
+                cwd=self.name,
+                cmd_type=CMD_TS_CLEANUP,
+                battery_measurements=self.battery_measurements)
+            result.add_result(cmd_result)
 
     def run(self):
         """