refactor(tests): core logic unit tests to follow test conventions

test/unit/test_card_creator.py

@@ -1,60 +1,85 @@
-"""Regression tests for async card process timeout handling."""
+"""Regression tests for async card process timeout handling.
+
+These tests ensure that asynchronous card creation processes are reliably managed,
+properly timed out, and remain resilient to system clock variations (like NTP syncs).
+"""
 
 import subprocess
 
 import pytest
 
 from metaflow.plugins.cards.card_creator import CardCreator, CardProcessManager
 
+# ---------------------------------------------------------------------------
+# Constants
+# ---------------------------------------------------------------------------
+
 CARD_UUID = "card-uuid"
 ASYNC_TIMEOUT = 60
 
+# Module paths for cleaner mocking
+MODULE_TIME = "metaflow.plugins.cards.card_creator.time"
+MODULE_SUBPROCESS = "metaflow.plugins.cards.card_creator.subprocess"
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
 
 @pytest.fixture(autouse=True)
 def clean_card_process_registry():
+    """Ensure a pristine process registry before and after every test."""
     CardProcessManager.async_card_processes.clear()
     yield
     CardProcessManager.async_card_processes.clear()
 
 
 @pytest.fixture
 def running_process(mocker):
+    """Provide a mock subprocess that appears to be actively running."""
     process = mocker.Mock()
     process.poll.return_value = None
     return process
 
 
 @pytest.fixture
 def card_creator():
+    """Provide a minimally configured CardCreator instance."""
     return CardCreator(
         top_level_options=[], should_save_metadata_lambda=lambda _: (False, {})
     )
 
 
+# ---------------------------------------------------------------------------
+# Tests
+# ---------------------------------------------------------------------------
+
+
 def test_register_card_process_uses_monotonic_timestamp(mocker, running_process):
-    mocker.patch("metaflow.plugins.cards.card_creator.time.time", return_value=1.0)
-    mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=42.0
-    )
+    """Verify that process registration relies on monotonic time, not wall-clock time."""
+    mocker.patch(f"{MODULE_TIME}.time", return_value=1.0)
+    mocker.patch(f"{MODULE_TIME}.monotonic", return_value=42.0)
 
     CardProcessManager._register_card_process(CARD_UUID, running_process)
-
     _, started = CardProcessManager._get_card_process(CARD_UUID)
+
     assert started == 42.0
 
 
 def test_wait_for_async_process_ignores_backward_wall_clock_jump(
     mocker, card_creator, running_process
 ):
-    mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=100.0
-    )
+    """Ensure a backward jump in the system clock (e.g., NTP sync) does not bypass the timeout logic."""
+    mocker.patch(f"{MODULE_TIME}.monotonic", return_value=100.0)
     CardProcessManager._register_card_process(CARD_UUID, running_process)
+
     running_process.poll.side_effect = [None, 0]
-    mocker.patch("metaflow.plugins.cards.card_creator.time.time", return_value=-86400.0)
     mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=700.0
