Reland "[lacros] Make test runner supports lacros_chrome_browsertests"

This reverts commit 485226400b.

Reason for revert: During the refactoring, 'os.listdir' was somehow
accidentally changed to 'list', which resulted in the test failures,
and this CL fixes the problem.

Original change's description:
> Revert "[lacros] Make test runner supports lacros_chrome_browsertests"
>
> This reverts commit e4eacd09b1.
>
> Reason for revert: Suspect for lacros test failures:
> https://ci.chromium.org/p/chromium/builders/ci/linux-lacros-tester-rel/2227
> The only lacros-related change in diff with the previous success run.
>
> Original change's description:
> > [lacros] Make test runner supports lacros_chrome_browsertests
> >
> > This CL makes test runner supports lacros_chrome_browsertests by:
> > 1. Appending necessary args to establish mojo connections.
> > 2. Always use '--test-launcher-jobs=1' to run tests serially because
> >    multiple clients crosapis are still not supported yet.
> >
> > Bug: 1120582
> > Change-Id: I865ace26aa4a86c83912c6660bc433d79e43ef76
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410350
> > Commit-Queue: Yuke Liao <liaoyuke@chromium.org>
> > Reviewed-by: Erik Chen <erikchen@chromium.org>
> > Reviewed-by: Sven Zheng <svenzheng@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#806903}
>
> TBR=erikchen@chromium.org,liaoyuke@chromium.org,svenzheng@chromium.org
>
> Change-Id: Ie630ade531428e78718127792c4f6ab6b0d55025
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1120582
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410246
> Reviewed-by: Sergey Poromov <poromov@chromium.org>
> Commit-Queue: Sergey Poromov <poromov@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#806980}

TBR=erikchen@chromium.org,poromov@chromium.org,liaoyuke@chromium.org,svenzheng@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1120582
Change-Id: Ifaa2235a0a4f9ce8deb9aea5011eda0924e1f051
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412253
Commit-Queue: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807111}

build/lacros/test_runner.py

@@ -70,6 +70,9 @@ _GS_ASH_CHROME_PATH = 'ash-chromium.zip'
 _PREBUILT_ASH_CHROME_DIR = os.path.join(os.path.dirname(__file__),
                                         'prebuilt_ash_chrome')
 
