chore: add utils CRAP score check#577
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. It is recommended to resolve "Warn" alerts too. Learn more about Socket for GitHub.
|
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
posthog/utils.py:415-424
**Hidden deletion side-effect in a predicate method**
`_key_has_version` reads as a pure boolean predicate, but it silently deletes the Redis key when JSON parsing fails. The caller `_delete_keys_with_version` already handles deletion for the `True` return path, so a reader expects `False` to mean "no action taken." The covert delete on the error path will fire even if a caller simply wanted to check the version without intending to clean up. Consider separating the concern — either rename to something like `_version_matches_or_delete_corrupt` to make the side-effect visible, or move the `self.redis.delete(key)` call back into `_delete_keys_with_version` after catching the exception there.
### Issue 2 of 3
posthog/test/test_utils.py:299-351
**New edge-case assertions embedded in the existing test rather than parameterised**
The team prefers parameterised tests. The six new `with` blocks (missing `win32_ver`, empty `mac_ver`, absent `mac_ver`, empty Linux distro, missing `release`) are appended inline inside `test_get_os_info` rather than expressed as `@pytest.mark.parametrize` cases. Inlining them increases the method's size and makes it harder to identify which scenario failed on a CI failure. Each private helper (`_get_windows_os_info`, `_get_macos_info`, `_get_linux_os_info`, `_platform_release`) could have its own parameterised test covering both the happy path and the fallback path.
### Issue 3 of 3
posthog/test/test_utils.py:713-722
**`test_redis_cache_key_helpers` bundles unrelated scenarios into one test**
The test exercises `_redis_key_to_string`, `_key_has_version` (no data), `_key_has_version` (corrupt JSON + implicit deletion), and the resulting store mutation — four distinct behaviours in a single method. Consistent with the team's preference for parameterised/focused tests, splitting these into individual tests (or a `@pytest.mark.parametrize` table) would make failure messages immediately actionable without having to mentally trace which assertion fired.
Reviews (1): Last reviewed commit: "chore: add utils CRAP score check" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _key_has_version(self, key, old_version): | ||
| try: | ||
| data = self.redis.get(key) | ||
| if not data: | ||
| return False | ||
| return json.loads(data).get("flag_version") == old_version | ||
| except (json.JSONDecodeError, KeyError): | ||
| # If we can't parse the entry, delete it to be safe | ||
| self.redis.delete(key) | ||
| return False |
There was a problem hiding this comment.
_key_has_version reads as a pure boolean predicate, but it silently deletes the Redis key when JSON parsing fails. The caller _delete_keys_with_version already handles deletion for the True return path, so a reader expects False to mean "no action taken." The covert delete on the error path will fire even if a caller simply wanted to check the version without intending to clean up. Consider separating the concern — either rename to something like _version_matches_or_delete_corrupt to make the side-effect visible, or move the self.redis.delete(key) call back into _delete_keys_with_version after catching the exception there.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/utils.py
Line: 415-424
Comment:
**Hidden deletion side-effect in a predicate method**
`_key_has_version` reads as a pure boolean predicate, but it silently deletes the Redis key when JSON parsing fails. The caller `_delete_keys_with_version` already handles deletion for the `True` return path, so a reader expects `False` to mean "no action taken." The covert delete on the error path will fire even if a caller simply wanted to check the version without intending to clean up. Consider separating the concern — either rename to something like `_version_matches_or_delete_corrupt` to make the side-effect visible, or move the `self.redis.delete(key)` call back into `_delete_keys_with_version` after catching the exception there.
How can I resolve this? If you propose a fix, please make it concise.| @@ -304,6 +308,15 @@ def test_get_os_info_branches(self): | |||
| ): | |||
| assert utils.get_os_info() == {"$os": "Mac OS X", "$os_version": "14.4"} | |||
|
|
|||
| with mock.patch.object( | |||
| utils.platform, "mac_ver", return_value=("", ("", "", ""), "") | |||
| ): | |||
| assert utils._get_macos_info() == ("Mac OS X", "", "") | |||
|
|
|||
| with mock.patch.object(utils.platform, "mac_ver", create=True): | |||
| delattr(utils.platform, "mac_ver") | |||
| assert utils._get_macos_info() == ("Mac OS X", "", "") | |||
|
|
|||
| with ( | |||
| mock.patch.object(utils.sys, "platform", "linux"), | |||
| mock.patch.object(utils.distro, "info", return_value={"version": "24.04"}), | |||
| @@ -315,6 +328,12 @@ def test_get_os_info_branches(self): | |||
| "$os_distro": "Ubuntu", | |||
| } | |||
|
|
|||
| with ( | |||
| mock.patch.object(utils.distro, "info", return_value={"version": ""}), | |||
| mock.patch.object(utils.distro, "name", return_value=""), | |||
| ): | |||
| assert utils._get_linux_os_info() == ("Linux", "", "") | |||
|
|
|||
| with ( | |||
| mock.patch.object(utils.sys, "platform", "freebsd13"), | |||
| mock.patch.object(utils.platform, "release", return_value="13.2"), | |||
| @@ -327,6 +346,10 @@ def test_get_os_info_branches(self): | |||
| ): | |||
| assert utils.get_os_info() == {"$os": "sunos", "$os_version": "5.11"} | |||
|
|
|||
| with mock.patch.object(utils.platform, "release", create=True): | |||
| delattr(utils.platform, "release") | |||
| assert utils._platform_release() == "" | |||
There was a problem hiding this comment.
The team prefers parameterised tests. The six new with blocks (missing win32_ver, empty mac_ver, absent mac_ver, empty Linux distro, missing release) are appended inline inside test_get_os_info rather than expressed as @pytest.mark.parametrize cases. Inlining them increases the method's size and makes it harder to identify which scenario failed on a CI failure. Each private helper (_get_windows_os_info, _get_macos_info, _get_linux_os_info, _platform_release) could have its own parameterised test covering both the happy path and the fallback path.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/test_utils.py
Line: 299-351
Comment:
**New edge-case assertions embedded in the existing test rather than parameterised**
The team prefers parameterised tests. The six new `with` blocks (missing `win32_ver`, empty `mac_ver`, absent `mac_ver`, empty Linux distro, missing `release`) are appended inline inside `test_get_os_info` rather than expressed as `@pytest.mark.parametrize` cases. Inlining them increases the method's size and makes it harder to identify which scenario failed on a CI failure. Each private helper (`_get_windows_os_info`, `_get_macos_info`, `_get_linux_os_info`, `_platform_release`) could have its own parameterised test covering both the happy path and the fallback path.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| failing_cache.invalidate_version(1) | ||
| failing_cache.clear() | ||
|
|
||
| def test_redis_cache_key_helpers(self): | ||
| key = self.cache._get_cache_key("user123", "missing") | ||
| invalid_key = self.cache._get_cache_key("user123", "invalid") | ||
| self.redis.store[invalid_key] = "not json" | ||
|
|
||
| assert self.cache._redis_key_to_string(key) == key | ||
| assert self.cache._key_has_version(key, 1) is False |
There was a problem hiding this comment.
test_redis_cache_key_helpers bundles unrelated scenarios into one test
The test exercises _redis_key_to_string, _key_has_version (no data), _key_has_version (corrupt JSON + implicit deletion), and the resulting store mutation — four distinct behaviours in a single method. Consistent with the team's preference for parameterised/focused tests, splitting these into individual tests (or a @pytest.mark.parametrize table) would make failure messages immediately actionable without having to mentally trace which assertion fired.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/test_utils.py
Line: 713-722
Comment:
**`test_redis_cache_key_helpers` bundles unrelated scenarios into one test**
The test exercises `_redis_key_to_string`, `_key_has_version` (no data), `_key_has_version` (corrupt JSON + implicit deletion), and the resulting store mutation — four distinct behaviours in a single method. Consistent with the team's preference for parameterised/focused tests, splitting these into individual tests (or a `@pytest.mark.parametrize` table) would make failure messages immediately actionable without having to mentally trace which assertion fired.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
posthog-python Compliance ReportDate: 2026-05-16 00:55:05 UTC
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 517ms |
| Format Validation.Event Has Uuid | ✅ | 1508ms |
| Format Validation.Event Has Lib Properties | ✅ | 1507ms |
| Format Validation.Distinct Id Is String | ✅ | 1507ms |
| Format Validation.Token Is Present | ✅ | 1507ms |
| Format Validation.Custom Properties Preserved | ✅ | 1506ms |
| Format Validation.Event Has Timestamp | ✅ | 1507ms |
| Retry Behavior.Retries On 503 | ✅ | 9519ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 3508ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 3507ms |
| Retry Behavior.Respects Retry After Header | ✅ | 9511ms |
| Retry Behavior.Implements Backoff | ✅ | 23528ms |
| Retry Behavior.Retries On 500 | ✅ | 7506ms |
| Retry Behavior.Retries On 502 | ✅ | 7511ms |
| Retry Behavior.Retries On 504 | ✅ | 7517ms |
| Retry Behavior.Max Retries Respected | ✅ | 23517ms |
| Deduplication.Generates Unique Uuids | ✅ | 1508ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 7512ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 14523ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 7509ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 1503ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 1507ms |
| Compression.Sends Gzip When Enabled | ✅ | 1507ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 1507ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 1005ms |
| Batch Format.Multiple Events Batched Together | ✅ | 1505ms |
| Error Handling.Does Not Retry On 403 | ✅ | 3510ms |
| Error Handling.Does Not Retry On 413 | ✅ | 3507ms |
| Error Handling.Retries On 408 | ✅ | 7509ms |
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 509ms |
| Request Payload.Flags Request Uses V2 Query Param | ❌ | 300142ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ❌ | 301088ms |
| Request Payload.Flags Request Omits Authorization Header | ❌ | 300960ms |
| Request Payload.Token In Flags Body Matches Init | ❌ | 301048ms |
| Request Payload.Groups Round Trip | ❌ | 300938ms |
| Request Payload.Groups Default To Empty Object | ❌ | 301054ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ❌ | 300928ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ❌ | 300983ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 301017ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ❌ | 301080ms |
| Request Lifecycle.No Flags Request On Init Alone | ❌ | 300960ms |
| Request Lifecycle.No Flags Request On Normal Capture | ❌ | 300984ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ❌ | 301033ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 301015ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 300914ms |
Failures
request_payload.request_with_person_properties_device_id
Field 'token' not found in /flags request body at path 'token'. Available keys: ['distinct_id', 'groups', 'person_properties', 'group_properties', 'geoip_disable', 'device_id', 'flag_keys_to_evaluate', 'sentAt', 'api_key']
request_payload.flags_request_uses_v2_query_param
No error message
request_payload.flags_request_hits_flags_path_not_decide
No error message
request_payload.flags_request_omits_authorization_header
No error message
request_payload.token_in_flags_body_matches_init
No error message
request_payload.groups_round_trip
No error message
request_payload.groups_default_to_empty_object
No error message
request_payload.person_properties_distinct_id_auto_populated_when_caller_omits_it
No error message
request_payload.disable_geoip_false_propagates_as_geoip_disable_false
No error message
request_payload.disable_geoip_omitted_defaults_to_false
No error message
request_payload.flag_keys_to_evaluate_contains_only_requested_key
No error message
request_lifecycle.no_flags_request_on_init_alone
No error message
request_lifecycle.no_flags_request_on_normal_capture
No error message
request_lifecycle.two_flag_calls_produce_two_remote_requests
No error message
request_lifecycle.mock_response_value_is_returned_to_caller
No error message
side_effect_events.get_feature_flag_captures_feature_flag_called_event
No error message
1 participant
💡 Motivation and Context
follow up #576
Follow-up to the targeted mutation-testing PR: add a CRAP-score gate for
posthog/utils.pyusingpytest-crapinstead of a hardcoded CRAP formula. This keeps the complexity/coverage quality bar explicit in CI while avoiding custom reimplementation of CRAP scoring.💚 How did you test it?
uv run --extra test --with pytest-crap --with pytest-cov pytest posthog/test/test_utils.py posthog/test/test_size_limited_dict.py --timeout=30 --cov=posthog.utils --crap --crap-threshold=10 -quv run --extra test --with pytest-crap --with pytest-cov python .github/scripts/check_crap_threshold.py posthog/utils.py --max-crap 10rm -f .coverage && rm -rf mutants && uv run --extra test --with mutmut mutmut run --max-children 1 && uv run --extra test --with mutmut mutmut resultsuv run --extra test pytest --timeout=30 -quvx ruff check .github/scripts/check_crap_threshold.py posthog/utils.py posthog/test/test_utils.py posthog/test/test_size_limited_dict.pyruby -e 'require "yaml"; YAML.load_file(".github/workflows/ci.yml"); puts "ci yaml ok"'📝 Checklist
If releasing new changes
sampo addto generate a changeset file