chore: add utils CRAP score check#577
Merged
Merged
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Contributor
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}
5 tasks
Contributor
posthog-python Compliance ReportDate: 2026-05-26 15:35:11 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
…ap-score-check # Conflicts: # .github/workflows/ci.yml # posthog/test/test_utils.py
Contributor
|
Reviews (2): Last reviewed commit: "restore useful utility comments" | Re-trigger Greptile |
Member
Author
|
merged #578 into this branch so you can skip the acceptance tests review, forgot about the stacked PRs, sorry |
dustinbyrne
approved these changes
May 26, 2026
# Conflicts: # .github/workflows/ci.yml # posthog/test/test_utils.py # posthog/utils.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 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