+# Number of seconds to wait for ash-chrome to start.
+ASH_CHROME_TIMEOUT_SECONDS = 10
+
 # List of targets that require ash-chrome as a Wayland server in order to run.
 _TARGETS_REQUIRE_ASH_CHROME = [
     'app_shell_unittests',
@@ -227,11 +230,49 @@ def _GetLatestVersionOfAshChrome():
       return f.read().strip()
 
 
+def _WaitForAshChromeToStart(tmp_xdg_dir, lacros_mojo_socket_file,
+                             is_lacros_chrome_browsertests):
+  """Waits for Ash-Chrome to be up and running and returns a boolean indicator.
+
+  Determine whether ash-chrome is up and running by checking whether two files
+  (lock file + socket) have been created in the |XDG_RUNTIME_DIR| and the lacros
+  mojo socket file has been created if running lacros_chrome_browsertests.
+  TODO(crbug.com/1107966): Figure out a more reliable hook to determine the
+  status of ash-chrome, likely through mojo connection.
+
+  Args:
+    tmp_xdg_dir (str): Path to the XDG_RUNTIME_DIR.
+    lacros_mojo_socket_file (str): Path to the lacros mojo socket file.
+    is_lacros_chrome_browsertests (bool): is running lacros_chrome_browsertests.
+
+  Returns:
+    A boolean indicating whether Ash-chrome is up and running.
+  """
+
+  def IsAshChromeReady(tmp_xdg_dir, lacros_mojo_socket_file,
+                       is_lacros_chrome_browsertests):
+    return (len(os.listdir(tmp_xdg_dir)) >= 2
+            and (not is_lacros_chrome_browsertests
+                 or os.path.exists(lacros_mojo_socket_file)))
+
+  time_counter = 0
+  while not IsAshChromeReady(tmp_xdg_dir, lacros_mojo_socket_file,
+                             is_lacros_chrome_browsertests):
+    time.sleep(0.5)
+    time_counter += 0.5
+    if time_counter > ASH_CHROME_TIMEOUT_SECONDS:
+      break
+
+  return IsAshChromeReady(tmp_xdg_dir, lacros_mojo_socket_file,
+                          is_lacros_chrome_browsertests)
+
+
 def _RunTestWithAshChrome(args, forward_args):
   """Runs tests with ash-chrome.
 
-  args (dict): Args for this script.
+  Args:
-  forward_args (dict): Args to be forwarded to the test command.
+    args (dict): Args for this script.
+    forward_args (dict): Args to be forwarded to the test command.
   """
   ash_chrome_version = args.ash_chrome_version or _GetLatestVersionOfAshChrome()
   _DownloadAshChromeIfNecessary(ash_chrome_version)
@@ -240,10 +281,19 @@ def _RunTestWithAshChrome(args, forward_args):
   ash_chrome_file = os.path.join(_GetAshChromeDirPath(ash_chrome_version),
                                  'chrome')
   try:
+    # Starts Ash-Chrome.
     tmp_xdg_dir_name = tempfile.mkdtemp()
     tmp_ash_data_dir_name = tempfile.mkdtemp()
-    ash_process = None
 
+    # Please refer to below file for how mojo connection is set up in testing.
+    # //chrome/browser/chromeos/crosapi/test_mojo_connection_manager.h
+    lacros_mojo_socket_file = '%s/lacros.sock' % tmp_ash_data_dir_name
+    lacros_mojo_socket_arg = ('--lacros-mojo-socket-for-testing=%s' %
+                              lacros_mojo_socket_file)
+    is_lacros_chrome_browsertests = (os.path.basename(
+        args.command) == 'lacros_chrome_browsertests')
+
+    ash_process = None
     ash_env = os.environ.copy()
     ash_env['XDG_RUNTIME_DIR'] = tmp_xdg_dir_name
     ash_cmd = [
@@ -252,6 +302,8 @@ def _RunTestWithAshChrome(args, forward_args):
         '--enable-wayland-server',
         '--no-startup-window',
     ]
+    if is_lacros_chrome_browsertests:
+      ash_cmd.append(lacros_mojo_socket_arg)
 
     ash_process_has_started = False
     total_tries = 3
@@ -259,24 +311,14 @@ def _RunTestWithAshChrome(args, forward_args):
     while not ash_process_has_started and num_tries < total_tries:
       num_tries += 1
       ash_process = subprocess.Popen(ash_cmd, env=ash_env)
-
+      ash_process_has_started = _WaitForAshChromeToStart(
-      # Determine whether ash-chrome is up and running by checking whether two
+          tmp_xdg_dir_name, lacros_mojo_socket_file,
-      # files (lock file + socket) have been created in the |XDG_RUNTIME_DIR|.
+          is_lacros_chrome_browsertests)
-      # TODO(crbug.com/1107966): Figure out a more reliable hook to determine
+      if ash_process_has_started:
-      # the status of ash-chrome, likely through mojo connection.
-      time_to_wait = 10
-      time_counter = 0
-      while len(os.listdir(tmp_xdg_dir_name)) < 2:
-        time.sleep(0.5)
-        time_counter += 0.5
-        if time_counter > time_to_wait:
-          break
-
-      if len(os.listdir(tmp_xdg_dir_name)) >= 2:
-        ash_process_has_started = True
         break
 
-      logging.warning('Starting ash-chrome timed out after %ds', time_to_wait)
+      logging.warning('Starting ash-chrome timed out after %ds',
+                      ASH_CHROME_TIMEOUT_SECONDS)
       logging.warning('Printing the output of "ps aux" for debugging:')
       subprocess.call(['ps', 'aux'])
       if ash_process and ash_process.poll() is None:
@@ -285,6 +327,26 @@ def _RunTestWithAshChrome(args, forward_args):
     if not ash_process_has_started:
       raise RuntimeError('Timed out waiting for ash-chrome to start')
 
+    # Starts tests.
+    if is_lacros_chrome_browsertests:
+      forward_args.append(lacros_mojo_socket_arg)
+
+      reason_of_jobs_1 = (
+          'multiple clients crosapi is not supported yet (crbug.com/1124490), '
+          'lacros_chrome_browsertests has to run tests serially')
+
+      if any('--test-launcher-jobs' in arg for arg in forward_args):
+        raise RuntimeError(
+            'Specifying "--test-launcher-jobs" is not allowed because %s. '
+            'Please remove it and this script will automatically append '
+            '"--test-launcher-jobs=1"' % reason_of_jobs_1)
+
+      # TODO(crbug.com/1124490): Run lacros_chrome_browsertests in parallel once
+      # the bug is fixed.
+      logging.warning('Appending "--test-launcher-jobs=1" because %s',
+                      reason_of_jobs_1)
+      forward_args.append('--test-launcher-jobs=1')
+
     test_env = os.environ.copy()
     test_env['EGL_PLATFORM'] = 'surfaceless'
     test_env['XDG_RUNTIME_DIR'] = tmp_xdg_dir_name

build/lacros/test_runner_test.py

@@ -59,6 +59,7 @@ class TestRunnerTest(unittest.TestCase):
       'browser_tests',
       'components_browsertests',
       'content_browsertests',
+      'lacros_chrome_browsertests',
   ])
   @mock.patch.object(os,
                      'listdir',
@@ -67,6 +68,7 @@ class TestRunnerTest(unittest.TestCase):
                      'mkdtemp',
                      side_effect=['/tmp/xdg', '/tmp/ash-data'])
   @mock.patch.object(os.environ, 'copy', side_effect=[{}, {}])
