From 918776af1ee268cce673cb8b0aae159f4be11f24 Mon Sep 17 00:00:00 2001 From: Arjun Chaturvedi Date: Wed, 27 May 2026 11:59:33 -0700 Subject: [PATCH 1/3] Add unit tests for runner subclasses Summary: Add per-subclass tests pinning binary names, conditional flag construction, filter behavior, and warmboot-script dispatch. - test_sai_agent_runner.py (10): mono vs multi_switch branching for warmboot file, replayer flags, run-args; production-feature filtering in `_filter_tests`; warmboot-script dispatch via `_setup_warmboot_test`. - test_link_runner.py (5): mono vs multi binary, warmboot file by mode, service start order (FSDB -> QSFP -> HW Agent), --disable-fsdb skip. - test_platform_services_runner.py (8): parametrized type -> binary map with fallback; multi-type iteration when --type omitted. - test_bcm_runner.py (3, NEW): default constants; `_get_sai_logging_flags` no-arg signature. - test_sai_runner.py (6, NEW): 5-flag SAI replayer set construction; optional --platform_mapping_override_path append; known-bad-tests file override fallback to SAI_HW_KNOWN_BAD_TESTS. - test_qsfp_runner.py (5, NEW): conditional --platform_mapping_override_path and --bsp_platform_mapping_override_path appends; coldboot wipes QSFP_SERVICE_DIR via subprocess.Popen. Differential Revision: D106428878 --- .../oss/scripts/run_scripts/tests/conftest.py | 8 + .../run_scripts/tests/test_bcm_runner.py | 31 ++++ .../tests/test_fboss_agent_utils.py | 4 +- .../run_scripts/tests/test_link_runner.py | 126 ++++++++++++++ .../tests/test_platform_services_runner.py | 92 +++++++++++ .../run_scripts/tests/test_qsfp_runner.py | 64 +++++++ .../tests/test_sai_agent_runner.py | 156 ++++++++++++++++++ .../run_scripts/tests/test_sai_runner.py | 68 ++++++++ 8 files changed, 547 insertions(+), 2 deletions(-) create mode 100644 fboss/oss/scripts/run_scripts/tests/test_bcm_runner.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_link_runner.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_platform_services_runner.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_qsfp_runner.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_sai_agent_runner.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_sai_runner.py diff --git a/fboss/oss/scripts/run_scripts/tests/conftest.py b/fboss/oss/scripts/run_scripts/tests/conftest.py index dd9022d50c709..f089d8136760f 100644 --- a/fboss/oss/scripts/run_scripts/tests/conftest.py +++ b/fboss/oss/scripts/run_scripts/tests/conftest.py @@ -14,8 +14,16 @@ from argparse import ArgumentParser +import run_test from run_test import TestRunner +# `run_test.args` is populated at runtime by argparse inside main(), so the +# attribute does not exist at import time. Declare it here so unit tests can +# patch it without `create=True` — which means a rename of `args` in run_test.py +# will surface as a loud AttributeError at patch time instead of being silently +# fabricated by mock. +run_test.args = None + class StubTestRunner(TestRunner): """Concrete TestRunner with all abstract methods stubbed for testing.""" diff --git a/fboss/oss/scripts/run_scripts/tests/test_bcm_runner.py b/fboss/oss/scripts/run_scripts/tests/test_bcm_runner.py new file mode 100644 index 0000000000000..979895f0b6f5e --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_bcm_runner.py @@ -0,0 +1,31 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for BcmTestRunner.""" + +import pytest +from run_test import BcmTestRunner + + +@pytest.fixture +def bcm_runner(): + return BcmTestRunner() + + +class TestConstants: + def test_default_paths_and_binary(self, bcm_runner): + assert bcm_runner._get_config_path() == "/etc/coop/bcm.conf" + assert bcm_runner._get_test_binary_name() == "/opt/fboss/bin/bcm_test" + assert bcm_runner._get_known_bad_tests_file() == "" + assert bcm_runner._get_unsupported_tests_file() == "" + assert bcm_runner._get_sai_replayer_logging_flags("/any/path") == [] + assert bcm_runner._get_sai_replayer_logging_flags(None) == [] + assert bcm_runner._get_test_run_args("dummy.conf") == [] + + +class TestSaiLoggingFlagsSignature: + def test_no_arg_signature_returns_empty(self, bcm_runner): + assert bcm_runner._get_sai_logging_flags() == [] + + def test_calling_with_sai_logging_arg_raises_typeerror(self, bcm_runner): + with pytest.raises(TypeError): + bcm_runner._get_sai_logging_flags("WARN") diff --git a/fboss/oss/scripts/run_scripts/tests/test_fboss_agent_utils.py b/fboss/oss/scripts/run_scripts/tests/test_fboss_agent_utils.py index c437662769763..0c2bf0b38415e 100644 --- a/fboss/oss/scripts/run_scripts/tests/test_fboss_agent_utils.py +++ b/fboss/oss/scripts/run_scripts/tests/test_fboss_agent_utils.py @@ -64,7 +64,7 @@ def test_multi_npu(self, mock_run): @patch("fboss_agent_utils.subprocess.run") def test_raises_on_stop_failure(self, mock_run): - def side_effect(cmd, **kwargs): + def side_effect(cmd, **_kwargs): if "systemctl stop fboss_sw_agent" in cmd: return _mock_run(1) return _mock_run(0) @@ -102,7 +102,7 @@ def test_sw_down(self, mock_run): @patch("fboss_agent_utils.subprocess.run") def test_hw_down(self, mock_run): - def side_effect(cmd, **kwargs): + def side_effect(cmd, **_kwargs): if "fboss_sw_agent" in cmd: return _mock_run(0) return _mock_run(3) diff --git a/fboss/oss/scripts/run_scripts/tests/test_link_runner.py b/fboss/oss/scripts/run_scripts/tests/test_link_runner.py new file mode 100644 index 0000000000000..494b2c5cc5fab --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_link_runner.py @@ -0,0 +1,126 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for LinkTestRunner. + +Covers the mono vs multi_switch branching for binary selection and warmboot +file path, plus the service start ordering in _setup_coldboot_test which +must launch FSDB before QSFP before HW Agent.""" + +from unittest.mock import MagicMock, patch + +import pytest +from run_test import LinkTestRunner + + +@pytest.fixture +def link_runner(): + return LinkTestRunner() + + +def _make_args(**overrides): + args = MagicMock() + args.agent_run_mode = "multi_switch" + args.num_npus = 1 + args.platform_mapping_override_path = None + args.bsp_platform_mapping_override_path = None + args.disable_fsdb = False + args.fsdb_config = None + args.qsfp_config = None + args.config = None + args.mgmt_if = "eth0" + args.known_bad_tests_file = None + for k, v in overrides.items(): + setattr(args, k, v) + return args + + +class TestBinaryNameByMode: + def test_mono_uses_mono_link_test_binary(self, link_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="mono")): + assert link_runner._get_test_binary_name().endswith( + "mono_link_test-sai_impl" + ) + + def test_multi_switch_uses_multi_link_test_binary(self, link_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="multi_switch")): + assert link_runner._get_test_binary_name().endswith( + "multi_link_test-sai_impl" + ) + + +class TestWarmbootCheckFileByMode: + """mono uses _0 suffix, multi_switch uses no suffix. + Wrong path silently skips warmboot.""" + + def test_mono_uses_switch_index_zero(self, link_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="mono")): + assert link_runner._get_warmboot_check_file().endswith("_0") + + def test_multi_switch_uses_no_switch_index(self, link_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="multi_switch")): + assert not link_runner._get_warmboot_check_file().endswith("_0") + + +class TestSetupColdbootServiceOrder: + """Production correctness: services must start FSDB → QSFP → HW Agent. + Reordering breaks downstream subscriptions (QSFP needs FSDB up first). + HW Agent only starts in multi_switch mode.""" + + def test_multi_switch_starts_services_in_order(self, link_runner): + call_order = [] + with ( + patch( + "run_test.args", + new=_make_args(agent_run_mode="multi_switch"), + ), + patch( + "run_test.setup_and_start_fsdb_service", + side_effect=lambda **_: call_order.append("fsdb"), + ), + patch( + "run_test.setup_and_start_qsfp_service", + side_effect=lambda **_: call_order.append("qsfp"), + ), + patch( + "run_test.setup_and_start_hw_agent_service", + side_effect=lambda **_: call_order.append("hw_agent"), + ), + ): + link_runner._setup_coldboot_test() + assert call_order == ["fsdb", "qsfp", "hw_agent"] + + def test_mono_does_not_start_hw_agent_service(self, link_runner): + # mono mode keeps the HW switch in-process; no separate HW Agent service. + call_order = [] + with ( + patch("run_test.args", new=_make_args(agent_run_mode="mono")), + patch( + "run_test.setup_and_start_fsdb_service", + side_effect=lambda **_: call_order.append("fsdb"), + ), + patch( + "run_test.setup_and_start_qsfp_service", + side_effect=lambda **_: call_order.append("qsfp"), + ), + patch( + "run_test.setup_and_start_hw_agent_service", + ) as mock_hw_agent, + ): + link_runner._setup_coldboot_test() + assert call_order == ["fsdb", "qsfp"] + mock_hw_agent.assert_not_called() + + def test_disable_fsdb_skips_fsdb_startup(self, link_runner): + with ( + patch( + "run_test.args", + new=_make_args(agent_run_mode="mono", disable_fsdb=True), + ), + patch("run_test.setup_and_start_fsdb_service") as mock_fsdb, + patch("run_test.setup_and_start_qsfp_service") as mock_qsfp, + ): + link_runner._setup_coldboot_test() + mock_fsdb.assert_not_called() + # Catches over-skipping: an early-return in _setup_coldboot_test that + # accidentally drops qsfp too must fail this test, not pass silently. + mock_qsfp.assert_called() diff --git a/fboss/oss/scripts/run_scripts/tests/test_platform_services_runner.py b/fboss/oss/scripts/run_scripts/tests/test_platform_services_runner.py new file mode 100644 index 0000000000000..d5bd616041f86 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_platform_services_runner.py @@ -0,0 +1,92 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for PlatformServicesTestRunner. + +The runner has two unique behaviors worth testing: (1) per-test-type binary +selection via a dict map with a fallback default, and (2) the multi-type +iteration in run_test that iterates TEST_TYPE_CHOICES when --type is omitted.""" + +from unittest.mock import MagicMock, patch + +import pytest +from run_test import PlatformServicesTestRunner + + +@pytest.fixture +def platform_runner(): + return PlatformServicesTestRunner() + + +def _make_args(**overrides): + args = MagicMock() + args.type = None + args.config = None + args.list_tests = False + args.skip_known_bad_tests = None + args.sai_logging = "WARN" + args.fboss_logging = "WARN" + args.test_run_timeout = 300 + args.run_on_reference_board = False + for k, v in overrides.items(): + setattr(args, k, v) + return args + + +class TestBinaryNameByType: + """The binary name comes from a dict map keyed on args.type. Unknown + types fall back to platform_hw_test.""" + + @pytest.mark.parametrize( + "test_type,expected", + [ + ("platform_hw_test", "platform_hw_test"), + ("data_corral_service_hw_test", "data_corral_service_hw_test"), + ("fan_service_hw_test", "fan_service_hw_test"), + ("fw_util_hw_test", "fw_util_hw_test"), + ("sensor_service_hw_test", "sensor_service_hw_test"), + ("weutil_hw_test", "weutil_hw_test"), + ("platform_manager_hw_test", "platform_manager_hw_test"), + ], + ) + def test_known_types_map_to_their_binary( + self, platform_runner, test_type, expected + ): + with patch("run_test.args", new=_make_args(type=test_type)): + assert platform_runner._get_test_binary_name() == expected + + def test_unknown_type_falls_back_to_platform_hw_test(self, platform_runner): + with patch("run_test.args", new=_make_args(type="not_a_real_type")): + assert platform_runner._get_test_binary_name() == "platform_hw_test" + + +class TestRunTestMultiTypeIteration: + """When --type is omitted, run_test iterates over all 7 TEST_TYPE_CHOICES, + setting args.type for each pass. This is the unique runner-level behavior + (vs the base TestRunner's single-type loop).""" + + def test_iterates_all_test_types_when_type_none(self, platform_runner): + seen_types = [] + # Spy on _get_tests_to_run to capture the type set at each iteration. + runner_args = _make_args(list_tests=False) + + def capture_type(): + seen_types.append(runner_args.type) + return ["FakeTest.A"] + + with ( + patch("run_test.args", new=runner_args), + patch.object(platform_runner, "_initialize_test_lists"), + patch.object( + platform_runner, "_get_tests_to_run", side_effect=capture_type + ), + patch.object(platform_runner, "_filter_tests", side_effect=lambda t: t), + patch.object( + platform_runner, "_backup_and_modify_config", side_effect=lambda c: c + ), + patch.object(platform_runner, "_run_tests", return_value=[]), + patch.object(platform_runner, "_print_output_summary"), + ): + platform_runner.run_test(runner_args) + + # All 7 platform test types are iterated, in the order of TEST_TYPE_CHOICES. + assert seen_types == list(PlatformServicesTestRunner.TEST_TYPE_CHOICES) diff --git a/fboss/oss/scripts/run_scripts/tests/test_qsfp_runner.py b/fboss/oss/scripts/run_scripts/tests/test_qsfp_runner.py new file mode 100644 index 0000000000000..c1e876675d632 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_qsfp_runner.py @@ -0,0 +1,64 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for QsfpTestRunner.""" + +from unittest.mock import MagicMock, patch + +import pytest +from run_test import QSFP_SERVICE_DIR, QsfpTestRunner + + +@pytest.fixture +def qsfp_runner(): + return QsfpTestRunner() + + +def _make_args(**overrides): + args = MagicMock() + args.qsfp_config = "/tmp/qsfp.conf" + args.platform_mapping_override_path = None + args.bsp_platform_mapping_override_path = None + args.known_bad_tests_file = None + for k, v in overrides.items(): + setattr(args, k, v) + return args + + +class TestRunArgsOverrides: + def test_no_overrides_only_qsfp_config(self, qsfp_runner): + with patch("run_test.args", new=_make_args()): + args_list = qsfp_runner._get_test_run_args("ignored.conf") + assert args_list == ["--qsfp-config", "/tmp/qsfp.conf"] + + def test_platform_mapping_override_appended_when_set(self, qsfp_runner): + with patch( + "run_test.args", + new=_make_args(platform_mapping_override_path="/tmp/pm.json"), + ): + args_list = qsfp_runner._get_test_run_args("ignored.conf") + idx = args_list.index("--platform_mapping_override_path") + assert args_list[idx + 1] == "/tmp/pm.json" + assert "--bsp_platform_mapping_override_path" not in args_list + + def test_both_overrides_appended_when_both_set(self, qsfp_runner): + with patch( + "run_test.args", + new=_make_args( + platform_mapping_override_path="/tmp/pm.json", + bsp_platform_mapping_override_path="/tmp/bsp.json", + ), + ): + args_list = qsfp_runner._get_test_run_args("ignored.conf") + pm_idx = args_list.index("--platform_mapping_override_path") + bsp_idx = args_list.index("--bsp_platform_mapping_override_path") + assert args_list[pm_idx + 1] == "/tmp/pm.json" + assert args_list[bsp_idx + 1] == "/tmp/bsp.json" + + +class TestColdbootCleansQsfpServiceDir: + def test_invokes_rm_rf_on_qsfp_service_dir(self, qsfp_runner): + with patch("run_test.subprocess.Popen") as mock_popen: + qsfp_runner._setup_coldboot_test() + mock_popen.assert_called_once() + cmd = mock_popen.call_args.args[0] + assert cmd == ["rm", "-rf", QSFP_SERVICE_DIR] diff --git a/fboss/oss/scripts/run_scripts/tests/test_sai_agent_runner.py b/fboss/oss/scripts/run_scripts/tests/test_sai_agent_runner.py new file mode 100644 index 0000000000000..fed276a735357 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_sai_agent_runner.py @@ -0,0 +1,156 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for SaiAgentTestRunner. + +Covers the mono vs multi_switch branching that drives binary, warmboot file, +SAI replayer flags, and platform-mapping override placement, plus the +production-feature filtering implemented in _filter_tests.""" + +import json +from unittest.mock import MagicMock, patch + +import pytest +from run_test import SaiAgentTestRunner + + +@pytest.fixture +def sai_agent_runner(): + return SaiAgentTestRunner() + + +def _make_args(**overrides): + args = MagicMock() + args.agent_run_mode = "multi_switch" + args.num_npus = 1 + args.platform_mapping_override_path = None + args.mgmt_if = "eth0" + args.enable_production_features = None + args.list_tests_for_features = None + args.production_features = None + args.known_bad_tests_file = None + args.unsupported_tests_file = None + for k, v in overrides.items(): + setattr(args, k, v) + return args + + +class TestWarmbootCheckFile: + """Mono uses switch_index=0 (can_warm_boot_0); multi_switch uses + switch_index=None (can_warm_boot). Mismatched paths silently skip the + warmboot.""" + + def test_mono_uses_switch_index_zero(self, sai_agent_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="mono")): + assert sai_agent_runner._get_warmboot_check_file().endswith("_0") + + def test_multi_switch_uses_no_switch_index(self, sai_agent_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="multi_switch")): + # multi_switch path must NOT have the _ suffix. + path = sai_agent_runner._get_warmboot_check_file() + assert not path.endswith("_0") + + +class TestSaiReplayerLoggingFlagsByMode: + """In multi_switch the SAI replayer is configured via the systemd unit + file (hw agent runs as a service), so the test binary must NOT receive + --enable-replayer flags. In mono the flags go on the binary.""" + + def test_multi_switch_returns_empty_when_path_set(self, sai_agent_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="multi_switch")): + assert sai_agent_runner._get_sai_replayer_logging_flags("/tmp/replay") == [] + + def test_mono_returns_replayer_flags_when_path_set(self, sai_agent_runner): + with patch("run_test.args", new=_make_args(agent_run_mode="mono")): + flags = sai_agent_runner._get_sai_replayer_logging_flags("/tmp/replay") + assert "--enable-replayer" in flags + assert "--sai-log" in flags + assert "/tmp/replay" in flags + + def test_returns_empty_when_path_none(self, sai_agent_runner): + # Regardless of mode, no path → no flags. + with patch("run_test.args", new=_make_args(agent_run_mode="mono")): + assert sai_agent_runner._get_sai_replayer_logging_flags(None) == [] + + +class TestTestRunArgsByMode: + """--platform_mapping_override_path is added to the binary args ONLY in + mono mode — multi_switch passes it via the systemd unit file instead.""" + + def test_mono_includes_platform_mapping_override(self, sai_agent_runner): + with patch( + "run_test.args", + new=_make_args( + agent_run_mode="mono", + platform_mapping_override_path="/tmp/pm.json", + ), + ): + args_list = sai_agent_runner._get_test_run_args("dummy.conf") + assert "--platform_mapping_override_path" in args_list + assert "/tmp/pm.json" in args_list + + def test_multi_switch_omits_platform_mapping_override(self, sai_agent_runner): + with patch( + "run_test.args", + new=_make_args( + agent_run_mode="multi_switch", + platform_mapping_override_path="/tmp/pm.json", + ), + ): + args_list = sai_agent_runner._get_test_run_args("dummy.conf") + # Override must NOT appear; multi_switch routes it via systemd unit. + assert "--platform_mapping_override_path" not in args_list + assert "/tmp/pm.json" not in args_list + + +class TestFilterTestsProductionFeatures: + """--enable-production-features ASIC: load asicToFeatureNames from the + JSON, then per-test invoke the binary with --list_production_feature and + keep tests whose feature set is a subset of the ASIC's features.""" + + def _features_json(self, tmp_path, mapping): + path = tmp_path / "features.json" + path.write_text(json.dumps({"asicToFeatureNames": mapping})) + return str(path) + + def test_keeps_tests_whose_features_are_subset_of_asic( + self, sai_agent_runner, tmp_path + ): + path = self._features_json( + tmp_path, {"tomahawk5": ["ACL_COUNTER", "DLB", "SINGLE_ACL_TABLE"]} + ) + + # First test reports features that are a subset → keep. + # Second test reports a feature NOT in the ASIC → drop. + def fake_run(cmd, **_): + test = next( + arg.split("=", 1)[1] for arg in cmd if arg.startswith("--gtest_filter=") + ) + if test == "AgentAclTest.OK": + stdout = "Feature List: ACL_COUNTER\n" + else: + stdout = "Feature List: NOT_SUPPORTED_FEATURE\n" + return MagicMock(stdout=stdout) + + with ( + patch( + "run_test.args", + new=_make_args( + agent_run_mode="multi_switch", + enable_production_features="tomahawk5", + production_features=path, + ), + ), + patch("run_test.subprocess.run", side_effect=fake_run), + ): + kept = sai_agent_runner._filter_tests( + ["AgentAclTest.OK", "AgentExoticTest.Bad"] + ) + assert kept == ["AgentAclTest.OK"] + + def test_no_filter_when_neither_flag_set(self, sai_agent_runner): + with patch( + "run_test.args", + new=_make_args(agent_run_mode="multi_switch"), + ): + tests = ["AgentA.t", "AgentB.t"] + assert sai_agent_runner._filter_tests(tests) == tests diff --git a/fboss/oss/scripts/run_scripts/tests/test_sai_runner.py b/fboss/oss/scripts/run_scripts/tests/test_sai_runner.py new file mode 100644 index 0000000000000..ad276b5bf9043 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_sai_runner.py @@ -0,0 +1,68 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for SaiTestRunner.""" + +from unittest.mock import MagicMock, patch + +import pytest +from run_test import SAI_HW_KNOWN_BAD_TESTS, SaiTestRunner + + +@pytest.fixture +def sai_runner(): + return SaiTestRunner() + + +def _make_args(**overrides): + args = MagicMock() + args.mgmt_if = "eth0" + args.platform_mapping_override_path = None + args.known_bad_tests_file = None + args.unsupported_tests_file = None + for k, v in overrides.items(): + setattr(args, k, v) + return args + + +class TestSaiReplayerLoggingFlags: + def test_path_set_emits_five_flags_in_order(self, sai_runner): + flags = sai_runner._get_sai_replayer_logging_flags("/tmp/replay.log") + assert flags == [ + "--enable-replayer", + "--enable_get_attr_log", + "--enable_packet_log", + "--sai-log", + "/tmp/replay.log", + ] + + def test_path_none_returns_empty(self, sai_runner): + assert sai_runner._get_sai_replayer_logging_flags(None) == [] + + +class TestRunArgsPlatformMappingOverride: + def test_omits_override_when_unset(self, sai_runner): + with patch("run_test.args", new=_make_args()): + args_list = sai_runner._get_test_run_args("conf.yaml") + assert args_list == ["--config", "conf.yaml", "--mgmt-if", "eth0"] + + def test_appends_override_when_set(self, sai_runner): + with patch( + "run_test.args", + new=_make_args(platform_mapping_override_path="/tmp/pm.json"), + ): + args_list = sai_runner._get_test_run_args("conf.yaml") + idx = args_list.index("--platform_mapping_override_path") + assert args_list[idx + 1] == "/tmp/pm.json" + + +class TestKnownBadTestsFileFallback: + def test_override_used_when_set(self, sai_runner): + with patch( + "run_test.args", + new=_make_args(known_bad_tests_file="/tmp/custom.json"), + ): + assert sai_runner._get_known_bad_tests_file() == "/tmp/custom.json" + + def test_default_used_when_unset(self, sai_runner): + with patch("run_test.args", new=_make_args()): + assert sai_runner._get_known_bad_tests_file() == SAI_HW_KNOWN_BAD_TESTS From a6dd818db973778df2274a666d8289b15942467c Mon Sep 17 00:00:00 2001 From: Arjun Chaturvedi Date: Wed, 27 May 2026 11:59:33 -0700 Subject: [PATCH 2/3] Add unit tests for service utility lifecycle (agent/FSDB/QSFP) Summary: Add lifecycle tests for the systemd service helpers wrapped by run_test.py runners. These pin the contract before the planned modular refactor moves these files into a `services/` subpackage. - test_agent_service.py (7): warmboot file path helper; cleanup stops the prod / for_testing / oss service variants and pkills each; setup raises on missing binary or missing config; setup_and_start dispatches warm vs cold; warm-boot file-missing behavior pinned for `warm_boot_hw_agent`. - test_fsdb_service.py (4): cleanup teardown, setup preconditions, cold-boot creates marker / warm-boot skips, start orchestration. - test_qsfp_service.py (5): cleanup teardown, setup preconditions, FSDB disabled vs enabled flips --fsdb_client_ssl_preferred=false in the unit file, --platform_mapping_override_path appended into ExecStart when set, cold/warm marker behavior. Differential Revision: D106437058 --- .../run_scripts/tests/test_agent_service.py | 127 +++++++++++++++++ .../run_scripts/tests/test_fsdb_service.py | 90 ++++++++++++ .../run_scripts/tests/test_qsfp_service.py | 133 ++++++++++++++++++ 3 files changed, 350 insertions(+) create mode 100644 fboss/oss/scripts/run_scripts/tests/test_agent_service.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_fsdb_service.py create mode 100644 fboss/oss/scripts/run_scripts/tests/test_qsfp_service.py diff --git a/fboss/oss/scripts/run_scripts/tests/test_agent_service.py b/fboss/oss/scripts/run_scripts/tests/test_agent_service.py new file mode 100644 index 0000000000000..14c4264305745 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_agent_service.py @@ -0,0 +1,127 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for fboss_agent_utils HW agent systemd service lifecycle.""" + +import os +import sys +from unittest.mock import ANY, MagicMock, patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +import fboss_agent_utils +from fboss_agent_utils import ( + agent_can_warm_boot_file_path, + cleanup_hw_agent_service, + setup_and_start_hw_agent_service, + warm_boot_hw_agent, +) + + +class TestAgentCanWarmBootFilePath: + def test_none_returns_base_path(self): + path = agent_can_warm_boot_file_path(switch_index=None) + assert path == fboss_agent_utils.FBOSS_AGENT_WB_FLAG_FILE + assert not path.endswith("_0") + + def test_index_appends_underscore_suffix(self): + assert agent_can_warm_boot_file_path(switch_index=0).endswith("_0") + assert agent_can_warm_boot_file_path(switch_index=3).endswith("_3") + + +class TestCleanupHwAgentService: + def test_stops_all_three_service_variants_and_pkills(self): + with patch.object(fboss_agent_utils.subprocess, "run") as mock_run: + cleanup_hw_agent_service([0]) + + joined = "\n".join(c.args[0] for c in mock_run.call_args_list) + assert "systemctl stop fboss_hw_agent@0" in joined + assert "systemctl stop fboss_hw_agent_for_testing_0" in joined + assert "systemctl stop fboss_hw_agent_oss@0" in joined + assert "systemctl disable fboss_hw_agent_oss@0" in joined + assert "systemctl daemon-reload" in joined + assert "pkill -f fboss_hw_agent@0" in joined + assert "pkill -f fboss_hw_agent_for_testing_0" in joined + assert "pkill -f fboss_hw_agent_oss@0" in joined + + +class TestSetupHwAgentServicePreconditions: + def test_raises_when_binary_missing(self, tmp_path): + config_path = tmp_path / "agent.conf" + config_path.write_text("") + with pytest.raises(Exception, match="HW Agent Service binary"): + setup_and_start_hw_agent_service( + switch_indexes=[0], + fboss_agent_config_path=str(config_path), + hw_agent_service_bin_path=str(tmp_path / "does_not_exist"), + ) + + def test_raises_when_config_missing(self, tmp_path): + with pytest.raises(Exception, match="Agent config path"): + setup_and_start_hw_agent_service( + switch_indexes=[0], + fboss_agent_config_path=str(tmp_path / "missing.conf"), + hw_agent_service_bin_path=sys.executable, + ) + + +class TestSetupAndStartDispatch: + # _setup_hw_agent_service writes systemd unit files to /tmp and rsyslog + # configs to /etc/rsyslog.d (which fails for non-root and is a real-system + # side effect). The tests here only verify the warm/cold dispatch and + # failure propagation in setup_and_start_hw_agent_service, so we stub + # _setup_hw_agent_service out entirely. + def test_warm_boot_dispatch(self): + with ( + patch.object(fboss_agent_utils, "_setup_hw_agent_service"), + patch.object( + fboss_agent_utils, "warm_boot_hw_agent", return_value=[0] + ) as mock_warm, + patch.object( + fboss_agent_utils, "cold_boot_hw_agent", return_value=[0] + ) as mock_cold, + ): + setup_and_start_hw_agent_service( + switch_indexes=[0], + fboss_agent_config_path="ignored.conf", + hw_agent_service_bin_path=sys.executable, + is_warm_boot=True, + ) + mock_warm.assert_called_once_with([0], service_name=ANY) + mock_cold.assert_not_called() + + def test_cold_boot_dispatch_and_failure_raises(self): + with ( + patch.object(fboss_agent_utils, "_setup_hw_agent_service"), + patch.object(fboss_agent_utils, "warm_boot_hw_agent", return_value=[0]), + patch.object( + fboss_agent_utils, "cold_boot_hw_agent", return_value=[1] + ) as mock_cold, + pytest.raises(Exception, match="cold-boot"), + ): + setup_and_start_hw_agent_service( + switch_indexes=[0], + fboss_agent_config_path="ignored.conf", + hw_agent_service_bin_path=sys.executable, + is_warm_boot=False, + ) + mock_cold.assert_called_once_with([0], service_name=ANY) + + +class TestWarmBootMissingFileBehavior: + def test_warmboot_proceeds_to_start_when_warmboot_file_missing(self): + def fake_run(cmd, **_): + rc = 1 if cmd.startswith("stat ") else 0 + return MagicMock(returncode=rc) + + with patch.object( + fboss_agent_utils.subprocess, "run", side_effect=fake_run + ) as mock_run: + return_codes = warm_boot_hw_agent(switch_indexes=[0]) + + commands = [c.args[0] for c in mock_run.call_args_list] + assert any( + c.startswith("systemctl start fboss_hw_agent_oss@0") for c in commands + ) + assert return_codes == [0] diff --git a/fboss/oss/scripts/run_scripts/tests/test_fsdb_service.py b/fboss/oss/scripts/run_scripts/tests/test_fsdb_service.py new file mode 100644 index 0000000000000..5b2475d246c37 --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_fsdb_service.py @@ -0,0 +1,90 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for fsdb_service_utils FSDB systemd service lifecycle.""" + +import os +import sys +from unittest.mock import patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +import fsdb_service_utils +from fsdb_service_utils import ( + _start_fsdb_service, + cleanup_fsdb_service, + setup_and_start_fsdb_service, +) + + +class TestCleanupFsdbService: + def test_stops_all_three_variants_and_pkills(self): + with patch.object(fsdb_service_utils.subprocess, "run") as mock_run: + cleanup_fsdb_service() + commands = "\n".join(c.args[0] for c in mock_run.call_args_list) + assert "systemctl stop fsdb.service" in commands + assert "systemctl stop fsdb_service_for_testing" in commands + assert "systemctl stop fsdb_service_oss" in commands + assert "systemctl disable fsdb_service_oss" in commands + assert "systemctl daemon-reload" in commands + assert "pkill -f fsdb_service_oss" in commands + + +class TestSetupPreconditions: + def _patches(self, monkeypatch, tmp_path): + monkeypatch.setattr( + fsdb_service_utils, + "_FSDB_SERVICE_UNIT_FILE_PATH", + str(tmp_path / "fsdb_oss.service"), + ) + monkeypatch.setattr( + fsdb_service_utils, + "_FSDB_SERVICE_RSYSLOG_CONF_PATH", + str(tmp_path / "rsyslog.conf"), + ) + + def test_raises_when_binary_missing(self, monkeypatch, tmp_path): + self._patches(monkeypatch, tmp_path) + monkeypatch.setattr( + fsdb_service_utils, + "_DEFAULT_OSS_FSDB_SERVICE_PATH", + str(tmp_path / "missing_fsdb_bin"), + ) + with pytest.raises(Exception, match="fsdb_service binary"): + setup_and_start_fsdb_service() + + def test_raises_when_config_missing(self, monkeypatch, tmp_path): + self._patches(monkeypatch, tmp_path) + monkeypatch.setattr( + fsdb_service_utils, "_DEFAULT_OSS_FSDB_SERVICE_PATH", sys.executable + ) + with pytest.raises(Exception, match="fsdb_service config path"): + setup_and_start_fsdb_service( + fsdb_service_config_path=str(tmp_path / "missing.conf") + ) + + +class TestStartColdVsWarmBoot: + def test_cold_boot_creates_marker(self): + with ( + patch.object(fsdb_service_utils.subprocess, "run") as mock_run, + patch.object( + fsdb_service_utils, "_setup_fsdb_service_coldboot" + ) as mock_marker, + ): + _start_fsdb_service(is_warm_boot=False) + mock_marker.assert_called_once() + commands = "\n".join(c.args[0] for c in mock_run.call_args_list) + assert "systemctl enable" in commands + assert "systemctl start fsdb_service_oss" in commands + + def test_warm_boot_skips_marker(self): + with ( + patch.object(fsdb_service_utils.subprocess, "run"), + patch.object( + fsdb_service_utils, "_setup_fsdb_service_coldboot" + ) as mock_marker, + ): + _start_fsdb_service(is_warm_boot=True) + mock_marker.assert_not_called() diff --git a/fboss/oss/scripts/run_scripts/tests/test_qsfp_service.py b/fboss/oss/scripts/run_scripts/tests/test_qsfp_service.py new file mode 100644 index 0000000000000..cf51c03e3efeb --- /dev/null +++ b/fboss/oss/scripts/run_scripts/tests/test_qsfp_service.py @@ -0,0 +1,133 @@ +# Copyright Meta Platforms, Inc. and affiliates. +# @noautodeps +"""Tests for qsfp_service_utils QSFP systemd service lifecycle.""" + +import os +import sys +from unittest.mock import patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +import qsfp_service_utils +from qsfp_service_utils import ( + _setup_qsfp_service, + _start_qsfp_service, + cleanup_qsfp_service, + setup_and_start_qsfp_service, +) + + +def _redirect_paths(monkeypatch, tmp_path): + monkeypatch.setattr( + qsfp_service_utils, + "_QSFP_SERVICE_UNIT_FILE_PATH", + str(tmp_path / "qsfp_oss.service"), + ) + monkeypatch.setattr( + qsfp_service_utils, + "_QSFP_SERVICE_RSYSLOG_CONF_PATH", + str(tmp_path / "rsyslog.conf"), + ) + + +class TestCleanupQsfpService: + def test_stops_all_three_variants_and_pkills(self): + with patch.object(qsfp_service_utils.subprocess, "run") as mock_run: + cleanup_qsfp_service() + commands = "\n".join(c.args[0] for c in mock_run.call_args_list) + assert "systemctl stop qsfp_service" in commands + assert "systemctl stop qsfp_service_for_testing" in commands + assert "systemctl stop qsfp_service_oss" in commands + assert "systemctl disable qsfp_service_oss" in commands + assert "pkill -f qsfp_service_oss" in commands + + +class TestSetupPreconditions: + def test_raises_when_binary_missing(self, monkeypatch, tmp_path): + _redirect_paths(monkeypatch, tmp_path) + monkeypatch.setattr( + qsfp_service_utils, + "_DEFAULT_OSS_QSFP_SERVICE_PATH", + str(tmp_path / "missing_qsfp_bin"), + ) + config = tmp_path / "qsfp.conf" + config.write_text("") + with pytest.raises(Exception, match="qsfp_service binary"): + setup_and_start_qsfp_service(qsfp_service_config_path=str(config)) + + def test_raises_when_config_none(self, monkeypatch, tmp_path): + _redirect_paths(monkeypatch, tmp_path) + monkeypatch.setattr( + qsfp_service_utils, "_DEFAULT_OSS_QSFP_SERVICE_PATH", sys.executable + ) + with pytest.raises(Exception, match=r"qsfp_service config path.*is None"): + setup_and_start_qsfp_service(qsfp_service_config_path=None) + + +class TestUnitFileExtraArgs: + def _setup(self, monkeypatch, tmp_path): + _redirect_paths(monkeypatch, tmp_path) + monkeypatch.setattr( + qsfp_service_utils, "_DEFAULT_OSS_QSFP_SERVICE_PATH", sys.executable + ) + + def test_fsdb_enabled_adds_ssl_preferred_flag(self, monkeypatch, tmp_path): + self._setup(monkeypatch, tmp_path) + config = tmp_path / "qsfp.conf" + config.write_text("") + with patch.object(qsfp_service_utils.subprocess, "run"): + _setup_qsfp_service( + qsfp_service_config_path=str(config), is_fsdb_disabled=False + ) + unit = (tmp_path / "qsfp_oss.service").read_text() + assert "--fsdb_client_ssl_preferred=false" in unit + assert "--publish_stats_to_fsdb=true" in unit + + def test_fsdb_disabled_omits_ssl_preferred_flag(self, monkeypatch, tmp_path): + self._setup(monkeypatch, tmp_path) + config = tmp_path / "qsfp.conf" + config.write_text("") + with patch.object(qsfp_service_utils.subprocess, "run"): + _setup_qsfp_service( + qsfp_service_config_path=str(config), is_fsdb_disabled=True + ) + unit = (tmp_path / "qsfp_oss.service").read_text() + assert "--fsdb_client_ssl_preferred=false" not in unit + + def test_platform_mapping_override_appended_when_set(self, monkeypatch, tmp_path): + self._setup(monkeypatch, tmp_path) + config = tmp_path / "qsfp.conf" + config.write_text("") + pm = tmp_path / "pm.json" + pm.write_text("{}") + with patch.object(qsfp_service_utils.subprocess, "run"): + _setup_qsfp_service( + qsfp_service_config_path=str(config), + platform_mapping_override_path=str(pm), + ) + unit = (tmp_path / "qsfp_oss.service").read_text() + assert f"--platform_mapping_override_path {pm}" in unit + + +class TestStartColdVsWarmBoot: + def test_cold_boot_creates_marker(self): + with ( + patch.object(qsfp_service_utils.subprocess, "run"), + patch.object( + qsfp_service_utils, "_setup_qsfp_service_coldboot" + ) as mock_marker, + ): + _start_qsfp_service(is_warm_boot=False) + mock_marker.assert_called_once() + + def test_warm_boot_skips_marker(self): + with ( + patch.object(qsfp_service_utils.subprocess, "run"), + patch.object( + qsfp_service_utils, "_setup_qsfp_service_coldboot" + ) as mock_marker, + ): + _start_qsfp_service(is_warm_boot=True) + mock_marker.assert_not_called() From 5286fa639da0d19f1e7135094a6d481386bc8bd5 Mon Sep 17 00:00:00 2001 From: Arjun Chaturvedi Date: Wed, 27 May 2026 11:59:33 -0700 Subject: [PATCH 3/3] Add GitHub workflow for run_scripts pytest unit tests Summary: Adds a lightweight GHA workflow that runs the pytest unit tests under `fboss/oss/scripts/run_scripts/tests/` on PRs that touch `run_scripts/**`. The existing `docker-unittest.py` only discovers compiled gtest binaries, so these ~120 pure-Python tests currently run on no PR. Runs in seconds with no getdeps/docker. Differential Revision: D106542800 --- .github/workflows/run-scripts-unittests.yml | 42 +++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .github/workflows/run-scripts-unittests.yml diff --git a/.github/workflows/run-scripts-unittests.yml b/.github/workflows/run-scripts-unittests.yml new file mode 100644 index 0000000000000..a21406b83bfb0 --- /dev/null +++ b/.github/workflows/run-scripts-unittests.yml @@ -0,0 +1,42 @@ +name: run_scripts unit tests + +on: + pull_request: + paths: + - 'fboss/oss/scripts/run_scripts/**' + - '.github/workflows/run-scripts-unittests.yml' + push: + branches: [ main ] + paths: + - 'fboss/oss/scripts/run_scripts/**' + - '.github/workflows/run-scripts-unittests.yml' + workflow_dispatch: + +# Cancel superseded runs on the same ref to avoid wasting CI minutes on +# rapid PR pushes — hermetic tests, safe to cancel mid-run. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +# Read-only test workflow; no need for the default broad write scope. +permissions: + contents: read + +jobs: + Run-Scripts-Unit-Tests: + runs-on: ubuntu-latest + # Pure-Python pytest suite finishes in seconds; 10 min is a generous + # upper bound that catches genuine hangs without sitting on the GHA + # default 6h timeout. + timeout-minutes: 10 + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-python@v6 + with: + python-version: '3.12' + - name: Install pytest and parameterized + # Pin major versions: hermetic tests should be reproducible across + # CI runs even when upstream releases breaking-API majors. + run: pip install 'pytest>=8,<9' 'parameterized>=0.9,<1' + - name: Run pytest + run: pytest fboss/oss/scripts/run_scripts/tests/ -v