Skip to content

fix: support monorepo hoisted dependencies in JS requirements check#1376

Merged
Saga4 merged 5 commits into
mainfrom
fix/js-requirements-monorepo-support
Feb 4, 2026
Merged

fix: support monorepo hoisted dependencies in JS requirements check#1376
Saga4 merged 5 commits into
mainfrom
fix/js-requirements-monorepo-support

Conversation

@mohammedahmed18

Copy link
Copy Markdown
Contributor

Problem

The verify_requirements() method in JavaScript support only checked for test frameworks (jest/vitest) in the local package's node_modules. In monorepos with workspace dependency hoisting (yarn workspaces, pnpm workspaces, etc.), dependencies are often installed at the workspace root instead of in each package.

This caused false positive warnings like:

WARNING  JavaScript requirements check found issues:
WARNING    - jest is not installed. Please run 'npm install --save-dev jest' to install it.

Even though jest was properly installed at the workspace root.

Solution

  • Check both local node_modules and workspace root node_modules
  • Use existing _find_monorepo_root() helper to locate workspace root
  • Add debug logging to show where framework was found
  • Update docstring to document monorepo support

Testing

Tested with Budibase monorepo where jest is installed at:

  • /home/ubuntu/budibase/packages/shared-core/node_modules/jest (doesn't exist)
  • /home/ubuntu/budibase/node_modules/jest (workspace root - now detected)

Related

This complements the monorepo support already added to:

  • jest-runner resolution in loop-runner.js
  • Jest benchmarking in test_runner.py

mohammedahmed18 and others added 3 commits February 4, 2026 09:31
The verify_requirements() method only checked for test frameworks (jest/vitest)
in the local package's node_modules. In monorepos with workspace hoisting (yarn/pnpm),
dependencies are often installed at the workspace root instead.

Changes:
- Check both local node_modules and workspace root node_modules
- Use _find_monorepo_root() to locate workspace root
- Add debug logging for framework resolution
- Update docstring to document monorepo support

Fixes false positive "jest is not installed" warnings in monorepo projects
where jest is hoisted to the workspace root.

Tested with Budibase monorepo where jest is at workspace root.
@claude

claude Bot commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

✅ Prek Checks

All linting and formatting checks passed successfully. No issues found.

✅ Mypy Type Checks

The changed lines in this PR do not introduce any new type errors. All mypy errors reported are pre-existing issues in the codebase and are unrelated to this PR's changes.

📋 Code Review

Status: No critical issues found

This PR successfully adds monorepo support for hoisted dependencies in JavaScript requirements checking. The implementation:

  1. New function find_node_modules_with_package() (init_javascript.py:149-172)

    • Well-documented with clear docstring
    • Properly searches up the directory tree for node_modules
    • Returns Path | None appropriately
    • Clean implementation with no issues
  2. Modified verify_requirements() method (support.py:1846-1905)

    • Uses the new helper function correctly
    • Maintains backward compatibility
    • Adds debug logging for visibility
    • Error messages are preserved and appropriate

Existing Review Comments:

  • One comment marked as "✅ Fixed in latest commit" - the helper function was successfully moved to init_javascript.py
  • One comment about missing test coverage remains open (see coverage analysis below)

📊 Test Coverage Analysis

File Main Branch This PR Change
codeflash/cli_cmds/init_javascript.py 29.9% 41.1% +11.2%
codeflash/languages/javascript/support.py 71.2% 73.6% +2.4%
Overall Coverage 78.84% 78.98% +0.14%

Coverage Details:

  • NEW function find_node_modules_with_package(): All code paths are executed by existing tests
    • Lines 149, 164-172 are covered
    • The function is properly tested through the verify_requirements() method calls
  • MODIFIED verify_requirements() method: Most changes are covered
    • Lines 1880, 1881, 1887, 1889-1891, 1894-1896, 1900, 1905 are covered
    • The new monorepo fallback logic is exercised

Coverage Assessment:

  • ✅ No coverage regression - overall coverage increased by +0.14%
  • ✅ Modified code is well-covered by existing tests
  • ⚠️ While the new function is covered by existing tests, the specific monorepo scenario (workspace root with hoisted dependencies) could benefit from an explicit test case as noted in the previous review comment

🔀 Codeflash Optimization PRs

Checked 30 open optimization PRs from codeflash-ai[bot]. None are ready to merge - all have failing CI checks (unit tests, type checks, or pr-review failures).


Last updated: 2026-02-04

rel_path = source_file.relative_to(project_root)
return "../" + rel_path.with_suffix("").as_posix()

def verify_requirements(self, project_root: Path, test_framework: str = "jest") -> tuple[bool, list[str]]:

@Saga4 Saga4 Feb 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Fixed in latest commit - moved to init_javascript.py as find_node_modules_with_package()

Comment on lines +1897 to +1906
# If not found locally, check for hoisted dependencies in monorepo workspace root
if not framework_found:
from codeflash.languages.javascript.test_runner import _find_monorepo_root

workspace_root = _find_monorepo_root(project_root)
if workspace_root:
workspace_framework = workspace_root / "node_modules" / test_framework
if workspace_framework.exists():
framework_found = True
logger.debug("Found %s in workspace root node_modules at %s", test_framework, workspace_framework)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Missing test coverage: This monorepo fallback logic is not covered by any tests.

The existing tests in test_javascript_requirements.py only verify the case where the test framework is in local node_modules. Consider adding a test case like:

def test_verify_requirements_monorepo_hoisted_dependencies(self, js_support, tmp_path):
    """Test that frameworks in workspace root node_modules are detected."""
    # Create package without local node_modules
    package_root = tmp_path / "packages" / "shared-core"
    package_root.mkdir(parents=True)
    
    # Create workspace root with jest
    workspace_node_modules = tmp_path / "node_modules"
    workspace_node_modules.mkdir()
    (workspace_node_modules / "jest").mkdir()
    
    # Add workspace marker (e.g., yarn.lock)
    (tmp_path / "yarn.lock").touch()
    
    with patch("subprocess.run") as mock_run:
        mock_run.return_value = MagicMock(returncode=0)
        
        success, errors = js_support.verify_requirements(package_root, "jest")
        
        assert success is True
        assert errors == []

This would ensure the monorepo support actually works as intended.

- Add find_node_modules_with_package() to init_javascript.py
- Uses same tree-search pattern as determine_js_package_manager()
- Simplify verify_requirements() to use the new helper
- Reduces code duplication and centralizes monorepo logic

Benefits:
- Consistent monorepo support across codebase
- Single source of truth for finding node_modules
- Easier to maintain and test
- Works alongside determine_js_package_manager() pattern
@Saga4 Saga4 merged commit 0e5bf04 into main Feb 4, 2026
25 of 27 checks passed
@Saga4 Saga4 deleted the fix/js-requirements-monorepo-support branch February 4, 2026 12:59
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