Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 1 addition & 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
97 changes: 97 additions & 0 deletions tests/comfy_cli/test_tracking.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
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, "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.


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)
assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) is not None
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

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