Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo; r?whimboo,maja_zf draft
authorAndreas Tolfsen <ato@mozilla.com>
Sat, 27 May 2017 17:58:55 +0100
changeset 592686 ff037d3b85d260d7daaae195a43e95771a160a24
parent 592668 2a63a6c35033b5cbc6c98cabc7657c7290284691
child 632906 103e713f4707e70a8d0018d9ae11fb0cf34a276d
push id63478
push userbmo:ato@mozilla.com
push dateMon, 12 Jun 2017 17:26:18 +0000
reviewerswhimboo, maja_zf
bugs1368195
milestone56.0a1
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo; r?whimboo,maja_zf BaseMarionetteTestRunner.appinfo is used in two places: when logging whether E10s is enabled at the beginning of a test run, and for comparing a few properties in the WebDriver capabilities test. It currently tries to serialise Services.appinfo, which has a field QueryInterface, which is a function. Because of the Function.prototype.inherits field, this serialisation results in an infinite recursion, presumably because the inherits method itself is a function. This patch removes BaseMarionetteTestRunner.appinfo as it is a clunky and potentially error-prone implementation due to the way it caches appinfo, and replaces the current consumers with functions retrieving specific properties from Services.appinfo. MozReview-Commit-ID: BPDA6TJrHHb
testing/marionette/harness/marionette_harness/runner/base.py
testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py
testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
--- a/testing/marionette/harness/marionette_harness/runner/base.py
+++ b/testing/marionette/harness/marionette_harness/runner/base.py
@@ -506,17 +506,16 @@ class BaseMarionetteTestRunner(object):
                  symbols_path=None,
                  shuffle=False, shuffle_seed=random.randint(0, sys.maxint), this_chunk=1,
                  total_chunks=1,
                  server_root=None, gecko_log=None, result_callbacks=None,
                  prefs=None, test_tags=None,
                  socket_timeout=BaseMarionetteArguments.socket_timeout_default,
                  startup_timeout=None, addons=None, workspace=None,
                  verbose=0, e10s=True, emulator=False, headless=False, **kwargs):
-        self._appinfo = None
         self._appName = None
         self._capabilities = None
         self._filename_pattern = None
         self._version_info = {}
 
         self.fixture_servers = {}
         self.fixtures = Fixtures()
         self.extra_kwargs = kwargs
@@ -547,22 +546,27 @@ class BaseMarionetteTestRunner(object):
         self.test_tags = test_tags
         self.startup_timeout = startup_timeout
         self.workspace = workspace
         # If no workspace is set, default location for gecko.log is .
         # and default location for profile is TMP
         self.workspace_path = workspace or os.getcwd()
         self.verbose = verbose
         self.headless = headless
+
+        # self.e10s stores the desired configuration, whereas
+        # self._e10s_from_browser is the cached value from querying e10s
+        # in self.is_e10s
         self.e10s = e10s
+        self._e10s_from_browser = None
         if self.e10s:
             self.prefs.update({
                 'browser.tabs.remote.autostart': True,
                 'browser.tabs.remote.force-enable': True,
-                'extensions.e10sBlocksEnabling': False
+                'extensions.e10sBlocksEnabling': False,
             })
 
         def gather_debug(test, status):
             # No screenshots and page source for skipped tests
             if status == "SKIP":
                 return
 
             rv = {}
@@ -652,33 +656,16 @@ class BaseMarionetteTestRunner(object):
             return self._capabilities
 
         self.marionette.start_session()
         self._capabilities = self.marionette.session_capabilities
         self.marionette.delete_session()
         return self._capabilities
 
     @property
-    def appinfo(self):
-        if self._appinfo:
-            return self._appinfo
-
-        self.marionette.start_session()
-        with self.marionette.using_context('chrome'):
-            self._appinfo = self.marionette.execute_script("""
-            try {
-              return Services.appinfo;
-            } catch (e) {
-              return null;
-            }""")
-        self.marionette.delete_session()
-        self._appinfo = self._appinfo or {}
-        return self._appinfo
-
-    @property
     def appName(self):
         if self._appName:
             return self._appName
 
         self._appName = self.capabilities.get('browserName')
         return self._appName
 
     @property
@@ -812,16 +799,32 @@ class BaseMarionetteTestRunner(object):
         for test in self.manifest_skipped_tests:
             name = os.path.basename(test['path'])
             self.logger.test_start(name)
             self.logger.test_end(name,
                                  'SKIP',
                                  message=test['disabled'])
             self.todo += 1
 