-    )
+        f"{MODULE_TIME}.time", return_value=-86400.0
+    )  # Extreme backward wall-clock jump
+    mocker.patch(
+        f"{MODULE_TIME}.monotonic", return_value=700.0
+    )  # Monotonic time correctly progresses
 
     card_creator._wait_for_async_processes_to_finish(
         CARD_UUID, async_timeout=ASYNC_TIMEOUT
@@ -67,14 +92,14 @@ def test_wait_for_async_process_ignores_backward_wall_clock_jump(
 def test_wait_for_async_process_leaves_process_within_timeout(
     mocker, card_creator, running_process
 ):
-    mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=100.0
-    )
+    """Ensure processes are allowed to continue running if the timeout threshold has not been breached."""
+    mocker.patch(f"{MODULE_TIME}.monotonic", return_value=100.0)
     CardProcessManager._register_card_process(CARD_UUID, running_process)
+
     running_process.poll.side_effect = [None, 0]
     mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=105.0
-    )
+        f"{MODULE_TIME}.monotonic", return_value=105.0
+    )  # Only 5 seconds elapsed
 
     card_creator._wait_for_async_processes_to_finish(
         CARD_UUID, async_timeout=ASYNC_TIMEOUT
@@ -85,32 +110,38 @@ def test_wait_for_async_process_leaves_process_within_timeout(
 
 
 def test_async_run_replaces_timed_out_process(mocker, card_creator, running_process):
+    """Verify that launching a new async command correctly kills and replaces an existing timed-out process."""
     replacement_process = mocker.Mock()
     popen = mocker.patch(
-        "metaflow.plugins.cards.card_creator.subprocess.Popen",
+        f"{MODULE_SUBPROCESS}.Popen",
         return_value=replacement_process,
     )
-    mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=100.0
-    )
+
+    # Register the initial process
+    mocker.patch(f"{MODULE_TIME}.monotonic", return_value=100.0)
     CardProcessManager._register_card_process(CARD_UUID, running_process)
-    mocker.patch("metaflow.plugins.cards.card_creator.time.time", return_value=-86400.0)
-    mocker.patch(
-        "metaflow.plugins.cards.card_creator.time.monotonic", return_value=700.0
-    )
 
+    # Simulate time passing beyond the timeout threshold
+    mocker.patch(f"{MODULE_TIME}.time", return_value=-86400.0)
+    mocker.patch(f"{MODULE_TIME}.monotonic", return_value=700.0)
+
+    # Attempt to run a new command with the same UUID
     output, failed = card_creator._run_command(
         ["python", "card.py"], CARD_UUID, {"KEY": "value"}, wait=False
     )
 
+    # Assertions
     assert output == b""
     assert failed is False
+
     running_process.kill.assert_called_once_with()
     popen.assert_called_once_with(
         ["python", "card.py"],
         env={"KEY": "value"},
         stderr=subprocess.DEVNULL,
         stdout=subprocess.DEVNULL,
     )
+
+    # Verify the registry holds the new process
     process, _ = CardProcessManager._get_card_process(CARD_UUID)
     assert process is replacement_process

test/unit/test_config_value.py

@@ -4,92 +4,123 @@
 
 from metaflow.user_configs.config_parameters import ConfigValue
 
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
 
-def test_isinstance():
-    orig_dict = {"a": 1, "b": 2}
-    c_value = ConfigValue(orig_dict)
-    assert isinstance(c_value, dict)
-
-
-def test_todict():
-    orig_dict = {"a": 1, "b": 2}
-    c_value = ConfigValue(orig_dict)
-    assert c_value.to_dict() == orig_dict
 
-    orig_dict = {"a": 1, "b": [1, 2, 3], "c": {"d": 4}, "e": {"f": [{"g": 5}]}}
-    c_value = ConfigValue(orig_dict)
-    assert c_value.to_dict() == orig_dict
+@pytest.fixture
+def simple_dict():
+    """Provides a basic, flat dictionary."""
+    return {"a": 1, "b": 2}
 
 
-def test_container_has_config_value():
-    orig_dict = {
+@pytest.fixture
+def nested_dict():
+    """Provides a complex, deeply nested dictionary containing lists and tuples."""
+    return {
         "a": 1,
         "b": [1, 2, 3],
         "c": {"d": 4},
         "e": {"f": [{"g": 5}]},
         "h": ({"i": 6},),
     }
-    c_value = ConfigValue(orig_dict)
+
+
+# ---------------------------------------------------------------------------
+# Tests
+# ---------------------------------------------------------------------------
+
+
+def test_config_value_is_dict_instance(simple_dict):
+    """Ensure ConfigValue correctly registers as a subclass/instance of dict."""
+    c_value = ConfigValue(simple_dict)
+    assert isinstance(c_value, dict)
+
+
+def test_todict_returns_original_data(simple_dict, nested_dict):
+    """Verify that to_dict() reconstructs the original standard dictionary exactly."""
+    assert ConfigValue(simple_dict).to_dict() == simple_dict
+    assert ConfigValue(nested_dict).to_dict() == nested_dict
+
+
+def test_container_preserves_config_value_type_internally(nested_dict):
+    """Verify that nested dictionaries within lists/tuples are correctly wrapped as ConfigValues."""
+    c_value = ConfigValue(nested_dict)
+
+    # Dot-notation access works deeply
     assert c_value.e.f[0].g == 5
+
+    # Nested dicts become ConfigValues
     assert isinstance(c_value.c, ConfigValue)
     assert isinstance(c_value.e, ConfigValue)
+
+    # Lists remain lists, but their dict elements become ConfigValues
     assert isinstance(c_value.e.f, list)
     assert isinstance(c_value.e.f[0], ConfigValue)
+
+    # Tuples remain tuples, but their dict elements become ConfigValues
     assert isinstance(c_value.h, tuple)
     assert isinstance(c_value.h[0], ConfigValue)
 
 
-def test_non_modifiable():
-    orig_dict = {"a": 1, "b": 2, "c": 3}
-    c_value = ConfigValue(orig_dict)
-    with pytest.raises(TypeError):
-        c_value["d"] = 4
-    with pytest.raises(TypeError):
-        c_value.popitem()
-    with pytest.raises(TypeError):
-        c_value.pop("a", 5)
-    with pytest.raises(TypeError):
-        c_value.clear()
-    with pytest.raises(TypeError):
-        c_value.update({"e": 6})
-    with pytest.raises(TypeError):
-        c_value.setdefault("f", 7)
+@pytest.mark.parametrize(
+    "operation",
+    [
+        lambda c: c.__setitem__("d", 4),
+        lambda c: c.popitem(),
+        lambda c: c.pop("a", 5),
+        lambda c: c.clear(),
+        lambda c: c.update({"e": 6}),
+        lambda c: c.setdefault("f", 7),
+        lambda c: c.__delitem__("b"),
+    ],
+    ids=[
+        "setitem",
+        "popitem",
+        "pop",
+        "clear",
+        "update",
+        "setdefault",
+        "delitem",
+    ],
+)
+def test_config_value_is_non_modifiable(simple_dict, operation):
+    """Ensure that all standard dict mutation methods raise a TypeError."""
+    # Expand simple_dict slightly so pop/del have valid targets if needed
+    extended_dict = {**simple_dict, "c": 3}
+    c_value = ConfigValue(extended_dict)
+
     with pytest.raises(TypeError):
-        del c_value["b"]
+        operation(c_value)
 
-    assert c_value.to_dict() == orig_dict
+    # Ensure the underlying structure was not secretly mutated
+    assert c_value.to_dict() == extended_dict
 
 
-def test_json_dumpable():
-    orig_dict = {
-        "a": 1,
-        "b": [1, 2, 3],
-        "c": {"d": 4},
-        "e": {"f": [{"g": 5}]},
-        "h": ({"i": 6},),
-    }
-    c_value = ConfigValue(orig_dict)
-    assert json.loads(json.dumps(c_value)) == json.loads(json.dumps(orig_dict))
+def test_json_dumpable(nested_dict):
+    """Ensure the custom ConfigValue dictionary behaves natively with the json module."""
+    c_value = ConfigValue(nested_dict)
 
+    # Compare the serialized outputs
+    assert json.loads(json.dumps(c_value)) == json.loads(json.dumps(nested_dict))
+
+
+def test_dict_like_iteration_and_access(nested_dict):
+    """Verify standard dictionary iteration, membership, and length behaviors work."""
+    c_value = ConfigValue(nested_dict)
 
-def test_dict_like_behavior():
-    orig_dict = {
-        "a": 1,
-        "b": [1, 2, 3],
-        "c": {"d": 4},
-        "e": {"f": [{"g": 5}]},
-        "h": ({"i": 6},),
-    }
-    c_value = ConfigValue(orig_dict)
     assert "a" in c_value
     assert "d" not in c_value
     assert len(c_value) == 5
-    assert c_value.keys() == orig_dict.keys()
+
+    assert c_value.keys() == nested_dict.keys()
+
     for k, v in c_value.items():
-        assert v == orig_dict[k]
+        assert v == nested_dict[k]
 
     for k in c_value.keys():
-        assert k in orig_dict
+        assert k in nested_dict
 
     for v in c_value.values():
-        assert v in orig_dict.values()
+        assert v in nested_dict.values()