[BattOr] Add real device smoke test

common/battor/battor/battor_wrapper_devicetest.py

Index: common/battor/battor/battor_wrapper_devicetest.py
diff --git a/common/battor/battor/battor_wrapper_devicetest.py b/common/battor/battor/battor_wrapper_devicetest.py
new file mode 100644
index 0000000000000000000000000000000000000000..2e613619dc81e62b1858b3ff882cd3ece1d626e4
--- /dev/null
+++ b/common/battor/battor/battor_wrapper_devicetest.py
@@ -0,0 +1,80 @@
+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import logging
+import platform
+import os
+import sys
+import time
+import unittest
+
+if __name__ == '__main__':

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

What's this for?

[rnephew (Reviews Here), 2016/04/29 17:05:57]

When calling this test directly (like the CQ does) you need to have
common/battor put into the python path in order to import battor_wrapper.

+  sys.path.append(
+      os.path.join(os.path.dirname(__file__), '..'))
+
+from battor import battor_wrapper
+from devil.utils import battor_device_mapping
+from devil.utils import find_usb_devices
+
+
+# TODO(rnephew): Expand to work on mac when the CQ has a mac with a BattOr.
+_SUPPORTED_CQ_PLATFORMS = ['win', 'linux']

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

Have we tested this on both Mac and Linux?

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Not on mac, I have no mac battor cables.

[charliea (OOO until 10-5), 2016/04/29 17:18:17]

Errr, sorry, I meant both Win and Linux.

[rnephew (Reviews Here), 2016/04/29 17:29:06]

Yep. It has been tried locally on both windows and linux, and on the CQ via
tryjob for windows. There are no linux battors on the CQ to teste that with.

+
+class BattorWrapperDeviceTest(unittest.TestCase):
+  def setUp(self):
+    test_platform = platform.system()
+    self._battor_list = None
+
+    if 'Win' in test_platform:
+      self._platform = 'win'
+    elif 'Linux' in test_platform:
+      self._platform = 'linux'
+      device_tree  = find_usb_devices.GetBusNumberToDeviceTreeMap()
+      self._battor_list = battor_device_mapping.GetBattorList(device_tree)
+    elif 'Darwin' in test_platform: # Mac.

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

I think the "# Mac." is probably unnecessary - it's just as easy to just read
one line further down.

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Done.

+      self._platform = 'mac'
+
+  def testFullRun(self):
+    if self._platform not in _SUPPORTED_CQ_PLATFORMS:
+      logging.critical('Platform %s is not supported on CQ.' % self._platform)
+      return
+
+    # If battor_list is an empty list, a BattOr was expected but not found.
+    if self._battor_list is not None and not self._battor_list:
+      logging.critical('No BattOrs attached. Cannot run tests.')
+      return # No BattOrs, cannot run test.

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

This comment is probably unnecessary - it's just a restatement of the critical
log above

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Done.

+
+    battor_path = None if not self._battor_list else self._battor_list[0]
+    battor = battor_wrapper.BattorWrapper(self._platform,

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

This seems really specific to Linux. Does this work on Windows?

[rnephew (Reviews Here), 2016/04/29 17:05:57]

It does, but only because we are passing it win as the platform. Fixed it to be
more clear whats going on.

+                                          battor_path='/dev/%s' % battor_path)
+    battor.StartShell()
+    battor.StartTracing()
+    # Sleep here because clock sync marker will be flaky if not.

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

Have you experienced flakiness here? I don't think that there's any technical
reason why it should be. If this is related to
https://bugs.chromium.org/p/chromium/issues/detail?id=602266&q=owner%3Acharli...,
then we should say something like:

TODO(rnephew): This sleep is required for now because crbug.com/602266 causes
the BattOr to crash when the trace time is too short. Once that bug is fixed, we
should remove this delay.

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Added, I have also seen the clock sync not show up if you issue it to close to
the starting of tracing.

+    time.sleep(1)
+    battor.RecordClockSyncMarker('abc')
+    # Sleep here because clock sync marker will be flaky if not.
+    time.sleep(1)

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

This shouldn't be required either. 

It's definitely the case where we need to delay a little bit after the
RecordClockSyncMarker call and before calling StopTracing to make sure that we
give time for the clock synced sample to be flushed. However, I wrote some code
to handle this within the BattOr agent itself:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/battor_agent...

What kind of behavior are you seeing without this delay in?

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Clock sync will not always be present. 

+    battor.StopTracing()
+    if self._platform == 'win':
+      # This is a work around for crbug.com/603309.

[charliea (OOO until 10-5), 2016/04/29 15:11:07]

Could you add some to this comment explaining how sleeping 5 seconds and killing
the battor_shell is a workaround for that bug?

[rnephew (Reviews Here), 2016/04/29 17:05:57]

Done.

+      time.sleep(5)
+      battor._battor_shell.kill()
+    results = battor.CollectTraceData()
+    self.assertTrue('# BattOr' in results[0])
+    self.assertTrue('# voltage_range' in results[1])
+    self.assertTrue('# current_range' in results[2])
+    self.assertTrue('# sample_rate' in results[3])
+    # First line with results. Should be 3 'words'.
+    self.assertTrue(len(results[4].split()) == 3)
+    clock_sync_found = False
+    for entry in results:
+      if '<abc>' in entry:
+        clock_sync_found = True
+        break
+    self.assertTrue(clock_sync_found, 'BattOr Data:%s\n' % repr(results))
+
+
+if __name__ == '__main__':
+  logging.getLogger().setLevel(logging.DEBUG)
+  unittest.main(verbosity=2)