fix(tests): update imports from kaizen to evolve module#126
Conversation
📝 WalkthroughWalkthroughThe dev dependency group in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test_full_flow.py (2)
53-54: Content slicing assumes string type.Same concern as in
test_filesystem_guidelines.py:EntityUpdate.contentcan bestr | list | dict, but[:80]will fail for dict types.Proposed fix
for update in updates: - print(f" - {update.event}: {update.content[:80]}...") + content_preview = str(update.content)[:80] + print(f" - {update.event}: {content_preview}...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_full_flow.py` around lines 53 - 54, The test prints assume update.content is a string and slices with update.content[:80], which will crash for dict/list types; update references are instances with the field EntityUpdate.content. Fix by normalizing content to a string before slicing—e.g., convert non-str content to a reliable string representation (repr or json.dumps) and then take the first 80 chars—so replace the direct slice on update.content with a safe stringification of update.content in the loop over updates.
16-50: Consider ensuring the namespace exists before use.Unlike
test_filesystem_guidelines.py, this script doesn't verify the namespace exists before callingupdate_entities. Based on the base backend implementation inevolve/backend/base.py,update_entitiescalls_validate_namespacewhich raisesNamespaceNotFoundExceptionif the namespace doesn't exist.If this is an intentional design choice (assuming setup is done elsewhere), you could document that assumption. Otherwise, adding a namespace check similar to
test_filesystem_guidelines.pywould make the test more robust for standalone execution.Proposed fix
+from evolve.schema.exceptions import NamespaceNotFoundException + def test_full_flow(): """Test creating and retrieving guidelines.""" print("=" * 60) print("Testing Full Evolve Flow") print("=" * 60) client = EvolveClient() + + # Ensure namespace exists + namespace_id = evolve_config.namespace_id + try: + ns = client.get_namespace_details(namespace_id) + print(f"Using namespace: {ns.id} (entities: {ns.num_entities})") + except NamespaceNotFoundException: + print(f"Creating namespace: {namespace_id}") + client.create_namespace(namespace_id) # Step 1: Create some sample guidelines🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_full_flow.py` around lines 16 - 50, The test calls EvolveClient.update_entities without ensuring the namespace exists, which can trigger NamespaceNotFoundException from the backend's _validate_namespace; add a pre-check similar to test_filesystem_guidelines.py by calling EvolveClient.get_namespace or a provided namespace-existence helper (or create the namespace via EvolveClient.create_namespace) for evolve_config.namespace_id before invoking update_entities, and handle or fail clearly if the namespace is missing so the test is robust for standalone execution.test_filesystem_guidelines.py (2)
29-34: Consider catching the specific exception.Using bare
except Exceptionis broad. Based on the relevant code snippet inevolve/backend/filesystem.py, theget_namespace_detailsmethod raisesNamespaceNotFoundExceptionwhen the namespace doesn't exist.Proposed fix
+from evolve.schema.exceptions import NamespaceNotFoundException + # ... later in the function ... try: ns = client.get_namespace_details(namespace_id) print(f"Namespace: {ns.id} (entities: {ns.num_entities})") - except Exception: + except NamespaceNotFoundException: print(f"Creating namespace: {namespace_id}") client.create_namespace(namespace_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_filesystem_guidelines.py` around lines 29 - 34, Replace the broad except block around client.get_namespace_details(namespace_id) with a specific handler for NamespaceNotFoundException: catch NamespaceNotFoundException to detect a missing namespace and call client.create_namespace(namespace_id) in that branch, and let other exceptions propagate (or handle them separately) so only the intended "namespace not found" case triggers creation; reference get_namespace_details, NamespaceNotFoundException, and create_namespace on the client to locate the change.
59-60: Content slicing assumes string type.
EntityUpdate.contentis typed asstr | list | dict(perevolve/schema/conflict_resolution.py). Slicing with[:60]will fail withTypeErrorfor dict content. While the test data here is all strings, consider a defensive approach:Proposed fix
for update in updates: - print(f" - {update.event}: {update.content[:60]}...") + content_preview = str(update.content)[:60] + print(f" - {update.event}: {content_preview}...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_filesystem_guidelines.py` around lines 59 - 60, The loop that prints updates assumes update.content is a string and uses slicing (for update in updates -> print(f" - {update.event}: {update.content[:60]}...")), but EntityUpdate.content can be str | list | dict; update.content[:60] will raise TypeError for dict/list. Fix by normalizing content before slicing: convert non-str values (list/dict) to a string representation (e.g., json.dumps or str()) then safely take the first 60 characters, and use that normalized variable in the print; ensure you handle None if applicable. Reference the loop variable update, the updates iterable, and EntityUpdate.content when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test_filesystem_guidelines.py`:
- Around line 29-34: Replace the broad except block around
client.get_namespace_details(namespace_id) with a specific handler for
NamespaceNotFoundException: catch NamespaceNotFoundException to detect a missing
namespace and call client.create_namespace(namespace_id) in that branch, and let
other exceptions propagate (or handle them separately) so only the intended
"namespace not found" case triggers creation; reference get_namespace_details,
NamespaceNotFoundException, and create_namespace on the client to locate the
change.
- Around line 59-60: The loop that prints updates assumes update.content is a
string and uses slicing (for update in updates -> print(f" - {update.event}:
{update.content[:60]}...")), but EntityUpdate.content can be str | list | dict;
update.content[:60] will raise TypeError for dict/list. Fix by normalizing
content before slicing: convert non-str values (list/dict) to a string
representation (e.g., json.dumps or str()) then safely take the first 60
characters, and use that normalized variable in the print; ensure you handle
None if applicable. Reference the loop variable update, the updates iterable,
and EntityUpdate.content when applying the change.
In `@test_full_flow.py`:
- Around line 53-54: The test prints assume update.content is a string and
slices with update.content[:80], which will crash for dict/list types; update
references are instances with the field EntityUpdate.content. Fix by normalizing
content to a string before slicing—e.g., convert non-str content to a reliable
string representation (repr or json.dumps) and then take the first 80 chars—so
replace the direct slice on update.content with a safe stringification of
update.content in the loop over updates.
- Around line 16-50: The test calls EvolveClient.update_entities without
ensuring the namespace exists, which can trigger NamespaceNotFoundException from
the backend's _validate_namespace; add a pre-check similar to
test_filesystem_guidelines.py by calling EvolveClient.get_namespace or a
provided namespace-existence helper (or create the namespace via
EvolveClient.create_namespace) for evolve_config.namespace_id before invoking
update_entities, and handle or fail clearly if the namespace is missing so the
test is robust for standalone execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8247da80-72c8-43af-b13f-cbadbca85d31
📒 Files selected for processing (3)
test_filesystem_guidelines.pytest_full_flow.pytests/unit/test_postgres_backend.py
visahak
force-pushed
the
fix/update-test-imports-kaizen-to-evolve
branch
3 times, most recently
from
7b0f2db to
07545b6
Compare
Added psycopg[binary]>=3.1 and pgvector>=0.3 to dev dependency group to ensure all unit tests can run during development and CI. This fixes the test collection error for test_postgres_backend.py while keeping postgres support optional for end users (via the pgvector optional dependency group).
visahak
force-pushed
the
fix/update-test-imports-kaizen-to-evolve
branch
from
07545b6 to
317217d
Compare
gaodan-fang
left a comment
gaodan-fang
left a comment
There was a problem hiding this comment.
Updated test_filesystem_guidelines.py to use evolve module instead of kaizen
Updated test_full_flow.py to use evolve module instead of kaizen
Added pytest.importorskip for psycopg in test_postgres_backend.py to skip tests when optional dependency is not installed
I didn't see these files to be changed, forgot to commit?
| "anyio", | ||
| "detect-secrets", | ||
| "mypy", | ||
| "pgvector>=0.3", |
There was a problem hiding this comment.
I'm hesitate that we add these dependencies in the dev section, as those are optional. They can install by running altk-evolve[pgvector]
There was a problem hiding this comment.
@gaodan-fang based on your comment above, do you want me to close this PR and you can submit a new one?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyproject.toml (1)
51-53: Consolidatepgvectorandpsycopgby referencing the project extra in dev dependencies.The same version constraints appear in both
[project.optional-dependencies].pgvector(lines 31–34) and[dependency-groups].dev(lines 51, 53). Rather than duplicating, use uv's support for self-referencing project extras: replace lines 51 and 53 with a single entry"evolve[pgvector]"in the dev group. This eliminates version drift risk and maintains a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 51 - 53, The dev dependency list duplicates the version constraints for "pgvector>=0.3" and "psycopg[binary]>=3.1" that are already declared in the project extra named "pgvector"; update the dependency-group "dev" to reference that extra instead of repeating both entries by replacing the two entries with a single reference to "evolve[pgvector]" so the dev group consumes the project extra and avoids duplicate version constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyproject.toml`:
- Around line 51-53: The dev dependency list duplicates the version constraints
for "pgvector>=0.3" and "psycopg[binary]>=3.1" that are already declared in the
project extra named "pgvector"; update the dependency-group "dev" to reference
that extra instead of repeating both entries by replacing the two entries with a
single reference to "evolve[pgvector]" so the dev group consumes the project
extra and avoids duplicate version constraints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81a9190e-a48d-4dae-8763-f507b9ae131b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml
Turns out they are old files that the AI assistant committed by mistake, since there are already similar tests. |
visahak
force-pushed
the
fix/update-test-imports-kaizen-to-evolve
branch
from
7732420 to
6d9de37
Compare
|
Closing this PR. |
2 participants
Error observed when running
uv run pytest- Updated test_filesystem_guidelines.py to use evolve module instead of kaizen- Updated test_full_flow.py to use evolve module instead of kaizen- Added pytest.importorskip for psycopg in test_postgres_backend.py to skip tests when optional dependency is not installedFixes 3 test collection errors that were preventing pytest from running successfully.
Summary by CodeRabbit