+    @property
+    def is_e10s(self):
+        """Query the browser on whether E10s (Electrolysis) is enabled."""
+        if self.marionette is None or self.marionette.session is None:
+            self._e10s_from_browser = None
+            raise Exception("No Marionette session to query e10s state")
+
+        if self._e10s_from_browser is not None:
+            return self._e10s_from_browser
+
+        with self.marionette.using_context("chrome"):
+            self._e10s_from_browser = self.marionette.execute_script(
+                "return Services.appinfo.browserTabsRemoteAutostart")
+
+        return self._e10s_from_browser
+
     def run_tests(self, tests):
         start_time = time.time()
         self._initialize_test_run(tests)
 
         if self.marionette is None:
             self.marionette = self.driverclass(**self._build_kwargs())
             self.logger.info("Profile path is %s" % self.marionette.profile_path)
 
@@ -839,23 +842,23 @@ class BaseMarionetteTestRunner(object):
 
         device_info = None
         if self.marionette.instance and self.emulator:
             try:
                 device_info = self.marionette.instance.runner.device.dm.getInfo()
             except Exception:
                 self.logger.warning('Could not get device info', exc_info=True)
 
-        appinfo_e10s = self.appinfo.get('browserTabsRemoteAutostart', False)
-        self.logger.info("e10s is {}".format("enabled" if appinfo_e10s else "disabled"))
-        if self.e10s != appinfo_e10s:
-            message_e10s = ("BaseMarionetteTestRunner configuration (self.e10s) does "
-                            "not match browser appinfo")
+        self.marionette.start_session()
+        self.logger.info("e10s is {}".format("enabled" if self.is_e10s else "disabled"))
+        if self.e10s != self.is_e10s:
             self.cleanup()
-            raise AssertionError(message_e10s)
+            raise AssertionError("BaseMarionetteTestRunner configuration (self.e10s) "
+                                 "does not match browser appinfo (self.is_e10s)")
+        self.marionette.delete_session()
 
         tests_by_group = defaultdict(list)
         for test in self.tests:
             tests_by_group[test['group']].append(test['filepath'])
 
         self.logger.suite_start(tests_by_group,
                                 version_info=self.version_info,
                                 device_info=device_info)
--- a/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py
+++ b/testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py
@@ -25,17 +25,17 @@ def mock_runner(runner, mock_marionette,
     self.marionette and other properties,
     to enable testing runner.run_tests().
     """
     runner.driverclass = Mock(return_value=mock_marionette)
     for attr in ['run_test_set', '_capabilities']:
         setattr(runner, attr, Mock())
     runner._appName = 'fake_app'
     # simulate that browser runs with e10s by default
-    runner._appinfo = {'browserTabsRemoteAutostart': True}
+    runner._e10s_from_browser = True
     monkeypatch.setattr('marionette_harness.runner.base.mozversion', Mock())
     return runner
 
 
 @pytest.fixture
 def build_kwargs_using(mach_parsed_kwargs):
     '''Helper function for test_build_kwargs_* functions'''
     def kwarg_builder(new_items, return_socket=False):
@@ -427,17 +427,18 @@ def test_e10s_option_sets_prefs(mach_par
         'extensions.e10sBlocksEnabling': False
     }
     for k,v in e10s_prefs.iteritems():
         if k == 'extensions.e10sBlocksEnabling' and not e10s:
             continue
         assert runner.prefs.get(k, False) == (v and e10s)
 
 def test_e10s_option_clash_raises(mock_runner):
-    mock_runner.e10s = False
+    mock_runner._e10s_from_browser = False
+
     with pytest.raises(AssertionError) as e:
         mock_runner.run_tests([u'test_fake_thing.py'])
         assert "configuration (self.e10s) does not match browser appinfo" in e.value.message
 
 if __name__ == '__main__':
     import sys
     sys.exit(pytest.main(
         ['--log-tbpl=-', __file__]))
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ -8,18 +8,23 @@ from marionette_harness import Marionett
 
 
 class TestCapabilities(MarionetteTestCase):
 
     def setUp(self):
         super(TestCapabilities, self).setUp()
         self.caps = self.marionette.session_capabilities
         with self.marionette.using_context("chrome"):
-            self.appinfo = self.marionette.execute_script(
-                "return Services.appinfo")
+            self.appinfo = self.marionette.execute_script("""
+                return {
+                  name: Services.appinfo.name,
+                  version: Services.appinfo.version,
+                  processID: Services.appinfo.processID,
+                }
+                """)
             self.os_name = self.marionette.execute_script(
                 "return Services.sysinfo.getProperty('name')").lower()
             self.os_version = self.marionette.execute_script(
                 "return Services.sysinfo.getProperty('version')")
 
     def test_mandated_capabilities(self):
         self.assertIn("browserName", self.caps)
         self.assertIn("browserVersion", self.caps)