Skip to content

[FIX][JS] Avoid adding .js to relative imports in tests with jest as it's using its own resolver#1999

Open
mohammedahmed18 wants to merge 2 commits intomainfrom
fix/dont-add-js-extension-to-relative-imports-with-jest
Open

[FIX][JS] Avoid adding .js to relative imports in tests with jest as it's using its own resolver#1999
mohammedahmed18 wants to merge 2 commits intomainfrom
fix/dont-add-js-extension-to-relative-imports-with-jest

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

No description provided.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 4, 2026

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


PR Review Summary

  • Triage PR size — LARGE (84 lines of production code added)
  • Run lint/typecheck (prek + mypy)
  • Resolve stale review threads — none found
  • Code review
  • Duplicate detection
  • Post summary

Prek Checks

All checks passed. No lint or formatting issues.

Code Review

Design issue (HIGH): fix_import_paths largely duplicates fix_jest_mock_paths

fix_import_paths (instrument.py:1373) and fix_jest_mock_paths (instrument.py:1289) share ~90% of their logic — same signature, identical file-existence checks, same path-rewriting algorithm, same error handling. The only real differences are the regex pattern and the log label. These should share a private helper, e.g.:

def _fix_relative_paths(test_code: str, pattern: re.Pattern[str], test_file_path: Path, source_file_path: Path, log_label: str) -> str:
    ...

Fix this →

Bug: tests_root parameter in fix_import_paths is unused

instrument.py:1373tests_root is accepted in the function signature but never referenced in the body. fix_jest_mock_paths uses it to compute project_root, but fix_import_paths dropped that line. Either use it (if needed) or remove the parameter from the signature (and update the call site in support.py:2062).

Fix this →

Minor: typo in comment, support.py:2079

"jest uses it's own resolver""jest uses its own resolver" (possessive, not contraction).

Minor: docstring on fix_import_paths

Project code style explicitly prohibits docstrings on new/changed code unless requested. The multi-line docstring at instrument.py:1374–1382 should be removed.

Duplicate Detection

HIGH confidence duplicate: fix_import_paths (instrument.py:1373) vs fix_jest_mock_paths (instrument.py:1289). Both functions are ~80 lines with identical structure: guard on empty input → resolve source_dir/test_dir → compile regex → inner fix_* closure with identical file-existence logic → call .sub(). The only semantic difference is the regex pattern. These should share a common private helper.

Test Coverage

All 8 new tests in TestFixImportPaths pass. The jest condition change in support.py:2079 is not independently unit-tested (it lives in process_generated_test_strings), but the new fix_import_paths function itself has good coverage across the key scenarios (import, require, dynamic import, empty input, package imports, concatenated imports).


Last updated: 2026-04-04T22:14Z

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.

1 participant