Repository: google/mobly

Issue:
Instrumentation parser output is concatenated in python3.
Python3 outputs newline characters as literals for byte strings.
This causes an issue because `run_instrumentation_test` outputs the raw instrumentation output, which ends up being concatenated since the newlines doesn't properly display with python3.

When fixing https://github.com/google/mobly/issues/429, I did not actually run against a real device with python3 (only ran unit tests with python 2 and 3), which meant that I did not actually make sure the output looked correct.

Discussion:
xpconanfan: Is there any test we can add for this?
winterfroststrom: I did write a unit test for this, but I originally decided against including it because it's kinda specific.

---

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

---



*Comments from [Reviewable](https://beta.reviewable.io/reviews/google/mobly/432#-:-L9l4tLx5kob-0O5JS3w:bgipfst)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index f1a4636..14828a4 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -436,9 +436,8 @@ class AndroidDevice(object):
         self._log_path = os.path.join(self._log_path_base,
                                       'AndroidDevice%s' % self._serial)
         self._debug_tag = self._serial
-        self.log = AndroidDeviceLoggerAdapter(logging.getLogger(), {
-            'tag': self.debug_tag
-        })
+        self.log = AndroidDeviceLoggerAdapter(logging.getLogger(),
+                                              {'tag': self.debug_tag})
         self.sl4a = None
         self.ed = None
         self._adb_logcat_process = None
@@ -937,6 +936,7 @@ class AndroidDevice(object):
         f_name = os.path.basename(self.adb_logcat_file_path)
         out_name = f_name.replace('adblog,', '').replace('.txt', '')
         out_name = ',%s,%s.txt' % (begin_time, out_name)
+        out_name = out_name.replace(':', '-')
         tag_len = utils.MAX_FILENAME_LEN - len(out_name)
         tag = tag[:tag_len]
         out_name = tag + out_name
diff --git a/mobly/controllers/android_device_lib/jsonrpc_client_base.py b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
index 07dc50d..dd32769 100644
--- a/mobly/controllers/android_device_lib/jsonrpc_client_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
@@ -316,6 +316,17 @@ class JsonRpcClientBase(object):
                 ad=self._ad)
         return result['result']
 
+    def disable_hidden_api_blacklist(self):
+        """If necessary and possible, disables hidden api blacklist."""
+        version_codename = self._ad.adb.getprop('ro.build.version.codename')
+        sdk_version = int(self._ad.adb.getprop('ro.build.version.sdk'))
+        # we check version_codename in addition to sdk_version because P builds
+        # in development report sdk_version 27, but still enforce the blacklist.
+        if self._ad.is_rootable and (sdk_version >= 28 or
+                                     version_codename == 'P'):
+            self._ad.adb.shell(
+                'settings put global hidden_api_blacklist_exemptions "*"')
+
     def __getattr__(self, name):
         """Wrapper for python magic to turn method calls into RPC calls."""
 
diff --git a/mobly/controllers/android_device_lib/sl4a_client.py b/mobly/controllers/android_device_lib/sl4a_client.py
index 4b5396a..1222925 100644
--- a/mobly/controllers/android_device_lib/sl4a_client.py
+++ b/mobly/controllers/android_device_lib/sl4a_client.py
@@ -61,6 +61,7 @@ class Sl4aClient(jsonrpc_client_base.JsonRpcClientBase):
             raise jsonrpc_client_base.AppStartError(
                 self._ad, '%s is not installed on %s' % (_APP_NAME,
                                                          self._adb.serial))
+        self.disable_hidden_api_blacklist()
 
         # sl4a has problems connecting after disconnection, so kill the apk and
         # try connecting again.
diff --git a/mobly/controllers/android_device_lib/snippet_client.py b/mobly/controllers/android_device_lib/snippet_client.py
index 1f7ec0d..7f4893f 100644
--- a/mobly/controllers/android_device_lib/snippet_client.py
+++ b/mobly/controllers/android_device_lib/snippet_client.py
@@ -90,6 +90,7 @@ class SnippetClient(jsonrpc_client_base.JsonRpcClientBase):
     def start_app_and_connect(self):
         """Overrides superclass. Launches a snippet app and connects to it."""
         self._check_app_installed()
+        self.disable_hidden_api_blacklist()
 
         persists_shell_cmd = self._get_persist_command()
         # Use info here so people can follow along with the snippet startup


Test patch:
diff --git a/mobly/base_instrumentation_test.py b/mobly/base_instrumentation_test.py
index 4966cd4..bb72075 100644
--- a/mobly/base_instrumentation_test.py
+++ b/mobly/base_instrumentation_test.py
@@ -927,7 +927,7 @@ class BaseInstrumentationTestClass(base_test.BaseTestClass):
             package=package,
             options=options,
             runner=runner,
-        )
+        ).decode('utf-8')
         logging.info('Outputting instrumentation test log...')
         logging.info(instrumentation_output)
 
@@ -935,5 +935,5 @@ class BaseInstrumentationTestClass(base_test.BaseTestClass):
         instrumentation_block = _InstrumentationBlock(prefix=prefix)
         for line in instrumentation_output.splitlines():
             instrumentation_block = self._parse_line(instrumentation_block,
-                                                     line.decode('utf-8'))
+                                                     line)
         return self._finish_parsing(instrumentation_block)
diff --git a/tests/mobly/base_instrumentation_test_test.py b/tests/mobly/base_instrumentation_test_test.py
index 2256475..3908015 100755
--- a/tests/mobly/base_instrumentation_test_test.py
+++ b/tests/mobly/base_instrumentation_test_test.py
@@ -34,6 +34,17 @@ MOCK_PREFIX = 'my_prefix'
 # A mock name for the instrumentation test subclass.
 MOCK_INSTRUMENTATION_TEST_CLASS_NAME = 'MockInstrumentationTest'
 
+MOCK_EMPTY_INSTRUMENTATION_TEST = """\
+INSTRUMENTATION_RESULT: stream=
+
+Time: 0.001
+
+OK (0 tests)
+
+
+INSTRUMENTATION_CODE: -1
+"""
+
 
 class MockInstrumentationTest(BaseInstrumentationTestClass):
     def __init__(self, tmp_dir, user_params={}):
@@ -229,18 +240,21 @@ INSTRUMENTATION_STATUS_CODE: -1
             instrumentation_output, expected_has_error=True)
 
     def test_run_instrumentation_test_with_no_tests(self):
-        instrumentation_output = """\
-INSTRUMENTATION_RESULT: stream=
-
-Time: 0.001
-
-OK (0 tests)
-
+        instrumentation_output = MOCK_EMPTY_INSTRUMENTATION_TEST
+        self.assert_run_instrumentation_test(
+            instrumentation_output, expected_completed_and_passed=True)
 
-INSTRUMENTATION_CODE: -1
-"""
+    @unittest.skipUnless(
+        sys.version_info >= (3, 0),
+        'Only python3 displays different string types differently.')
+    @mock.patch('logging.info')
+    def test_run_instrumentation_test_logs_correctly(self, mock_info_logger):
+        instrumentation_output = MOCK_EMPTY_INSTRUMENTATION_TEST
         self.assert_run_instrumentation_test(
             instrumentation_output, expected_completed_and_passed=True)
+        for mock_call in mock_info_logger.mock_calls:
+            logged_format = mock_call[1][0]
+            self.assertIsInstance(logged_format, str)
 
     def test_run_instrumentation_test_with_passing_test(self):
         instrumentation_output = """\
diff --git a/tests/mobly/controllers/android_device_lib/sl4a_client_test.py b/tests/mobly/controllers/android_device_lib/sl4a_client_test.py
index e75bd53..bc06220 100755
--- a/tests/mobly/controllers/android_device_lib/sl4a_client_test.py
+++ b/tests/mobly/controllers/android_device_lib/sl4a_client_test.py
@@ -34,6 +34,12 @@ class MockAdbProxy(object):
                 return bytes('', 'utf-8')
             return bytes('package:com.googlecode.android_scripting', 'utf-8')
 
+    def getprop(self, params):
+        if params == 'ro.build.version.codename':
+            return 'Z'
+        elif params == 'ro.build.version.sdk':
+            return '28'
+
     def __getattr__(self, name):
         """All calls to the none-existent functions in adb proxy would
         simply return the adb command string.
diff --git a/tests/mobly/controllers/android_device_lib/snippet_client_test.py b/tests/mobly/controllers/android_device_lib/snippet_client_test.py
index 28dc3d8..2c875d8 100755
--- a/tests/mobly/controllers/android_device_lib/snippet_client_test.py
+++ b/tests/mobly/controllers/android_device_lib/snippet_client_test.py
@@ -55,6 +55,12 @@ class MockAdbProxy(object):
         elif 'which' in params:
             return b''
 
+    def getprop(self, params):
+        if params == 'ro.build.version.codename':
+            return 'Z'
+        elif params == 'ro.build.version.sdk':
+            return '28'
+
     def __getattr__(self, name):
         """All calls to the none-existent functions in adb proxy would
         simply return the adb command string.
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index b1428ec..f175f17 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -630,7 +630,7 @@ class AndroidDeviceTest(unittest.TestCase):
         ad.cat_adb_log('some_test', MOCK_ADB_LOGCAT_BEGIN_TIME)
         cat_file_path = os.path.join(
             ad.log_path, 'AdbLogExcerpts',
-            ('some_test,02-29 14:02:20.123,%s,%s.txt') % (ad.model, ad.serial))
+            ('some_test,02-29 14-02-20.123,%s,%s.txt') % (ad.model, ad.serial))
         with open(cat_file_path, 'r') as f:
             actual_cat = f.read()
         self.assertEqual(actual_cat, ''.join(MOCK_ADB_LOGCAT_CAT_RESULT))
diff --git a/tests/mobly/controllers/monsoon_test.py b/tests/mobly/controllers/monsoon_test.py
index a726049..c8b6eeb 100755
--- a/tests/mobly/controllers/monsoon_test.py
+++ b/tests/mobly/controllers/monsoon_test.py
@@ -12,12 +12,16 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import platform
+
 from future.tests.base import unittest
 
 
 class MonsoonTest(unittest.TestCase):
-
+    @unittest.skipIf(platform.system() == 'Windows',
+                     'fcntl does not exist on Windows')
     def test_monsoon_import(self):
+        # TODO: Replace 'fnctl' with a Windows equivalent when on Windows
         from mobly.controllers import monsoon
 
 
diff --git a/tests/mobly/logger_test.py b/tests/mobly/logger_test.py
index b1cf839..1ac9f1d 100755
--- a/tests/mobly/logger_test.py
+++ b/tests/mobly/logger_test.py
@@ -1,11 +1,11 @@
 # Copyright 2016 Google Inc.
-# 
+#
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
diff --git a/tests/mobly/output_test.py b/tests/mobly/output_test.py
index 9655f4c..ff5cae8 100755
--- a/tests/mobly/output_test.py
+++ b/tests/mobly/output_test.py
@@ -15,6 +15,7 @@
 import logging
 import mock
 import os
+import platform
 import shutil
 import tempfile
 import unittest
@@ -28,6 +29,10 @@ from tests.lib import mock_controller
 from tests.lib import integration_test
 from tests.lib import teardown_class_failure_test
 
+if platform.system() == 'Windows':
+    import win32file
+    from win32com import client
+
 
 class OutputTest(unittest.TestCase):
     """This test class has unit tests for the implementation of Mobly's output
@@ -87,6 +92,39 @@ class OutputTest(unittest.TestCase):
             for item in blacklist:
                 self.assertNotIn(item, content)
 
