Skip to content

fix: handle co-located test directories with traverse_up#1979

Open
mohammedahmed18 wants to merge 2 commits intomainfrom
fix/colocated-test-path-resolution
Open

fix: handle co-located test directories with traverse_up#1979
mohammedahmed18 wants to merge 2 commits intomainfrom
fix/colocated-test-path-resolution

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

Problem

When optimizing functions in projects with co-located __tests__ directories, the CLI crashes with:

ValueError: '/workspace/target/src/gateway/server/__tests__/codeflash-generated/test_xxx.test.ts' 
is not in the subpath of '/workspace/target/test'

This occurs when:

  • Source file: src/gateway/server/ws-connection/connect-policy.ts
  • Configured tests_root: test/
  • Generated test location: src/gateway/server/__tests__/codeflash-generated/

Root Cause

The CLI's _find_codeflash_test_dir() method intelligently places tests near their source files by looking for co-located __tests__ directories. However, verifier.py line 37 tries to compute a module path relative to the configured tests_root:

# Before (buggy):
test_module_path = Path(module_name_from_file_path(test_path, test_cfg.tests_project_rootdir))

When the test path is NOT under tests_root, this fails because module_name_from_file_path() doesn't traverse up by default.

Solution

Enable the traverse_up parameter:

# After (fixed):
test_module_path = Path(module_name_from_file_path(
    test_path, test_cfg.tests_project_rootdir, traverse_up=True
))

With traverse_up=True, the function can find a common ancestor directory and compute the relative path from there, properly handling tests outside the configured tests_root.

Testing

Added comprehensive unit tests:

  • ✅ Normal case: file inside project root
  • ✅ Error case: file outside root without traverse_up
  • ✅ Success case: file outside root with traverse_up
  • ✅ Real-world scenario: co-located tests structure
  • ✅ All existing tests pass
  • ✅ Linting passes

Impact

  • Fixes crashes when working with co-located test directories
  • Enables proper handling of monorepo structures
  • Preserves compatibility with traditional test/ directories

References

Trace ID: 7b97ddba-6ecd-42fd-b572-d40658746836

This fixes a ValueError that occurs when generated tests are placed in
co-located __tests__ directories outside the configured tests_root.

Root cause:
The CLI's _find_codeflash_test_dir() method generates tests in co-located
__tests__ directories (e.g., src/gateway/server/__tests__/) when they exist,
but verifier.py tried to compute a module path relative to the configured
tests_root (e.g., /workspace/target/test), causing:

ValueError: '/workspace/target/src/gateway/server/__tests__/codeflash-generated/test_xxx.test.ts'
is not in the subpath of '/workspace/target/test'

Fix:
- Added traverse_up=True to module_name_from_file_path() call in verifier.py
- This allows the function to find a common ancestor directory and compute
  the relative path from there, handling tests outside tests_root

Testing:
- Added comprehensive unit tests in test_module_name_from_file_path.py
- All existing tests pass ✅
- Linting passes ✅

Impact:
- Resolves crashes when optimizing functions with co-located test directories
- Enables proper handling of monorepo and __tests__ directory structures

Trace IDs affected: 7b97ddba-6ecd-42fd-b572-d40658746836

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Codeflash Bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 3, 2026

Claude finished @mohammedahmed18's task in 2m 39s —— View job


PR Review Summary

  • Triage PR size — SMALL (3 lines production code changed in verifier.py + 85 lines of new tests)
  • Run lint/typecheck checks
  • Resolve stale review threads — none found
  • Code review
  • Duplicate detection

Prek Checks

Ruff checks passed. Mypy reports 2 pre-existing errors in verifier.py (lines 63 and 120) unrelated to this PR — not introduced by this change.


Code Review

The fix is correct. Adding traverse_up=True prevents the ValueError crash when co-located __tests__ directories are outside the configured tests_root.

How traverse_up works (code_utils.py:354): when the file isn't under project_root_path, the function walks up from file_path.parent and returns the first relative path it can compute. In practice, for a file outside the root, this always succeeds on the first iteration and returns just the filename stem (e.g., test_xxx.test). This is acceptable for test_module_path — the AI service just needs a stable file identifier.

Minor observations:

  1. Verbose comment (verifier.py:37-38): The two-line comment explains what the code does rather than why, which goes against this project's comment style (CLAUDE.md: "only explain 'why', not 'what'"). The parameter name traverse_up=True is self-documenting enough. Fix this →

  2. Docstrings in tests (tests/test_module_name_from_file_path.py): The test class and methods all have docstrings, which conflicts with the project guideline ("Do not add docstrings to new or changed code unless explicitly asked"). Fix this →

  3. Weak test assertion (tests/test_module_name_from_file_path.py:49): test_file_outside_project_root_with_traverse_up only asserts "test_foo" in result and not result.startswith("."). A stronger assertion would be assert result == "test_foo" since that's the exact expected output.

No bugs, security issues, or breaking API changes.


Duplicate Detection

No duplicates detected. traverse_up=True is only added in one call site.


Last updated: 2026-04-03T13:37Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants