fix

Author: yifanyang

ci/fireci/fireciplugins/macrobenchmark/commands.py

@@ -50,9 +50,7 @@ def macrobenchmark():
   type=click.Path(dir_okay=True, resolve_path=True, path_type=Path),
   default='macrobenchmark-output.json',
   show_default=True,
-  help='The file for saving macrobenchmark test output. If running locally, the file contains '
-       'the directory name of local test reports. If running remotely, the file contains Firebase '
-       'Test Lab results directory names.'
+  help='The file for saving macrobenchmark test output if running on Firebase Test Lab.'
 )
 @ci_command(group=macrobenchmark)
 def run(build_only: bool, local: bool, repeat: int, output: Path):

ci/fireci/fireciplugins/macrobenchmark/run/runner.py

@@ -16,6 +16,8 @@
 import json
 import logging
 import tempfile
+
+import click
 import yaml
 
 from .test_project_builder import TestProjectBuilder
@@ -34,25 +36,38 @@ async def start(build_only: bool, local: bool, repeat: int, output: Path):
   test_dir = _prepare_test_directory()
   template_project_dir = Path('health-metrics/benchmark/template')
 
-  test_projects = []
-  for test_config in config['test-apps']:
-    builder = TestProjectBuilder(test_config, test_dir, template_project_dir, product_versions)
-    test_projects.append(builder.build())
+  test_projects = [
+    TestProjectBuilder(
+      test_config,
+      test_dir,
+      template_project_dir,
+      product_versions,
+    ).build() for test_config in config['test-apps']]
 
   if not build_only:
     if local:
-      test_outputs = []
       for test_project in test_projects:
-        test_output = test_project.run_local(repeat)
-        test_outputs.append(test_output)
+        test_project.run_local(repeat)
     else:
-      remote_runs = [x.run_remote(repeat) for x in test_projects]
-      test_outputs = await asyncio.gather(*remote_runs)
+      remote_runs = [test_project.run_remote(repeat) for test_project in test_projects]
+      results = await asyncio.gather(*remote_runs, return_exceptions=True)
+      test_outputs = [x for x in results if not isinstance(x, Exception)]
+      exceptions = [x for x in results if isinstance(x, Exception)]
+
+      with open(output, 'w') as file:
+        json.dump(test_outputs, file)
+        logger.info(f'Output of remote testing saved to: {output}')
+
+      if exceptions:
+        logger.error(f'Exceptions occurred: {exceptions}')
+      for test_output in test_outputs:
+        if test_output['exceptions']:
+          logger.error(f'Exceptions occurred: {test_output["exceptions"]}')
 
-    with open(output, 'w') as file:
-      json.dump(test_outputs, file)
+      if exceptions or any(test_output['exceptions'] for test_output in test_outputs):
+        raise click.ClickException('Macrobenchmark test failed with above exceptions')
 
-  logger.info(f'Completed macrobenchmark test and saved output to "{output}"')
+  logger.info(f'Completed macrobenchmark test successfully')
 
 
 def _assemble_all_products() -> dict[str, str]:

ci/fireci/fireciplugins/macrobenchmark/run/test_project.py

@@ -16,6 +16,7 @@
 import glob
 import re
 import shutil
+import random
 
 from .log_decorator import LogDecorator
 from .utils import execute, execute_async, generate_test_run_id
@@ -27,10 +28,10 @@
 logger = getLogger('fireci.macrobenchmark')
 
 
-class TestOutput(TypedDict, total=False):
+class RemoteTestOutput(TypedDict, total=False):
   project: str
-  local_reports_dir: str
-  ftl_results_dirs: list[str]
+  successful_runs: list[str]
+  exceptions: list[str]  # Using str due to Exception being not JSON serializable
 
 
 class TestProject:
