Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions comfy_cli/tracking.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ def track_event(event_name: str, properties: any = None):
if properties is None:
properties = {}
logging.debug(f"tracking event called with event_name: {event_name} and properties: {properties}")
# enable_tracking = config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING)
enable_tracking = False
enable_tracking = config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING)
if not enable_tracking:
return
Comment thread
bigcat88 marked this conversation as resolved.

Expand Down Expand Up @@ -98,6 +97,7 @@ def init_tracking(enable_tracking: bool):
"""
Initialize the tracking system by setting the user identifier and tracking enabled status.
"""
global user_id
logging.debug(f"Initializing tracking with enable_tracking: {enable_tracking}")
config_manager.set(constants.CONFIG_KEY_ENABLE_TRACKING, str(enable_tracking))
if not enable_tracking:
Expand All @@ -109,6 +109,7 @@ def init_tracking(enable_tracking: bool):
curr_user_id = str(uuid.uuid4())
config_manager.set(constants.CONFIG_KEY_USER_ID, curr_user_id)
logging.debug(f'Setting user identifier for tracking user_id: {curr_user_id}."')
user_id = curr_user_id

# Note: only called once when the user interacts with the CLI for the
# first time iff the permission is granted.
Expand Down
105 changes: 105 additions & 0 deletions tests/comfy_cli/test_tracking.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
from unittest.mock import MagicMock, patch

import pytest

from comfy_cli import constants
from comfy_cli.config_manager import ConfigManager

# Unwrap the singleton to get fresh ConfigManager instances per test.
_ConfigManagerCls = ConfigManager.__closure__[0].cell_contents


@pytest.fixture
def tracking_module(tmp_path):
"""Yield comfy_cli.tracking with a fresh tmp-path ConfigManager and a mocked Mixpanel client."""
config_dir = tmp_path / "comfy-cli"
config_dir.mkdir()
with patch.object(_ConfigManagerCls, "get_config_path", return_value=str(config_dir)):
cfg = _ConfigManagerCls()

import comfy_cli.tracking as tracking_mod

with (
patch.object(tracking_mod, "config_manager", cfg),
patch.object(tracking_mod, "user_id", None),
patch.object(tracking_mod, "cli_version", "test-cli-version"),
patch.object(tracking_mod, "tracing_id", "test-tracing-id"),
patch.object(tracking_mod, "mp", MagicMock()),
):
Comment thread
bigcat88 marked this conversation as resolved.
yield tracking_mod


class TestTrackEvent:
def test_short_circuits_when_disabled(self, tracking_module):
tracking_module.config_manager.set(constants.CONFIG_KEY_ENABLE_TRACKING, "False")
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_not_called()

def test_short_circuits_when_not_configured(self, tracking_module):
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_not_called()

def test_fires_when_enabled(self, tracking_module):
tracking_module.config_manager.set(constants.CONFIG_KEY_ENABLE_TRACKING, "True")
tracking_module.track_event("some_event", {"k": "v"})
tracking_module.mp.track.assert_called_once()
_, kwargs = tracking_module.mp.track.call_args
assert kwargs["event_name"] == "some_event"
assert kwargs["properties"]["k"] == "v"
assert "cli_version" in kwargs["properties"]
assert "tracing_id" in kwargs["properties"]

def test_properties_default_to_empty_dict(self, tracking_module):
tracking_module.config_manager.set(constants.CONFIG_KEY_ENABLE_TRACKING, "True")
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_called_once()
_, kwargs = tracking_module.mp.track.call_args
assert set(kwargs["properties"].keys()) == {"cli_version", "tracing_id"}

def test_swallows_mixpanel_errors(self, tracking_module):
tracking_module.config_manager.set(constants.CONFIG_KEY_ENABLE_TRACKING, "True")
tracking_module.mp.track.side_effect = RuntimeError("boom")
tracking_module.track_event("some_event")
Comment thread
bigcat88 marked this conversation as resolved.
tracking_module.mp.track.assert_called_once()


class TestInitTrackingRoundTrip:
"""End-to-end: init_tracking() writes the string "False"/"True", and track_event honors it.

Regression for a prior bug where track_event used config_manager.get(), which returned
the raw string "False" (a truthy value), so disabling via this code path had no effect.
"""

