-
Notifications
You must be signed in to change notification settings - Fork 119
fix: re-enable telemetry and cover enable_tracking config #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
49177e6
e6d700b
c44e894
1111e41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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()), | ||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||
|
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") | ||||||||||||||||||||||||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding a comment to explain Line 80 resets the mock counter after 📝 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
Suggested change
🧰 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 |
||||||||||||||||||||||||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🧪 Proposed additional testdef 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 |
||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.