test: cover determine_project_for_conversation fallback chain (closes #87)#89
test: cover determine_project_for_conversation fallback chain (closes #87)#89bradjin8 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a comprehensive unittest + Hypothesis suite exercising all stages of determine_project_for_conversation: helpers and setup, definitive mapping, project-layouts, file-path stages, bubble-context stages, path-segment fallback, terminal/malformed cases, and property-based fuzz tests. ChangesWorkspace Resolver Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_workspace_assignment_fallback.py (1)
20-25: 💤 Low value
sys.pathinsertion runs after the import it's meant to enable.
from services.workspace_resolver import ...(Line 20) executes beforeREPO_ROOTis inserted intosys.path(Lines 23-25). If the repo root weren't already importable, the import on Line 20 would fail first, so the path manipulation is effectively dead code. Move thesys.pathsetup above the project imports to make it meaningful.♻️ Proposed reordering
import unittest from hypothesis import given, settings from hypothesis import strategies as st -from services.workspace_resolver import determine_project_for_conversation -from utils.path_helpers import normalize_file_path - REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) if REPO_ROOT not in sys.path: sys.path.insert(0, REPO_ROOT) + +from services.workspace_resolver import determine_project_for_conversation +from utils.path_helpers import normalize_file_path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workspace_assignment_fallback.py` around lines 20 - 25, The sys.path insertion (REPO_ROOT and sys.path.insert(0, REPO_ROOT)) currently comes after the imports and is therefore ineffective; move the REPO_ROOT computation and the sys.path insertion so they occur before any project imports (e.g., before the lines importing determine_project_for_conversation and normalize_file_path) so that the module import of services.workspace_resolver will succeed when the repo root is not already on sys.path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_workspace_assignment_fallback.py`:
- Around line 20-25: The sys.path insertion (REPO_ROOT and sys.path.insert(0,
REPO_ROOT)) currently comes after the imports and is therefore ineffective; move
the REPO_ROOT computation and the sys.path insertion so they occur before any
project imports (e.g., before the lines importing
determine_project_for_conversation and normalize_file_path) so that the module
import of services.workspace_resolver will succeed when the repo root is not
already on sys.path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6625dfb6-be45-43aa-a90d-3189c2576dec
📒 Files selected for processing (1)
tests/test_workspace_assignment_fallback.py
Should fixtests/test_workspace_assignment_fallback.py (new test) — Add a conflicting-signals case where two non-definitive stages disagree (e.g. newlyCreatedFiles → workspace A, codeBlockData → workspace B) and assert which workspace wins. Issue #87 calls out “conflicting signals” explicitly; today only definitive-vs-layout conflict is covered (test_definitive_mapping_wins_over_conflicting_layouts). Stage order is the core contract of this function; a single ordering test would guard against accidental reordering during refactors. tests/test_workspace_assignment_fallback.py:136 — Rename or split test_missing_project_layouts_key_falls_through. The name implies the projectLayouts stage; the case actually exercises newlyCreatedFiles + workspace_entries with no project_layouts_map entry. Behavior is correct; the name misleads future maintainers. Nice to havetests/test_workspace_assignment_fallback.py — Add a path-segment case driven only by bubble fields (relevantFiles / attachments) on a path outside workspace roots (today path-segment coverage uses newlyCreatedFiles only). tests/test_workspace_assignment_fallback.py — Hypothesis property: when composer_id_to_workspace_id maps to an ID in invalid_workspace_ids and no other stage can match, result is never that invalid ID. tests/test_workspace_assignment_fallback.py — Explicit project_layouts_map entry with unresolvable roots (missing from workspace_path_to_id / project_name_to_workspace_id) to document fall-through to file-path stages. |
2 participants
Summary
Closes #87
tests/test_workspace_assignment_fallback.pyfrom 1 test to 21 (19 unit + 2 Hypothesis) fordetermine_project_for_conversationinservices/workspace_resolver.py.projectLayouts,newlyCreatedFiles,codeBlockData, bubble header paths (relevantFiles,attachedFileCodeChunksUris,context.fileSelections), path-segment last resort, and terminalNone.Test plan
python -m pytest tests/test_workspace_assignment_fallback.py -vSummary by CodeRabbit