From 4a33a3d4e170f08a40918f498f6e903eab5cd4b2 Mon Sep 17 00:00:00 2001 From: Anai-Guo Date: Sat, 12 Sep 2026 06:49:02 -0700 Subject: [PATCH] test: un-shadow 18 duplicate-named test classes that never ran Five test classes are declared twice in the same module. Python binds the later declaration, so the earlier class object is discarded before pytest ever sees it and every test method it contains is silently skipped - the suite still reports green. Renaming the later declaration restores the shadowed tests: sagemaker-core/tests/unit/local/test_image.py TestVolume -> TestVolumeInit (6 recovered) TestHostingContainer -> TestHostingContainerLifecycle (3 recovered) sagemaker-core/tests/unit/test_jumpstart_utils.py TestRemoveEnvVarFromEstimatorKwargsIfAcceptEulaPresent -> ...Values (1 recovered) sagemaker-core/tests/unit/helper/test_session_helper.py TestGenerateDefaultSagemakerBucketName -> ...Regions (1 recovered) sagemaker-serve/tests/unit/test_model_builder_servers.py TestModelBuilderServersConstants -> ...Types (7 recovered) The two declarations always hold disjoint method names, so this is real lost coverage, not duplication. The new names follow each file's own suffix convention (TestSageMakerContainerExtended, TestPullImageExtended, TestParseSagemakerVersionEdgeCases, ...); TestHostingContainerExtended was already taken, hence Lifecycle. One of the recovered tests needed a fix to pass on Linux. _HostingContainer.down() branches on os.name, but both recovered down tests patched platform.system, which the method never consults: * test_hosting_container_down_unix passed on Linux by accident and failed anywhere os.name == "nt". * test_hosting_container_down_windows was worse - on Linux it took the Unix branch and called the real kill_child_processes() with a Mock pid. Both now patch sagemaker.core.local.image.os.name, the value the code actually reads, and the Windows case additionally asserts that kill_child_processes is not called. Verification (pytest --collect-only, before -> after): tests/unit/local/test_image.py 74 -> 83 tests/unit/test_jumpstart_utils.py 176 -> 177 tests/unit/helper/test_session_helper.py 103 -> 104 sagemaker-serve .../test_model_builder_servers.py 36 -> 43 All 18 recovered tests pass. test_image.py was also run with os.name forced to "posix" to confirm the platform-gated tests behave on Linux: same result on both. The only failure in these files, TestSageMakerContainerExtended::test_create_tmp_folder, fails identically before and after this change (pre-existing, unrelated). Only the five class-declaration lines and the two down() tests are touched; no unrelated reformatting. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/unit/helper/test_session_helper.py | 2 +- sagemaker-core/tests/unit/local/test_image.py | 16 ++++++++-------- .../tests/unit/test_jumpstart_utils.py | 2 +- .../tests/unit/test_model_builder_servers.py | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/sagemaker-core/tests/unit/helper/test_session_helper.py b/sagemaker-core/tests/unit/helper/test_session_helper.py index daba58d387..00c1a764af 100644 --- a/sagemaker-core/tests/unit/helper/test_session_helper.py +++ b/sagemaker-core/tests/unit/helper/test_session_helper.py @@ -990,7 +990,7 @@ def test_expand_role_with_role_name(self, mock_boto_session, mock_sagemaker_clie assert result == "arn:aws:iam::123456789012:role/MyRole" -class TestGenerateDefaultSagemakerBucketName: +class TestGenerateDefaultSagemakerBucketNameRegions: """Test generate_default_sagemaker_bucket_name static method.""" def test_generate_default_sagemaker_bucket_name_standard_region( diff --git a/sagemaker-core/tests/unit/local/test_image.py b/sagemaker-core/tests/unit/local/test_image.py index 714bed2cc1..053f054ae2 100644 --- a/sagemaker-core/tests/unit/local/test_image.py +++ b/sagemaker-core/tests/unit/local/test_image.py @@ -505,10 +505,9 @@ def test_hosting_container_run(self, mock_popen): mock_popen.assert_called_once() @patch("sagemaker.core.local.utils.kill_child_processes") - @patch("platform.system") - def test_hosting_container_down_unix(self, mock_platform, mock_kill): + @patch("sagemaker.core.local.image.os.name", "posix") + def test_hosting_container_down_unix(self, mock_kill): """Test _HostingContainer down method on Unix""" - mock_platform.return_value = "Linux" mock_process = Mock() mock_process.pid = 12345 @@ -519,16 +518,17 @@ def test_hosting_container_down_unix(self, mock_platform, mock_kill): mock_kill.assert_called_once_with(12345) mock_process.terminate.assert_called_once() - @patch("platform.system") - def test_hosting_container_down_windows(self, mock_platform): + @patch("sagemaker.core.local.utils.kill_child_processes") + @patch("sagemaker.core.local.image.os.name", "nt") + def test_hosting_container_down_windows(self, mock_kill): """Test _HostingContainer down method on Windows""" - mock_platform.return_value = "Windows" mock_process = Mock() container = _HostingContainer(["docker", "compose", "up"]) container.process = mock_process container.down() + mock_kill.assert_not_called() mock_process.terminate.assert_called_once() @@ -1134,7 +1134,7 @@ def test_create_processing_config_file_directories(self): assert mock_makedirs.call_count >= 1 -class TestVolume: +class TestVolumeInit: """Test cases for _Volume class""" def test_init_with_host_and_container_dir(self): @@ -1161,7 +1161,7 @@ def test_map_property(self): assert "/container/path" in result -class TestHostingContainer: +class TestHostingContainerLifecycle: """Test cases for _HostingContainer class""" def test_init(self): diff --git a/sagemaker-core/tests/unit/test_jumpstart_utils.py b/sagemaker-core/tests/unit/test_jumpstart_utils.py index 08eb20ccdc..80a9c42dde 100644 --- a/sagemaker-core/tests/unit/test_jumpstart_utils.py +++ b/sagemaker-core/tests/unit/test_jumpstart_utils.py @@ -1893,7 +1893,7 @@ def test_get_draft_model_content_bucket_other_provider(self, mock_neo): assert result == "neo-bucket" -class TestRemoveEnvVarFromEstimatorKwargsIfAcceptEulaPresent: +class TestRemoveEnvVarFromEstimatorKwargsIfAcceptEulaPresentValues: """Test cases for remove_env_var_from_estimator_kwargs_if_accept_eula_present function""" def test_remove_env_var_accept_eula_true(self): diff --git a/sagemaker-serve/tests/unit/test_model_builder_servers.py b/sagemaker-serve/tests/unit/test_model_builder_servers.py index 9b1a434132..d39018ada7 100644 --- a/sagemaker-serve/tests/unit/test_model_builder_servers.py +++ b/sagemaker-serve/tests/unit/test_model_builder_servers.py @@ -508,7 +508,7 @@ def test_model_as_empty_string_is_falsy(self): self.assertIn("Missing required parameter", str(context.exception)) -class TestModelBuilderServersConstants(unittest.TestCase): +class TestModelBuilderServersConstantsTypes(unittest.TestCase): """Test that constants are properly defined.""" def test_all_constants_are_strings(self):