@@ -39,7 +40,7 @@ def __init__(self, name: str, project_dir: Path, custom_logger: Logger | LoggerA
     self.test_project_dir = project_dir
     self.logger = custom_logger
 
-  def run_local(self, repeat: int) -> TestOutput:
+  def run_local(self, repeat: int):
     self.logger.info(f'Running test locally for {repeat} times ...')
     local_reports_dir = self.test_project_dir.joinpath('_reports')
 
@@ -59,10 +60,9 @@ def run_local(self, repeat: int) -> TestOutput:
           shutil.copy(report, device_dir)
           run_logger.debug(f'Copied report file "{report}" to "{device_dir}"')
 
-    self.logger.info(f'Completed all {repeat} runs, reports saved at "{local_reports_dir}"')
-    return TestOutput(project=self.name, local_reports_dir=str(local_reports_dir))
+    self.logger.info(f'Finished all {repeat} runs, local reports dir: "{local_reports_dir}"')
 
-  async def run_remote(self, repeat: int) -> TestOutput:
+  async def run_remote(self, repeat: int) -> RemoteTestOutput:
     self.logger.info(f'Running test remotely for {repeat} times ...')
 
     with chdir(self.test_project_dir):
@@ -71,9 +71,9 @@ async def run_remote(self, repeat: int) -> TestOutput:
       test_apk_path = glob.glob('**/macrobenchmark-benchmark.apk', recursive=True)[0]
       self.logger.info(f'App apk: "{app_apk_path}", Test apk: "{test_apk_path}"')
 
-      async def run(index: int, results_dir: str):
+      async def run(index: int, run_id: str) -> str:
         run_logger = LogDecorator(self.logger, f'run-{index}')
-        run_logger.info(f'Run-{index}: {results_dir}')
+        run_logger.info(f'Run-{index}: {run_id}')
         ftl_environment_variables = [
           'clearPackageData=true',
           'additionalTestOutputDir=/sdcard/Download',
@@ -84,19 +84,25 @@ async def run(index: int, results_dir: str):
         args += ['--type', 'instrumentation']
         args += ['--app', app_apk_path]
         args += ['--test', test_apk_path]
-        args += ['--device', 'model=f2q,version=30,locale=en,orientation=portrait']
         args += ['--device', 'model=oriole,version=32,locale=en,orientation=portrait']
-        args += ['--device', 'model=redfin,version=30,locale=en,orientation=portrait']
         args += ['--directories-to-pull', '/sdcard/Download']
         args += ['--results-bucket', 'fireescape-benchmark-results']
-        args += ['--results-dir', results_dir]
+        args += ['--results-dir', run_id]
         args += ['--environment-variables', ','.join(ftl_environment_variables)]
         args += ['--timeout', '30m']
         args += ['--project', 'fireescape-c4819']
         await execute_async(executable, *args, logger=run_logger)
+        return run_id
+
+      runs = [run(i, generate_test_run_id()) for i in range(repeat)]
+      results = await asyncio.gather(*runs, return_exceptions=True)
+      successes = [x for x in results if not isinstance(x, Exception)]
+      exceptions = [x for x in results if isinstance(x, Exception)]
+
+    self.logger.info(f'Finished all {repeat} runs, successes: {successes}, failures: {exceptions}')
 
-      ftl_results_dirs = [generate_test_run_id() for _ in range(repeat)]
-      runs = [run(i, ftl_results_dirs[i]) for i in range(repeat)]
-      await asyncio.gather(*runs)
-    self.logger.info(f'Completed all {repeat} runs, ftl results dirs: {ftl_results_dirs}')
-    return TestOutput(project=self.name, ftl_results_dirs=ftl_results_dirs)
+    return RemoteTestOutput(
+      project=self.name,
+      successful_runs=successes,
+      exceptions=[str(e) for e in exceptions]
+    )

ci/fireci/fireciplugins/macrobenchmark/run/utils.py

@@ -12,7 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import click
 import datetime
 import string
 import random
@@ -45,7 +44,7 @@ def execute(program: str, *args: [str], logger: Logger | LoggerAdapter) -> None:
   else:
     message = f'"{command}" failed with return code {popen.returncode}'
     logger.error(message)
-    raise click.ClickException(message)
+    raise RuntimeError(message)
 
 
 async def execute_async(program: str, *args: [str], logger: Logger | LoggerAdapter) -> None:
@@ -62,4 +61,4 @@ async def execute_async(program: str, *args: [str], logger: Logger | LoggerAdapt
   else:
     message = f'"{command}" failed with return code {process.returncode}'
     logger.error(message)
-    raise click.ClickException(message)
+    raise RuntimeError(message)