From 54f9282b090f478851789af0604225e39bcf96b3 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 28 Jan 2025 12:04:05 -0500 Subject: [PATCH] Break up `is_supported` check into parsing and evaluating steps (#149) Separates `evaluate_test_config` into `parse_lang_spec_from_flags` and `evaluate_is_supported` so that custom override logic can be inserted between them. This change exposed a bug with `cls.lang_spec.server_lang` getting out of sync with `cls.server_image` when the image is overridden in suite's `setUpClass()`. --- framework/xds_k8s_testcase.py | 49 +++++++++++++++++++------------ framework/xds_url_map_testcase.py | 4 ++- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/framework/xds_k8s_testcase.py b/framework/xds_k8s_testcase.py index ce9996c6..1c602e7d 100644 --- a/framework/xds_k8s_testcase.py +++ b/framework/xds_k8s_testcase.py @@ -28,6 +28,7 @@ from absl.testing import absltest from google.protobuf import json_format import grpc +from typing_extensions import TypeAlias from framework import xds_flags from framework import xds_k8s_flags @@ -68,6 +69,8 @@ ClientDeploymentArgs = k8s_xds_client_runner.ClientDeploymentArgs KubernetesServerRunner = k8s_xds_server_runner.KubernetesServerRunner KubernetesClientRunner = k8s_xds_client_runner.KubernetesClientRunner +TestConfig: TypeAlias = skips.TestConfig +Lang: TypeAlias = skips.Lang _LoadBalancerStatsResponse = grpc_testing.LoadBalancerStatsResponse _LoadBalancerAccumulatedStatsResponse = ( grpc_testing.LoadBalancerAccumulatedStatsResponse @@ -89,36 +92,35 @@ _TD_CONFIG_MAX_WAIT_SEC: Final[int] = int(TD_CONFIG_MAX_WAIT.total_seconds()) -def evaluate_test_config( - check: Callable[[skips.TestConfig], bool] -) -> skips.TestConfig: - """Evaluates the test config check against Abseil flags. +def parse_lang_spec_from_flags() -> TestConfig: + test_config = TestConfig( + client_lang=skips.get_lang(xds_k8s_flags.CLIENT_IMAGE.value), + server_lang=skips.get_lang(xds_k8s_flags.SERVER_IMAGE.value), + version=xds_flags.TESTING_VERSION.value, + ) + logger.info("Detected language and version: %s", test_config) + return test_config - TODO(sergiitk): split into parse_lang_spec and check_is_supported. - """ + +def evaluate_is_supported( + test_config: TestConfig, is_supported_fn: Callable[[TestConfig], bool] +): + """Evaluates the suite-specific is_supported against the test_config.""" # NOTE(lidiz) a manual skip mechanism is needed because absl/flags # cannot be used in the built-in test-skipping decorators. See the # official FAQs: # https://abseil.io/docs/python/guides/flags#faqs - test_config = skips.TestConfig( - client_lang=skips.get_lang(xds_k8s_flags.CLIENT_IMAGE.value), - server_lang=skips.get_lang(xds_k8s_flags.SERVER_IMAGE.value), - version=xds_flags.TESTING_VERSION.value, - ) - if not check(test_config): + if not is_supported_fn(test_config): logger.info("Skipping %s", test_config) raise absltest.SkipTest(f"Unsupported test config: {test_config}") - logger.info("Detected language and version: %s", test_config) - return test_config - class TdPropagationRetryableError(Exception): """Indicates that TD config hasn't propagated yet, and it's safe to retry""" class XdsKubernetesBaseTestCase(base_testcase.BaseTestCase): - lang_spec: skips.TestConfig + lang_spec: TestConfig client_namespace: str client_runner: KubernetesClientRunner ensure_firewall: bool = False @@ -151,7 +153,7 @@ class XdsKubernetesBaseTestCase(base_testcase.BaseTestCase): enable_dualstack: bool = False @staticmethod - def is_supported(config: skips.TestConfig) -> bool: + def is_supported(config: TestConfig) -> bool: """Overridden by the test class to decide if the config is supported. Returns: @@ -168,9 +170,20 @@ def setUpClass(cls): logger.info("----- Testing %s -----", cls.__name__) logger.info("Logs timezone: %s", time.localtime().tm_zone) + lang_spec = parse_lang_spec_from_flags() + + # Currently it's possible to lang_spec.server_lang to be out of sync + # with the server_image when a test_suite overrides the server_image + # at the end of its setUpClass(). + # A common example is overriding the server image to canonical. + # This can be fixed by moving server image overrides to its own + # class method and re-parsing the lang spec. + # TODO(sergiitk): provide custom server_image_override(TestConfig) + # Raises unittest.SkipTest if given client/server/version does not # support current test case. - cls.lang_spec = evaluate_test_config(cls.is_supported) + evaluate_is_supported(lang_spec, cls.is_supported) + cls.lang_spec = lang_spec # Must be called before KubernetesApiManager or GcpApiManager init. xds_flags.set_socket_default_timeout_from_flag() diff --git a/framework/xds_url_map_testcase.py b/framework/xds_url_map_testcase.py index 89818f0c..8a0551c9 100644 --- a/framework/xds_url_map_testcase.py +++ b/framework/xds_url_map_testcase.py @@ -220,9 +220,11 @@ def setUpClass(cls): logging.info("----- Testing %s -----", cls.__name__) logging.info("Logs timezone: %s", time.localtime().tm_zone) + lang_spec = xds_k8s_testcase.parse_lang_spec_from_flags() + # Raises unittest.SkipTest if given client/server/version does not # support current test case. - xds_k8s_testcase.evaluate_test_config(cls.is_supported) + xds_k8s_testcase.evaluate_is_supported(lang_spec, cls.is_supported) # Configure cleanup to run after all tests regardless of # whether setUpClass failed.