Fix --parallel=platform On Android, running multiple browsers on the same physical device results in creating different platform objects. This makes --parallel=platform useless, since is tries to run browsers in parallel on the same device and fails. This CL fixes that by introducing a platform key to identify the physical device and grouping runs by this key. TAG=agy Change-Id: If49744417819492ffe2da1c8d860d9dde5aacec1 Reviewed-on: https://chromium-review.googlesource.com/c/crossbench/+/7889317 Commit-Queue: Mikhail Khokhlov <khokhlov@google.com> Reviewed-by: Camillo Bruni <cbruni@chromium.org>
diff --git a/crossbench/browsers/chrome/downloader.py b/crossbench/browsers/chrome/downloader.py index 167cd5e..0674605 100644 --- a/crossbench/browsers/chrome/downloader.py +++ b/crossbench/browsers/chrome/downloader.py
@@ -115,7 +115,8 @@ return self._find_milestone_archive_url() def _find_milestone_archive_url(self) -> tuple[BrowserVersion, str | None]: - platform = self.VERSION_URL_PLATFORM_LOOKUP.get(self._browser_platform.key) + platform = self.VERSION_URL_PLATFORM_LOOKUP.get( + self._browser_platform.type_key) if not platform: raise ValueError(f"Unsupported platform {self._browser_platform}") # Version ordering is: stable < beta < dev < canary < canary_asan
diff --git a/crossbench/browsers/chromium/driver_finder.py b/crossbench/browsers/chromium/driver_finder.py index 644bf45..bc69e18 100644 --- a/crossbench/browsers/chromium/driver_finder.py +++ b/crossbench/browsers/chromium/driver_finder.py
@@ -180,10 +180,12 @@ def _get_cft_url(self, milestone: int) -> tuple[str, str | None]: logging.debug("ChromeDriverFinder: Looking up chrome-for-testing version.") - platform_name: str | None = self.CFT_PLATFORM.get(self.host_platform.key) + platform_name: str | None = self.CFT_PLATFORM.get( + self.host_platform.type_key) if not platform_name: raise DriverNotFoundError( - f"Unsupported platform {self.host_platform.key} for chromedriver.") + f"Unsupported platform {self.host_platform.type_key} " + "for chromedriver.") listing_url, version_data = self._get_cft_version_data(milestone) download_url: str | None = None if version_data: @@ -332,10 +334,10 @@ def _get_canary_url(self) -> tuple[str, str | None]: logging.debug( "ChromeDriverFinder: Try downloading the chromedriver canary version") - properties = self.CHROMIUM_DASH_PARAMS.get(self.host_platform.key) + properties = self.CHROMIUM_DASH_PARAMS.get(self.host_platform.type_key) if not properties: - raise DriverNotFoundError( - f"Unsupported platform={self.platform}, key={self.host_platform.key}") + raise DriverNotFoundError(f"Unsupported platform={self.platform}, " + f"type_key={self.host_platform.type_key}") dash_platform = properties["dash_platform"] dash_channel = properties.get("dash_channel", "canary") # Limit should be > len(canary_versions) so we also get potentially @@ -384,7 +386,8 @@ f"for {self.browser.version}") # Use prefixes to limit listing results and increase chances of finding # a matching version - listing_prefix = self.CHROMIUM_LISTING_PREFIX.get(self.host_platform.key) + listing_prefix = self.CHROMIUM_LISTING_PREFIX.get( + self.host_platform.type_key) if not listing_prefix: raise NotImplementedError( f"Unsupported chromedriver platform {self.host_platform}")
diff --git a/crossbench/browsers/firefox/downloader.py b/crossbench/browsers/firefox/downloader.py index eb6a047..252ea12 100644 --- a/crossbench/browsers/firefox/downloader.py +++ b/crossbench/browsers/firefox/downloader.py
@@ -74,7 +74,7 @@ reinstall: bool = False) -> None: assert not browser_type, f"Unexpected browser_type: {browser_type}" assert not platform_name, f"Unexpected platform_name: {platform_name}" - firefox_platform_name = _PLATFORM_NAME_LOOKUP.get(browser_platform.key) + firefox_platform_name = _PLATFORM_NAME_LOOKUP.get(browser_platform.type_key) if not firefox_platform_name: raise ValueError( "Unsupported macOS architecture for downloading Firefox: "
diff --git a/crossbench/plt/android_adb.py b/crossbench/plt/android_adb.py index c5b25c2..3530446 100644 --- a/crossbench/plt/android_adb.py +++ b/crossbench/plt/android_adb.py
@@ -668,6 +668,11 @@ def serial_id(self) -> str: return self._adb.serial_id + @property + @override + def key(self) -> tuple[Any, ...]: + return ("android", self.serial_id) + @functools.cached_property def _uiautomator_device(self) -> android_device.AndroidDevice: ad = android_device.AndroidDevice(self.serial_id)
diff --git a/crossbench/plt/base.py b/crossbench/plt/base.py index 59d7463..4888f7f 100644 --- a/crossbench/plt/base.py +++ b/crossbench/plt/base.py
@@ -151,8 +151,7 @@ def unique_name(self) -> str: """Unique id per platform.""" key_str = ".".join(self.key) - remote_str = "remote" if self.is_remote else "local" - return f"{key_str}.{remote_str}.{self.id}" + return f"{key_str}.{self.id}" @property @abc.abstractmethod @@ -198,6 +197,11 @@ return self.unique_name @property + def key(self) -> tuple[Any, ...]: + return ("local", self.name, str(self.machine)) + + @property + def is_remote(self) -> bool: return False @@ -222,7 +226,6 @@ return MachineArch.ARM_32 raise NotImplementedError(f"Unsupported machine type: {raw}") - @functools.lru_cache(maxsize=1) def _raw_machine_arch(self) -> str: self.assert_is_local() return py_platform.machine() @@ -240,7 +243,7 @@ return self.machine == MachineArch.ARM_64 @property - def key(self) -> tuple[str, str]: + def type_key(self) -> tuple[str, str]: """Key used for looking up platform specific objects.""" return (self.name, str(self.machine))
diff --git a/crossbench/plt/ios.py b/crossbench/plt/ios.py index 9f4aa12..6e9e5eb 100644 --- a/crossbench/plt/ios.py +++ b/crossbench/plt/ios.py
@@ -149,7 +149,6 @@ # TODO: Can we get actual iOS uptime? return dt.timedelta() - @functools.lru_cache(maxsize=1) @override def _raw_machine_arch(self) -> str: return "arm64"
diff --git a/crossbench/plt/posix.py b/crossbench/plt/posix.py index 8f3ca6a..291a134 100644 --- a/crossbench/plt/posix.py +++ b/crossbench/plt/posix.py
@@ -70,7 +70,6 @@ def version(self) -> PlatformVersion: return PosixVersion.parse(self.version_str) - @functools.lru_cache(maxsize=1) def _raw_machine_arch(self) -> str: if self.is_local: return super()._raw_machine_arch()
diff --git a/crossbench/plt/ssh.py b/crossbench/plt/ssh.py index 568366b..61b3d2f 100644 --- a/crossbench/plt/ssh.py +++ b/crossbench/plt/ssh.py
@@ -5,7 +5,7 @@ from __future__ import annotations import abc -from typing import TYPE_CHECKING, Final, Mapping +from typing import TYPE_CHECKING, Any, Final, Mapping from crossbench.plt.port_manager import PortManager from crossbench.plt.remote import RemotePlatformMixin @@ -56,6 +56,10 @@ def is_remote_ssh(self) -> bool: return True + @property + def key(self) -> tuple[Any, ...]: + return ("ssh", self.host, str(self.port)) + @abc.abstractmethod def build_ssh_cmd(self, *args: CmdArg,
diff --git a/crossbench/plt/win.py b/crossbench/plt/win.py index c661017..ce898de 100644 --- a/crossbench/plt/win.py +++ b/crossbench/plt/win.py
@@ -84,7 +84,6 @@ # https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/taskkill self.sh("taskkill", "/F", "/IM", process_name, check=False) - @functools.lru_cache(maxsize=1) @override def _raw_machine_arch(self) -> str: self.assert_is_local()
diff --git a/crossbench/probes/cb_perfetto/downloader.py b/crossbench/probes/cb_perfetto/downloader.py index 8a3979c..070340f 100644 --- a/crossbench/probes/cb_perfetto/downloader.py +++ b/crossbench/probes/cb_perfetto/downloader.py
@@ -49,7 +49,7 @@ @property def url(self) -> str: # TODO: use new platform.lookup helper. - platform_name = PLATFORM_LOOKUP[self._platform.key] + platform_name = PLATFORM_LOOKUP[self._platform.type_key] return f"{_BASE_STORAGE_URL}/{self._version}/{platform_name}/{self._tool}" @property
diff --git a/crossbench/runner/runner.py b/crossbench/runner/runner.py index f96aef2..8666868 100644 --- a/crossbench/runner/runner.py +++ b/crossbench/runner/runner.py
@@ -41,7 +41,6 @@ from crossbench.action_runner.base import ActionRunner from crossbench.benchmarks.base import Benchmark from crossbench.browsers.browser import Browser - from crossbench.plt.base import Platform from crossbench.probes.thermal_monitor import ThermalStatus from crossbench.runner.groups.base import RunGroup from crossbench.runner.timing import AnyTimeUnit @@ -89,7 +88,7 @@ runs, key=lambda run: run.browser_session, sort_key=None) elif self == ThreadMode.PLATFORM: groups = collection_helper.group_by( - runs, key=lambda run: run.browser_platform, sort_key=None) + runs, key=lambda run: run.browser_platform.key, sort_key=None) elif self == ThreadMode.BROWSER: groups = collection_helper.group_by( runs, key=lambda run: run.browser, sort_key=None) @@ -621,8 +620,9 @@ def has_only_single_run_platforms(self) -> bool: if not self.runs: raise RuntimeError(f"{type(self)} has no runs") - platform_runs: dict[Platform, list[Run]] = collection_helper.group_by( - self.runs, key=lambda run: run.browser_platform) + platform_runs: dict[tuple[Any, ...], + list[Run]] = collection_helper.group_by( + self.runs, key=lambda run: run.browser_platform.key) return all(len(runs) <= 1 for runs in platform_runs.values()) def _get_runs(self) -> Iterable[Run]:
diff --git a/tests/crossbench/cli/config/test_browser_variants.py b/tests/crossbench/cli/config/test_browser_variants.py index 840cd23..401b1d3 100644 --- a/tests/crossbench/cli/config/test_browser_variants.py +++ b/tests/crossbench/cli/config/test_browser_variants.py
@@ -105,7 +105,7 @@ self.assertGreaterEqual(len(config.variants), 1) def _expect_sh_linux_ssh_browser_config(self): - self._expect_linux_ssh("uname -m", result="arm64") + pass def _expect_sh_linux_ssh_browser_instance(self): self._expect_linux_ssh("'[' -e /path/to/google/chrome ']'") @@ -116,7 +116,6 @@ def _expect_sh_chromeos_ssh_browser_config(self): self._expect_chromeos_ssh("'[' -e /usr/local/autotest/bin/autologin.py ']'") - self._expect_chromeos_ssh("uname -m", result="arm64") def _expect_sh_chromeos_ssh_browser_instance(self): self._expect_chromeos_ssh("'[' -e /opt/google/chrome/chrome ']'") @@ -259,7 +258,7 @@ return_value=plt.MachineArch.ARM_64): variants = BrowserVariantsConfig.parse_args(args).variants self.assertEqual(len(variants), 2) - self.assertEqual(variants[0].label, "android.arm64.remote.777_0") + self.assertEqual(variants[0].label, "android.emulator-5556.777_0") self.assertEqual(variants[1].label, f"{self.platform}_1") with self.assertRaises(ConfigError) as cm:
diff --git a/tests/crossbench/plt/test_android_adb.py b/tests/crossbench/plt/test_android_adb.py index 4282268..aba4fe6 100644 --- a/tests/crossbench/plt/test_android_adb.py +++ b/tests/crossbench/plt/test_android_adb.py
@@ -165,8 +165,6 @@ }) def test_unique_name(self): - self.expect_sh("getprop ro.product.cpu.abi", result="arm64-v8a") - self.expect_sh("getprop ro.product.cpu.abi", result="arm64-v8a") platform_2 = AndroidAdbPlatform( self.host_platform, "SomeDeviceId", adb=self.adb) self.assertNotEqual(self.platform.unique_name, platform_2.unique_name)
diff --git a/tests/crossbench/plt/test_platform_equality.py b/tests/crossbench/plt/test_platform_equality.py new file mode 100644 index 0000000..c1dccf9 --- /dev/null +++ b/tests/crossbench/plt/test_platform_equality.py
@@ -0,0 +1,98 @@ +# Copyright 2026 The Chromium Authors +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +from __future__ import annotations + +import unittest +from unittest import mock + +from crossbench import path as pth +from crossbench import plt +from crossbench.plt.android_adb import Adb, AndroidAdbPlatform +from crossbench.plt.linux_ssh import LinuxSshPlatform +from tests import test_helper +from tests.crossbench.mock_helper import LinuxMockPlatform, MacOsMockPlatform + + +class PlatformEqualityTestCase(unittest.TestCase): + + def test_local_platform_equality(self) -> None: + platform_1 = LinuxMockPlatform() + platform_2 = LinuxMockPlatform() + self.assertNotEqual(platform_1, platform_2) + self.assertEqual(platform_1.key, platform_2.key) + + platform_3 = MacOsMockPlatform() + self.assertNotEqual(platform_1.key, platform_3.key) + + def test_android_platform_equality(self) -> None: + host_platform = mock.MagicMock(spec=plt.Platform) + host_platform.is_remote = False + + adb_1 = mock.MagicMock(spec=Adb) + adb_1.serial_id = "device_1" + platform_1 = AndroidAdbPlatform(host_platform, adb=adb_1) + + adb_2 = mock.MagicMock(spec=Adb) + adb_2.serial_id = "device_1" + platform_2 = AndroidAdbPlatform(host_platform, adb=adb_2) + + adb_3 = mock.MagicMock(spec=Adb) + adb_3.serial_id = "device_2" + platform_3 = AndroidAdbPlatform(host_platform, adb=adb_3) + + self.assertNotEqual(platform_1, platform_2) + self.assertEqual(platform_1.key, platform_2.key) + self.assertNotEqual(platform_1.key, platform_3.key) + + def test_ssh_platform_equality(self) -> None: + host_platform = mock.MagicMock(spec=plt.Platform) + host_platform.is_remote = False + + # Patch _create_default_tmp_dir to avoid running remote command on init + with mock.patch.object( + LinuxSshPlatform, + "_create_default_tmp_dir", + return_value=pth.AnyPath("/tmp"), + ): + platform_1 = LinuxSshPlatform( + host_platform, host="host1", port=22, ssh_port=22, ssh_user="user") + platform_2 = LinuxSshPlatform( + host_platform, host="host1", port=22, ssh_port=22, ssh_user="user") + platform_3 = LinuxSshPlatform( + host_platform, host="host2", port=22, ssh_port=22, ssh_user="user") + platform_4 = LinuxSshPlatform( + host_platform, host="host1", port=2222, ssh_port=22, ssh_user="user") + + self.assertNotEqual(platform_1, platform_2) + self.assertEqual(platform_1.key, platform_2.key) + + self.assertNotEqual(platform_1.key, platform_3.key) + self.assertNotEqual(platform_1.key, platform_4.key) + + def test_different_platform_types_not_equal(self) -> None: + host_platform = mock.MagicMock(spec=plt.Platform) + host_platform.is_remote = False + + local_platform = LinuxMockPlatform() + + adb = mock.MagicMock(spec=Adb) + adb.serial_id = "device_1" + android_platform = AndroidAdbPlatform(host_platform, adb=adb) + + with mock.patch.object( + LinuxSshPlatform, + "_create_default_tmp_dir", + return_value=pth.AnyPath("/tmp"), + ): + ssh_platform = LinuxSshPlatform( + host_platform, host="host1", port=22, ssh_port=22, ssh_user="user") + + self.assertNotEqual(local_platform.key, android_platform.key) + self.assertNotEqual(local_platform.key, ssh_platform.key) + self.assertNotEqual(android_platform.key, ssh_platform.key) + + +if __name__ == "__main__": + test_helper.run_pytest(__file__)
diff --git a/tests/crossbench/probes/test_cpu_frequency_map.py b/tests/crossbench/probes/test_cpu_frequency_map.py index d9c0630..c7516fa 100644 --- a/tests/crossbench/probes/test_cpu_frequency_map.py +++ b/tests/crossbench/probes/test_cpu_frequency_map.py
@@ -61,7 +61,8 @@ self._create_cpu_dir("cpu1", [42]) with self.assertRaisesRegex( - ValueError, r"Target frequency 42 for cpu0 not allowed in linux.*. " + ValueError, + r"Target frequency 42 for cpu0 not allowed in local.linux.*. " r"Available frequencies: \[1, 2\]"): frequency_map.get_target_frequencies(self.platform) @@ -71,7 +72,8 @@ self._create_cpu_dir("cpu0", [1, 2]) with self.assertRaisesRegex( - ValueError, r"Target frequency 42 for cpu0 not allowed in linux.*. " + ValueError, + r"Target frequency 42 for cpu0 not allowed in local.linux.*. " r"Available frequencies: \[1, 2\]"): frequency_map.get_target_frequencies(self.platform)
diff --git a/tests/crossbench/runner/helper.py b/tests/crossbench/runner/helper.py index 9a00427..7b90614 100644 --- a/tests/crossbench/runner/helper.py +++ b/tests/crossbench/runner/helper.py
@@ -222,6 +222,10 @@ def __str__(self): return self.name + @property + def key(self) -> str: + return self.name + class MockWait(NamedTuple): time: AnyTimeUnit
diff --git a/tests/crossbench/runner/test_runner.py b/tests/crossbench/runner/test_runner.py index f5e0c51..a02b214 100644 --- a/tests/crossbench/runner/test_runner.py +++ b/tests/crossbench/runner/test_runner.py
@@ -465,8 +465,15 @@ def test_has_only_single_run_platforms_multi_platform(self): benchmark = MockBenchmark((self.stories[0],)) + + class RemoteMockPlatform(FullMockPlatform): + + @property + def key(self) -> tuple[str, ...]: + return ("remote", "remote-mock-platform") + mock_remote_chrome = MockChromeDev( - "chrome-dev_remote", settings=Settings(platform=FullMockPlatform())) + "chrome-dev_remote", settings=Settings(platform=RemoteMockPlatform())) browsers = (self.browsers[0], mock_remote_chrome) runner = self.default_runner(browsers=browsers, benchmark=benchmark) runner.run() @@ -474,8 +481,15 @@ self.assertTrue(runner.has_only_single_run_platforms()) def test_has_only_single_run_platforms_multi_platform_stories(self): + + class RemoteMockPlatform(FullMockPlatform): + + @property + def key(self) -> tuple[str, ...]: + return ("remote", "remote-mock-platform") + mock_remote_chrome = MockChromeDev( - "chrome-dev_remote", settings=Settings(platform=FullMockPlatform())) + "chrome-dev_remote", settings=Settings(platform=RemoteMockPlatform())) browsers = (self.browsers[0], mock_remote_chrome) runner = self.default_runner(browsers=browsers) runner.run()