+  @mock.patch.object(os.path, 'exists', return_value=True)
   @mock.patch.object(os.path, 'isfile', return_value=True)
   @mock.patch.object(test_runner,
                      '_GetLatestVersionOfAshChrome',
@@ -85,15 +87,28 @@ class TestRunnerTest(unittest.TestCase):
       ash_chrome_args = mock_popen.call_args_list[0][0][0]
       self.assertTrue(ash_chrome_args[0].endswith(
           'build/lacros/prebuilt_ash_chrome/793554/chrome'))
-      self.assertListEqual([
+      expected_ash_chrome_args = [
-          '--user-data-dir=/tmp/ash-data', '--enable-wayland-server',
+          '--user-data-dir=/tmp/ash-data',
-          '--no-startup-window'
+          '--enable-wayland-server',
-      ], ash_chrome_args[1:])
+          '--no-startup-window',
+      ]
+      if command == 'lacros_chrome_browsertests':
+        expected_ash_chrome_args.append(
+            '--lacros-mojo-socket-for-testing=/tmp/ash-data/lacros.sock')
+      self.assertListEqual(expected_ash_chrome_args, ash_chrome_args[1:])
       ash_chrome_env = mock_popen.call_args_list[0][1].get('env', {})
       self.assertDictEqual({'XDG_RUNTIME_DIR': '/tmp/xdg'}, ash_chrome_env)
 
       test_args = mock_popen.call_args_list[1][0][0]
-      self.assertListEqual([command], test_args)
+      if command == 'lacros_chrome_browsertests':
+        self.assertListEqual([
+            command,
+            '--lacros-mojo-socket-for-testing=/tmp/ash-data/lacros.sock',
+            '--test-launcher-jobs=1'
+        ], test_args)
+      else:
+        self.assertListEqual([command], test_args)
+
       test_env = mock_popen.call_args_list[1][1].get('env', {})
       self.assertDictEqual(
           {
@@ -101,9 +116,11 @@ class TestRunnerTest(unittest.TestCase):
               'EGL_PLATFORM': 'surfaceless'
           }, test_env)
 
+
   @mock.patch.object(os,
                      'listdir',
                      return_value=['wayland-0', 'wayland-0.lock'])
+  @mock.patch.object(os.path, 'exists', return_value=True)
   @mock.patch.object(os.path, 'isfile', return_value=True)
   @mock.patch.object(test_runner,
                      '_GetLatestVersionOfAshChrome',
@@ -123,6 +140,7 @@ class TestRunnerTest(unittest.TestCase):
   @mock.patch.object(os,
                      'listdir',
                      return_value=['wayland-0', 'wayland-0.lock'])
+  @mock.patch.object(os.path, 'exists', return_value=True)
   @mock.patch.object(os.path, 'isfile', return_value=True)
   @mock.patch.object(test_runner, '_DownloadAshChromeIfNecessary')
   @mock.patch.object(subprocess, 'Popen', return_value=mock.Mock())