def test_disable_is_respected_by_track_event(self, tracking_module):
tracking_module.init_tracking(False)
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_not_called()

def test_enable_is_respected_by_track_event(self, tracking_module):
tracking_module.init_tracking(True)
tracking_module.mp.track.reset_mock()
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_called_once()

Comment on lines +78 to +83
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding a comment to explain reset_mock() — it's a mock-ness of mercy!

Line 80 resets the mock counter after init_tracking(True), presumably because init fires an "install" event that shouldn't count toward the subsequent track_event assertion. A brief comment would help future maintainers understand why this reset is necessary.

📝 Proposed clarity improvement
     def test_enable_is_respected_by_track_event(self, tracking_module):
         tracking_module.init_tracking(True)
+        # init_tracking fires an "install" event; reset to test subsequent track_event calls
         tracking_module.mp.track.reset_mock()
         tracking_module.track_event("some_event")
         tracking_module.mp.track.assert_called_once()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_enable_is_respected_by_track_event(self, tracking_module):
tracking_module.init_tracking(True)
tracking_module.mp.track.reset_mock()
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_called_once()
def test_enable_is_respected_by_track_event(self, tracking_module):
tracking_module.init_tracking(True)
# init_tracking fires an "install" event; reset to test subsequent track_event calls
tracking_module.mp.track.reset_mock()
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_called_once()
🧰 Tools
🪛 Pylint (4.0.5)

[convention] 78-78: Missing function or method docstring

(C0116)


[warning] 78-78: Redefining name 'tracking_module' from outer scope (line 13)

(W0621)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/comfy_cli/test_tracking.py` around lines 78 - 83, Add a short inline
comment in the test_enable_is_respected_by_track_event test explaining why
tracking_module.mp.track.reset_mock() is called: note that init_tracking(True)
triggers an initial "install" (or similar) tracking call which we intentionally
clear so the subsequent tracking_module.track_event("some_event") assertion only
validates the event under test; reference the reset_mock call and the
init_tracking/track_event/mp.track symbols in the comment for clarity.

def test_disable_persists_as_parseable_bool(self, tracking_module):
tracking_module.init_tracking(False)
assert tracking_module.config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) is False

def test_enable_generates_user_id(self, tracking_module):
assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) is None
tracking_module.init_tracking(True)
generated_user_id = tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID)
assert generated_user_id is not None
assert tracking_module.user_id == generated_user_id
_, kwargs = tracking_module.mp.track.call_args
assert kwargs["distinct_id"] == generated_user_id

Comment on lines +88 to +96
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding a test for re-enabling with existing user_id.

The current test validates first-time user_id generation, but doesn't cover the case where a user disables then re-enables tracking. Consider adding a test that verifies init_tracking(True) preserves an existing user_id rather than generating a new one, ensuring tracking consistency across enable/disable cycles.

🧪 Proposed additional test
def test_enable_preserves_existing_user_id(self, tracking_module):
    # First enable generates ID
    tracking_module.init_tracking(True)
    first_user_id = tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID)
    
    # Disable then re-enable
    tracking_module.init_tracking(False)
    tracking_module.init_tracking(True)
    
    # Should preserve the same user_id
    assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) == first_user_id
    assert tracking_module.user_id == first_user_id
🧰 Tools
🪛 Pylint (4.0.5)

[convention] 88-88: Missing function or method docstring

(C0116)


[warning] 88-88: Redefining name 'tracking_module' from outer scope (line 13)

(W0621)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/comfy_cli/test_tracking.py` around lines 88 - 96, Add a new test (e.g.,
test_enable_preserves_existing_user_id) that exercises
tracking_module.init_tracking to ensure an existing user_id is preserved when
re-enabling: call tracking_module.init_tracking(True) to generate and read
first_user_id from
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID), call
init_tracking(False) then init_tracking(True) again, and assert
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) and
tracking_module.user_id still equal first_user_id (i.e., no new ID is
generated).

def test_disable_does_not_generate_user_id(self, tracking_module):
tracking_module.init_tracking(False)
assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) is None

def test_install_event_fires_once_across_calls(self, tracking_module):
tracking_module.init_tracking(True)
assert tracking_module.mp.track.call_count == 1
tracking_module.init_tracking(True)
assert tracking_module.mp.track.call_count == 1
Loading