+    @unittest.skipIf(platform.system() == 'Windows',
+                     'Symlinks are usually specific to Unix operating systems')
+    def test_symlink(self):
+        """Verifies the symlink is created and links properly."""
+        mock_test_config = self.create_mock_test_config(
+            self.base_mock_test_config)
+        tr = test_runner.TestRunner(self.log_dir, self.test_bed_name)
+        tr.setup_logger()
+        symlink = os.path.join(self.log_dir, self.test_bed_name, 'latest')
+        self.assertEqual(os.readlink(symlink), logging.log_path)
+
+    @unittest.skipIf(platform.system() != 'Windows',
+                     'Shortcuts are specific to Windows operating systems')
+    def test_shortcut(self):
+        """Verifies the shortcut is created and links properly."""
+        shortcut_path = os.path.join(self.log_dir, self.test_bed_name,
+                                     'latest.lnk')
+        shell = client.Dispatch("WScript.Shell")
+        shortcut = shell.CreateShortCut(shortcut_path)
+        self.assertFalse(shortcut.Targetpath)
+        mock_test_config = self.create_mock_test_config(
+            self.base_mock_test_config)
+        tr = test_runner.TestRunner(self.log_dir, self.test_bed_name)
+        tr.setup_logger()
+        tr._teardown_logger()
+        shortcut = shell.CreateShortCut(shortcut_path)
+        # Normalize paths for case and truncation
+        normalized_shortcut_path = os.path.normcase(
+            win32file.GetLongPathName(shortcut.Targetpath))
+        normalized_logger_path = os.path.normcase(
+            win32file.GetLongPathName(logging.log_path))
+        self.assertEqual(normalized_shortcut_path, normalized_logger_path)
+
     def test_setup_logger_before_run(self):
         """Verifies the expected output files from a test run.
 
@@ -103,7 +141,7 @@ class OutputTest(unittest.TestCase):
         logging.debug(debug_uuid)
         tr.add_test_class(mock_test_config, integration_test.IntegrationTest)
         tr.run()
-        output_dir = os.path.join(self.log_dir, self.test_bed_name, 'latest')
+        output_dir = logging.log_path
         (summary_file_path, debug_log_path,
          info_log_path) = self.assert_output_logs_exist(output_dir)
         self.assert_log_contents(
@@ -190,7 +228,7 @@ class OutputTest(unittest.TestCase):
         tr = test_runner.TestRunner(self.log_dir, self.test_bed_name)
         tr.add_test_class(mock_test_config, integration_test.IntegrationTest)
         tr.run()
-        output_dir = os.path.join(self.log_dir, self.test_bed_name, 'latest')
+        output_dir = logging.log_path
         (summary_file_path, debug_log_path,
          info_log_path) = self.assert_output_logs_exist(output_dir)
         summary_entries = []
@@ -211,7 +249,7 @@ class OutputTest(unittest.TestCase):
         tr.add_test_class(mock_test_config,
                           teardown_class_failure_test.TearDownClassFailureTest)
         tr.run()
-        output_dir = os.path.join(self.log_dir, self.test_bed_name, 'latest')
+        output_dir = logging.log_path
         summary_file_path = os.path.join(output_dir,
                                          records.OUTPUT_FILE_SUMMARY)
         found = False
diff --git a/tests/mobly/test_runner_test.py b/tests/mobly/test_runner_test.py
index 35eeded..ab26971 100755
--- a/tests/mobly/test_runner_test.py
+++ b/tests/mobly/test_runner_test.py
@@ -12,8 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import logging
 import mock
 import os
+import platform
+import re
 import shutil
 import tempfile
 import yaml
@@ -201,8 +204,7 @@ class TestRunnerTest(unittest.TestCase):
         tr = test_runner.TestRunner(self.log_dir, self.test_bed_name)
         tr.add_test_class(mock_test_config, integration_test.IntegrationTest)
         tr.run()
-        summary_path = os.path.join(mock_test_config.log_path,
-                                    mock_test_config.test_bed_name, 'latest',
+        summary_path = os.path.join(logging.log_path,
                                     records.OUTPUT_FILE_SUMMARY)
         with open(summary_path, 'r') as f:
             summary_entries = list(yaml.load_all(f))
@@ -297,7 +299,7 @@ class TestRunnerTest(unittest.TestCase):
                 test_runner.Error,
                 'TestRunner\'s log folder is "/different/log/dir", but a test '
                 r'config with a different log folder \("%s"\) was added.' %
-                self.log_dir):
+                re.escape(self.log_dir)):
             tr.add_test_class(self.base_mock_test_config,
                               integration_test.IntegrationTest)