From 7670401d158c6442d0d335d1af445d36160378f9 Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Wed, 25 Feb 2026 13:58:11 -0800 Subject: [PATCH] Add PHPUnit test execution and Composer package management for PHP projects PHP projects like SpiderPlus had no test execution support - the agent couldn't catch PHPUnit test failures before pushing, leading to CI failures requiring extra iterations. This adds the full PHP test pipeline following the same pattern as Jest/Node. Key changes: - PHPUnit test runner (run_phpunit_test.py) with deprecation warning suppression - Composer package management via CodeBuild (ensure_php_packages.py) - vendor.tar.gz extraction alongside node_modules.tar.gz - Config template copying (.default/.example/.sample -> target) in prepare_repo_for_work - PHP CLI + Composer added to Lambda Docker image and CodeBuild buildspec - Inline reuse-check logic in both ensure_node_packages and ensure_php_packages - Integration into verify_task_is_complete and verify_task_is_ready --- CLAUDE.md | 8 +- Dockerfile | 10 + README.md | 23 +- constants/files.py | 2 + infrastructure/setup-vpc-nat-efs.yml | 27 +- .../agents/test_verify_task_is_complete.py | 12 + services/agents/test_verify_task_is_ready.py | 68 ++- services/agents/verify_task_is_complete.py | 22 +- services/agents/verify_task_is_ready.py | 21 +- services/efs/clone_and_install.py | 50 +- services/efs/copy_repo_from_efs_to_tmp.py | 6 +- services/efs/extract_dependencies.py | 59 +- services/efs/test_clone_and_install.py | 31 +- services/efs/test_extract_dependencies.py | 53 +- services/git/copy_config_templates.py | 39 ++ services/git/prepare_repo_for_work.py | 8 +- services/git/test_copy_config_templates.py | 93 +++ services/node/ensure_node_packages.py | 131 ++--- services/node/test_ensure_node_packages.py | 539 ++++++++---------- services/php/ensure_php_packages.py | 109 ++++ services/php/test_ensure_php_packages.py | 176 ++++++ services/phpunit/run_phpunit_test.py | 86 +++ services/phpunit/test_run_phpunit_test.py | 241 ++++++++ services/webhook/check_suite_handler.py | 12 +- services/webhook/new_pr_handler.py | 7 + services/webhook/review_run_handler.py | 12 +- 26 files changed, 1360 insertions(+), 485 deletions(-) create mode 100644 services/git/copy_config_templates.py create mode 100644 services/git/test_copy_config_templates.py create mode 100644 services/php/ensure_php_packages.py create mode 100644 services/php/test_ensure_php_packages.py create mode 100644 services/phpunit/run_phpunit_test.py create mode 100644 services/phpunit/test_run_phpunit_test.py diff --git a/CLAUDE.md b/CLAUDE.md index c248efaef..5a4d22b6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,16 +30,10 @@ These words mean you are GUESSING, not providing facts. ## Development Commands -### Environment Setup +### Running Locally ```bash -# Start development environment (recommended) ./start.sh - -# Manual setup if needed -python3 -m venv venv -source venv/bin/activate -pip install -r requirements.txt ``` ### Code Quality diff --git a/Dockerfile b/Dockerfile index 89aaf16ac..a470865da 100644 --- a/Dockerfile +++ b/Dockerfile @@ -15,6 +15,16 @@ RUN curl -fsSL https://rpm.nodesource.com/setup_24.x | bash - && \ dnf install -y nodejs && \ npm install -g yarn +# Install PHP CLI and Composer for PHPUnit test execution +# Same packages are also installed in CodeBuild (infrastructure/setup-vpc-nat-efs.yml) +# php-cli: PHP command-line interpreter to run PHPUnit +# php-json: JSON encoding/decoding used by PHPUnit for config and result output +# php-mbstring: Multibyte string functions required by PHPUnit for string diffing +# php-xml: DOM/XML parsing used by PHPUnit for phpunit.xml config and JUnit report generation +# php-pdo: Database abstraction layer needed by customer test suites (e.g., SpiderPlus) +RUN dnf install -y php-cli php-json php-mbstring php-xml php-pdo && \ + curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer + # Install cloc directly without adding the entire EPEL repository RUN curl -L https://github.com/AlDanial/cloc/releases/download/v1.98/cloc-1.98.pl -o /usr/local/bin/cloc && \ chmod +x /usr/local/bin/cloc diff --git a/README.md b/README.md index 0f5a3a518..2d701e21f 100644 --- a/README.md +++ b/README.md @@ -1,27 +1,6 @@ # GitAuto AI -## 1. What is GitAuto - -[GitAuto](https://gitauto.ai) is a GitHub coding agent that opens pull requests from backlog tickets for software engineering managers to complete more bug fixes and feature requests. Assign tasks to GitAuto first, and have people work on more complex tickets. - -- Want to give GitAuto a try? Go to [GitAuto installation](https://github.com/apps/gitauto-ai). -- Want to see demo videos? Go to [GitAuto YouTube](https://www.youtube.com/@gitauto). -- Want to know more about GitAuto? Go to [GitAuto homepage](https://gitauto.ai). -- Want to chat about your use case? Feel free to contact us at [email](mailto:info@gitauto.ai), [admin](https://github.com/hiroshinishio), [X](https://x.com/gitautoai), or [LinkedIn](https://www.linkedin.com/company/gitauto/). - -## 2. How to use GitAuto - -1. Install GitAuto to your repositories from [GitHub Marketplace](https://github.com/apps/gitauto-ai). - 1. Choose the repositories where you want to use GitAuto. - 2. You can change the repositories later. -2. Trigger GitAuto in one of these ways: - - **Dashboard**: Go to the [GitAuto dashboard](https://gitauto.ai/dashboard/coverage) to select files with low test coverage and create PRs. - - **Schedule**: Enable daily automation on the dashboard. GitAuto creates PRs automatically once a day at 00:00 UTC for files with low test coverage. -3. GitAuto creates a PR and works on the changes. You will get a notification once GitAuto completes. -4. Review the PR and merge it if it looks good. -5. If changes are needed, leave a review comment on the PR. GitAuto responds to review comments and updates the code. - -## 3. How to run GitAuto locally +## How to run GitAuto locally ### 3-1. Create your GitHub app for local development diff --git a/constants/files.py b/constants/files.py index 9381a599c..2bc1fab1c 100644 --- a/constants/files.py +++ b/constants/files.py @@ -1,5 +1,7 @@ JS_TS_FILE_EXTENSIONS = (".js", ".jsx", ".ts", ".tsx") +PHP_TEST_FILE_EXTENSIONS = ("Test.php",) + TS_TEST_FILE_EXTENSIONS = ( ".test.ts", ".test.tsx", diff --git a/infrastructure/setup-vpc-nat-efs.yml b/infrastructure/setup-vpc-nat-efs.yml index 4d49556ba..b6cafea8e 100644 --- a/infrastructure/setup-vpc-nat-efs.yml +++ b/infrastructure/setup-vpc-nat-efs.yml @@ -419,17 +419,28 @@ Resources: - echo "Installing packages in $EFS_DIR using $PKG_MANAGER" - LOCAL_DIR="/tmp$EFS_DIR" - mkdir -p "$LOCAL_DIR" - - tar -cf - -C "$EFS_DIR" --exclude node_modules . | tar -xf - -C "$LOCAL_DIR" - - cd "$LOCAL_DIR" - | - if [ -n "$NPM_TOKEN" ]; then - export NPM_TOKEN="$NPM_TOKEN" + if [ "$PKG_MANAGER" = "composer" ]; then + tar -cf - -C "$EFS_DIR" --exclude vendor . | tar -xf - -C "$LOCAL_DIR" + cd "$LOCAL_DIR" + # Same PHP packages as Dockerfile - see comments there for details + yum install -y php-cli php-json php-mbstring php-xml php-pdo + curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer + composer install --no-interaction --no-dev + rm -f "$EFS_DIR/vendor.tar.gz" + tar -czf "$EFS_DIR/vendor.tar.gz" -C "$LOCAL_DIR" vendor + else + tar -cf - -C "$EFS_DIR" --exclude node_modules . | tar -xf - -C "$LOCAL_DIR" + cd "$LOCAL_DIR" + if [ -n "$NPM_TOKEN" ]; then + export NPM_TOKEN="$NPM_TOKEN" + fi + $PKG_MANAGER install + rm -f "$EFS_DIR/node_modules.tar.gz" + tar -czf "$EFS_DIR/node_modules.tar.gz" -C "$LOCAL_DIR" node_modules fi - - $PKG_MANAGER install - - rm -f "$EFS_DIR/node_modules.tar.gz" - - tar -czf "$EFS_DIR/node_modules.tar.gz" -C "$LOCAL_DIR" node_modules - rm -rf "$LOCAL_DIR" - - echo "Package install complete - tarball at $EFS_DIR/node_modules.tar.gz" + - echo "Package install complete for $PKG_MANAGER in $EFS_DIR" TimeoutInMinutes: 60 VpcConfig: VpcId: !Ref VPC diff --git a/services/agents/test_verify_task_is_complete.py b/services/agents/test_verify_task_is_complete.py index c7dc7f496..d5419964f 100644 --- a/services/agents/test_verify_task_is_complete.py +++ b/services/agents/test_verify_task_is_complete.py @@ -10,6 +10,7 @@ from services.eslint.run_eslint_fix import ESLintResult from services.github.types.github_types import BaseArgs from services.jest.run_jest_test import JestResult +from services.phpunit.run_phpunit_test import PhpunitResult from services.prettier.run_prettier_fix import PrettierResult from services.tsc.run_tsc_check import TscResult @@ -51,6 +52,17 @@ def mock_jest_test(): yield +@pytest.fixture(autouse=True) +def mock_phpunit_test(): + """Auto-mock run_phpunit_test for all tests to prevent actual test execution.""" + with patch( + "services.agents.verify_task_is_complete.run_phpunit_test", + new_callable=AsyncMock, + return_value=PhpunitResult(success=True, errors=[], error_files=set()), + ): + yield + + @pytest.fixture(autouse=True) def mock_get_eslint_config(): """Auto-mock get_eslint_config for all tests.""" diff --git a/services/agents/test_verify_task_is_ready.py b/services/agents/test_verify_task_is_ready.py index 6df6608d0..c3f263ba4 100644 --- a/services/agents/test_verify_task_is_ready.py +++ b/services/agents/test_verify_task_is_ready.py @@ -36,7 +36,11 @@ async def test_valid_file_returns_success( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/index.ts"] + base_args=base_args, + file_paths=["src/index.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is True assert result.errors == [] @@ -70,7 +74,11 @@ async def test_prettier_fails_returns_errors( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/broken.ts"] + base_args=base_args, + file_paths=["src/broken.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is False assert len(result.errors) == 1 @@ -105,7 +113,11 @@ async def test_eslint_fails_returns_errors( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/broken.ts"] + base_args=base_args, + file_paths=["src/broken.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is False assert len(result.errors) == 1 @@ -127,7 +139,11 @@ async def test_non_js_files_skipped(mock_get_raw_content): }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["README.md", "config.json"] + base_args=base_args, + file_paths=["README.md", "config.json"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is True assert result.errors == [] @@ -147,7 +163,13 @@ async def test_empty_file_list(): "base_branch": "main", }, ) - result = await verify_task_is_ready(base_args=base_args, file_paths=[]) + result = await verify_task_is_ready( + base_args=base_args, + file_paths=[], + run_tsc=False, + run_jest=False, + run_phpunit=False, + ) assert result.success is True assert result.errors == [] assert result.fixes_applied == [] @@ -173,7 +195,11 @@ async def test_file_not_found_skipped( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/missing.ts"] + base_args=base_args, + file_paths=["src/missing.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is True assert result.errors == [] @@ -208,7 +234,11 @@ async def test_fixes_applied_and_pushed( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/index.ts"] + base_args=base_args, + file_paths=["src/index.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) assert result.success is True assert result.errors == [] @@ -245,7 +275,11 @@ async def test_eslint_partial_fix_pushes_and_reports_errors( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/index.ts"] + base_args=base_args, + file_paths=["src/index.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) # Lint-only errors (no-unused-vars) are ignored by verify_task_is_ready # Only coverage-relevant errors cause failure @@ -284,7 +318,11 @@ async def test_no_explicit_any_ignored( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/utils/auth0.ts"] + base_args=base_args, + file_paths=["src/utils/auth0.ts"], + run_tsc=False, + run_jest=False, + run_phpunit=False, ) # no-explicit-any is a lint-only error, not coverage-relevant # Previously this caused the agent to loop for 900s trying to fix unfixable errors @@ -327,7 +365,11 @@ async def test_run_tsc_reports_type_errors( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/index.ts"], run_tsc=True + base_args=base_args, + file_paths=["src/index.ts"], + run_tsc=True, + run_jest=False, + run_phpunit=False, ) assert result.success is False assert len(result.errors) == 1 @@ -372,7 +414,11 @@ async def test_run_jest_reports_test_failures( }, ) result = await verify_task_is_ready( - base_args=base_args, file_paths=["src/index.test.ts"], run_jest=True + base_args=base_args, + file_paths=["src/index.test.ts"], + run_tsc=False, + run_jest=True, + run_phpunit=False, ) assert result.success is False assert len(result.errors) == 2 diff --git a/services/agents/verify_task_is_complete.py b/services/agents/verify_task_is_complete.py index 8ca306d7e..30a3e6e0c 100644 --- a/services/agents/verify_task_is_complete.py +++ b/services/agents/verify_task_is_complete.py @@ -1,7 +1,11 @@ import os from dataclasses import dataclass, field -from constants.files import JS_TEST_FILE_EXTENSIONS, TS_TEST_FILE_EXTENSIONS +from constants.files import ( + JS_TEST_FILE_EXTENSIONS, + PHP_TEST_FILE_EXTENSIONS, + TS_TEST_FILE_EXTENSIONS, +) from services.eslint.ensure_eslint_relaxed_for_tests import ( ensure_eslint_relaxed_for_tests, ) @@ -20,6 +24,7 @@ from services.node.ensure_tsconfig_relaxed_for_tests import ( ensure_tsconfig_relaxed_for_tests, ) +from services.phpunit.run_phpunit_test import run_phpunit_test from services.prettier.run_prettier_fix import run_prettier_fix from services.tsc.create_tsc_issue import create_tsc_issue from services.tsc.run_tsc_check import run_tsc_check @@ -232,6 +237,21 @@ async def verify_task_is_complete(base_args: BaseArgs, **_kwargs): ) formatting_applied.append(f"- {snap_path}: Snapshot updated") + # Run PHPUnit tests on PHP test files + php_test_files = [ + f["filename"] + for f in pr_files + if f["filename"].endswith(PHP_TEST_FILE_EXTENSIONS) and f["status"] != "removed" + ] + phpunit_result = await run_phpunit_test( + base_args=base_args, + test_file_paths=php_test_files, + ) + if phpunit_result.errors: + for err in phpunit_result.errors: + remaining_errors.append(f"- phpunit: {err}") + error_files.update(phpunit_result.error_files) + if remaining_errors: error_msg = "\n".join(remaining_errors) logger.warning("Remaining errors after fixes:\n%s", error_msg) diff --git a/services/agents/verify_task_is_ready.py b/services/agents/verify_task_is_ready.py index 1a59fb1ff..ab0c50253 100644 --- a/services/agents/verify_task_is_ready.py +++ b/services/agents/verify_task_is_ready.py @@ -1,10 +1,12 @@ from dataclasses import dataclass, field +from constants.files import PHP_TEST_FILE_EXTENSIONS from services.eslint.run_eslint_fix import run_eslint_fix from services.github.commits.replace_remote_file import replace_remote_file_content from services.github.files.get_raw_content import get_raw_content from services.github.types.github_types import BaseArgs from services.jest.run_jest_test import run_jest_test +from services.phpunit.run_phpunit_test import run_phpunit_test from services.prettier.run_prettier_fix import run_prettier_fix from services.tsc.run_tsc_check import run_tsc_check from utils.error.handle_exceptions import handle_exceptions @@ -29,9 +31,10 @@ async def verify_task_is_ready( *, base_args: BaseArgs, file_paths: list[str], - run_tsc: bool = False, - run_jest: bool = False, -) -> VerifyTaskIsReadyResult: + run_tsc: bool, + run_jest: bool, + run_phpunit: bool, +): owner = base_args.get("owner", "") repo = base_args.get("repo", "") token = base_args.get("token", "") @@ -128,6 +131,18 @@ async def verify_task_is_ready( errors.append(f"- {jest_result.runner_name}: {err}") files_with_errors.update(jest_result.error_files) + # Run PHPUnit tests if requested + if run_phpunit: + php_test_files = [f for f in file_paths if f.endswith(PHP_TEST_FILE_EXTENSIONS)] + phpunit_result = await run_phpunit_test( + base_args=base_args, + test_file_paths=php_test_files, + ) + if phpunit_result.errors: + for err in phpunit_result.errors: + errors.append(f"- phpunit: {err}") + files_with_errors.update(phpunit_result.error_files) + if errors: return VerifyTaskIsReadyResult( success=False, diff --git a/services/efs/clone_and_install.py b/services/efs/clone_and_install.py index e37832692..d1269cacd 100644 --- a/services/efs/clone_and_install.py +++ b/services/efs/clone_and_install.py @@ -1,9 +1,12 @@ +from dataclasses import dataclass + from services.efs.get_efs_dir import get_efs_dir from services.git.get_clone_url import get_clone_url from services.git.git_clone_to_efs import git_clone_to_efs from services.github.branches.get_default_branch import get_default_branch from services.github.token.get_installation_token import get_installation_access_token from services.node.ensure_node_packages import ensure_node_packages +from services.php.ensure_php_packages import ensure_php_packages from services.supabase.installations.get_installation_by_owner import ( get_installation_by_owner, ) @@ -12,6 +15,15 @@ from utils.logging.logging_config import logger, set_owner_repo, set_trigger +@dataclass +class CloneAndInstallResult: + status: str + message: str + efs_dir: str + node_installed: bool + php_installed: bool + + @handle_exceptions(default_return_value=None, raise_on_error=False) async def clone_and_install(owner: str, repo: str): set_owner_repo(owner, repo) @@ -21,7 +33,13 @@ async def clone_and_install(owner: str, repo: str): installation = get_installation_by_owner(owner) if not installation: logger.warning("No installation found for %s", owner) - return {"status": "error", "message": f"No installation found for {owner}"} + return CloneAndInstallResult( + status="error", + message=f"No installation found for {owner}", + efs_dir="", + node_installed=False, + php_installed=False, + ) installation_id = installation["installation_id"] owner_id = installation["owner_id"] @@ -35,7 +53,13 @@ async def clone_and_install(owner: str, repo: str): branch, is_empty = get_default_branch(owner=owner, repo=repo, token=token) if is_empty: logger.warning("Repository %s/%s is empty", owner, repo) - return {"status": "error", "message": "Repository is empty"} + return CloneAndInstallResult( + status="error", + message="Repository is empty", + efs_dir="", + node_installed=False, + php_installed=False, + ) logger.info("Using default branch: %s", branch) efs_dir = get_efs_dir(owner, repo) @@ -45,7 +69,7 @@ async def clone_and_install(owner: str, repo: str): await git_clone_to_efs(efs_dir, clone_url, branch) logger.info("Installing node packages") - result = await ensure_node_packages( + node_result = await ensure_node_packages( owner=owner, owner_id=owner_id, repo=repo, @@ -53,6 +77,24 @@ async def clone_and_install(owner: str, repo: str): token=token, efs_dir=efs_dir, ) + logger.info("node: ready=%s", node_result) + + logger.info("Installing PHP packages") + php_result = await ensure_php_packages( + owner=owner, + owner_id=owner_id, + repo=repo, + branch=branch, + token=token, + efs_dir=efs_dir, + ) + logger.info("php: ready=%s", php_result) logger.info("Clone and install completed for %s/%s", owner, repo) - return {"status": "success", "efs_dir": efs_dir, "installed": result} + return CloneAndInstallResult( + status="success", + message="", + efs_dir=efs_dir, + node_installed=node_result, + php_installed=php_result, + ) diff --git a/services/efs/copy_repo_from_efs_to_tmp.py b/services/efs/copy_repo_from_efs_to_tmp.py index 0b7b5fe2c..f54843b48 100644 --- a/services/efs/copy_repo_from_efs_to_tmp.py +++ b/services/efs/copy_repo_from_efs_to_tmp.py @@ -4,9 +4,11 @@ from utils.error.handle_exceptions import handle_exceptions from utils.logging.logging_config import logger -# node_modules.tar.gz: extracted separately by extract_dependencies +# node_modules.tar.gz, vendor.tar.gz: extracted separately by extract_dependencies # index.lock: stale git lock from concurrent EFS operations -IGNORE_EFS_FILES = shutil.ignore_patterns("node_modules.tar.gz", "index.lock") +IGNORE_EFS_FILES = shutil.ignore_patterns( + "node_modules.tar.gz", "vendor.tar.gz", "index.lock" +) @handle_exceptions(default_return_value=None, raise_on_error=False) diff --git a/services/efs/extract_dependencies.py b/services/efs/extract_dependencies.py index 86678bb66..bc77d9514 100644 --- a/services/efs/extract_dependencies.py +++ b/services/efs/extract_dependencies.py @@ -4,34 +4,41 @@ from utils.error.handle_exceptions import handle_exceptions from utils.logging.logging_config import logger +# Dependency directories to extract from EFS tarballs +_DEPENDENCY_DIRS = ["node_modules", "vendor"] + @handle_exceptions(default_return_value=None, raise_on_error=False) def extract_dependencies(efs_dir: str, clone_dir: str): - # Extract cached dependencies from EFS tarball to clone_dir - # EFS stores node_modules.tar.gz (single file, fast to read) - # Lambda extracts to local /tmp (fast, ~15-30s for 150k files) - tarball_path = os.path.join(efs_dir, "node_modules.tar.gz") - target_path = os.path.join(clone_dir, "node_modules") - - if os.path.exists(target_path): - logger.info( - "Extract skip: node_modules already exists at %s", - target_path, - ) - return + """Extract cached dependencies from EFS tarball to clone_dir. + + EFS stores node_modules.tar.gz and vendor.tar.gz (single file, fast to read). + Lambda extracts to local /tmp (fast, ~15-30s for 150k files). + """ + for dep_dir in _DEPENDENCY_DIRS: + tarball_path = os.path.join(efs_dir, f"{dep_dir}.tar.gz") + target_path = os.path.join(clone_dir, dep_dir) + + if os.path.exists(target_path): + logger.info( + "Extract skip: %s already exists at %s", + dep_dir, + target_path, + ) + continue + + if not os.path.exists(tarball_path): + logger.info( + "Extract skip: no tarball at %s", + tarball_path, + ) + continue - if not os.path.exists(tarball_path): - logger.info( - "Extract skip: no tarball at %s (not a Node.js project or CodeBuild hasn't run)", - tarball_path, + # Extract tarball to clone_dir (-x=extract, -z=decompress gzip, -f=file, -C=target dir) + logger.info("Extracting %s from EFS tarball...", dep_dir) + subprocess.run( + ["tar", "-xzf", tarball_path, "-C", clone_dir], + check=True, + capture_output=True, ) - return - - # Extract tarball to clone_dir (-x=extract, -z=decompress gzip, -f=file, -C=target dir) - logger.info("Extracting dependencies from EFS tarball...") - subprocess.run( - ["tar", "-xzf", tarball_path, "-C", clone_dir], - check=True, - capture_output=True, - ) - logger.info("Extracted: %s -> %s", tarball_path, target_path) + logger.info("Extracted: %s -> %s", tarball_path, target_path) diff --git a/services/efs/test_clone_and_install.py b/services/efs/test_clone_and_install.py index 29e6d8ecb..3ae56a01b 100644 --- a/services/efs/test_clone_and_install.py +++ b/services/efs/test_clone_and_install.py @@ -39,13 +39,14 @@ async def test_clone_and_install_no_installation( result = await clone_and_install("nonexistent-owner", "test-repo") - assert result["status"] == "error" - assert "No installation found" in result["message"] + assert result.status == "error" + assert "No installation found" in result.message @pytest.mark.asyncio @patch("services.efs.clone_and_install.set_trigger") @patch("services.efs.clone_and_install.set_owner_repo") +@patch("services.efs.clone_and_install.ensure_php_packages", new_callable=AsyncMock) @patch("services.efs.clone_and_install.ensure_node_packages", new_callable=AsyncMock) @patch("services.efs.clone_and_install.git_clone_to_efs", new_callable=AsyncMock) @patch("services.efs.clone_and_install.get_clone_url") @@ -60,7 +61,8 @@ async def test_clone_and_install_with_target_branch( mock_get_efs_dir, mock_get_clone_url, mock_git_clone, - mock_install_packages, + mock_install_node, + mock_install_php, _mock_set_owner_repo, _mock_set_trigger, mock_installation, @@ -71,13 +73,15 @@ async def test_clone_and_install_with_target_branch( mock_get_repo.return_value = mock_repository mock_get_efs_dir.return_value = "/mnt/efs/test-owner/test-repo" mock_get_clone_url.return_value = "https://github.com/test-owner/test-repo.git" - mock_install_packages.return_value = True + mock_install_node.return_value = True + mock_install_php.return_value = False result = await clone_and_install("test-owner", "test-repo") - assert result["status"] == "success" - assert result["efs_dir"] == "/mnt/efs/test-owner/test-repo" - assert result["installed"] is True + assert result.status == "success" + assert result.efs_dir == "/mnt/efs/test-owner/test-repo" + assert result.node_installed is True + assert result.php_installed is False mock_git_clone.assert_called_once_with( "/mnt/efs/test-owner/test-repo", @@ -89,6 +93,7 @@ async def test_clone_and_install_with_target_branch( @pytest.mark.asyncio @patch("services.efs.clone_and_install.set_trigger") @patch("services.efs.clone_and_install.set_owner_repo") +@patch("services.efs.clone_and_install.ensure_php_packages", new_callable=AsyncMock) @patch("services.efs.clone_and_install.ensure_node_packages", new_callable=AsyncMock) @patch("services.efs.clone_and_install.git_clone_to_efs", new_callable=AsyncMock) @patch("services.efs.clone_and_install.get_clone_url") @@ -105,7 +110,8 @@ async def test_clone_and_install_with_default_branch( mock_get_efs_dir, mock_get_clone_url, mock_git_clone, - mock_install_packages, + mock_install_node, + mock_install_php, _mock_set_owner_repo, _mock_set_trigger, mock_installation, @@ -116,11 +122,12 @@ async def test_clone_and_install_with_default_branch( mock_get_default_branch.return_value = ("main", False) mock_get_efs_dir.return_value = "/mnt/efs/test-owner/test-repo" mock_get_clone_url.return_value = "https://github.com/test-owner/test-repo.git" - mock_install_packages.return_value = True + mock_install_node.return_value = True + mock_install_php.return_value = False result = await clone_and_install("test-owner", "test-repo") - assert result["status"] == "success" + assert result.status == "success" mock_get_default_branch.assert_called_once_with( owner="test-owner", repo="test-repo", token="test-token" ) @@ -154,5 +161,5 @@ async def test_clone_and_install_empty_repository( result = await clone_and_install("test-owner", "test-repo") - assert result["status"] == "error" - assert "Repository is empty" in result["message"] + assert result.status == "error" + assert "Repository is empty" in result.message diff --git a/services/efs/test_extract_dependencies.py b/services/efs/test_extract_dependencies.py index 33bf9da93..2351914c1 100644 --- a/services/efs/test_extract_dependencies.py +++ b/services/efs/test_extract_dependencies.py @@ -3,12 +3,14 @@ from services.efs.extract_dependencies import extract_dependencies -def test_extract_dependencies_extracts_tarball(): +def test_extract_dependencies_extracts_node_modules_tarball(): def exists_side_effect(path): if path == "/mnt/efs/repo/node_modules.tar.gz": return True if path == "/tmp/repo/node_modules": return False + if path == "/mnt/efs/repo/vendor.tar.gz": + return False return False with ( @@ -25,7 +27,52 @@ def exists_side_effect(path): ) -def test_extract_dependencies_skips_when_tarball_not_exists(): +def test_extract_dependencies_extracts_vendor_tarball(): + def exists_side_effect(path): + if path == "/mnt/efs/repo/node_modules.tar.gz": + return False + if path == "/mnt/efs/repo/vendor.tar.gz": + return True + if path == "/tmp/repo/vendor": + return False + return False + + with ( + patch("os.path.exists", side_effect=exists_side_effect), + patch("subprocess.run") as mock_run, + ): + mock_run.return_value = MagicMock(returncode=0) + extract_dependencies("/mnt/efs/repo", "/tmp/repo") + + mock_run.assert_called_once_with( + ["tar", "-xzf", "/mnt/efs/repo/vendor.tar.gz", "-C", "/tmp/repo"], + check=True, + capture_output=True, + ) + + +def test_extract_dependencies_extracts_both_tarballs(): + def exists_side_effect(path): + if path in ( + "/mnt/efs/repo/node_modules.tar.gz", + "/mnt/efs/repo/vendor.tar.gz", + ): + return True + if path in ("/tmp/repo/node_modules", "/tmp/repo/vendor"): + return False + return False + + with ( + patch("os.path.exists", side_effect=exists_side_effect), + patch("subprocess.run") as mock_run, + ): + mock_run.return_value = MagicMock(returncode=0) + extract_dependencies("/mnt/efs/repo", "/tmp/repo") + + assert mock_run.call_count == 2 + + +def test_extract_dependencies_skips_when_no_tarballs(): with ( patch("os.path.exists", return_value=False), patch("subprocess.run") as mock_run, @@ -35,7 +82,7 @@ def test_extract_dependencies_skips_when_tarball_not_exists(): mock_run.assert_not_called() -def test_extract_dependencies_skips_when_node_modules_already_exists(): +def test_extract_dependencies_skips_when_targets_already_exist(): with ( patch("os.path.exists", return_value=True), patch("subprocess.run") as mock_run, diff --git a/services/git/copy_config_templates.py b/services/git/copy_config_templates.py new file mode 100644 index 000000000..33a8b7206 --- /dev/null +++ b/services/git/copy_config_templates.py @@ -0,0 +1,39 @@ +import os +import shutil + +from utils.error.handle_exceptions import handle_exceptions +from utils.logging.logging_config import logger + +# Common config template suffixes in PHP, Node.js, and other ecosystems +# e.g., .env.example → .env, preference.inc.default → preference.inc +_TEMPLATE_SUFFIXES = (".default", ".example", ".sample") + + +@handle_exceptions(default_return_value=None, raise_on_error=False) +def copy_config_templates(clone_dir: str): + copied = 0 + for root, _dirs, files in os.walk(clone_dir): + # Skip dependency directories + basename = os.path.basename(root) + if basename in ("node_modules", "vendor", "vendor-bin", ".git"): + _dirs[:] = [] + continue + + for filename in files: + for suffix in _TEMPLATE_SUFFIXES: + if not filename.endswith(suffix): + continue + + template_path = os.path.join(root, filename) + target_path = template_path[: -len(suffix)] + if os.path.exists(target_path): + rel_path = os.path.relpath(target_path, clone_dir) + logger.info("Config template skip: %s already exists", rel_path) + continue + + shutil.copy2(template_path, target_path) + rel_path = os.path.relpath(target_path, clone_dir) + logger.info("Copied config template: %s", rel_path) + copied += 1 + if copied: + logger.info("Copied %d config template(s)", copied) diff --git a/services/git/prepare_repo_for_work.py b/services/git/prepare_repo_for_work.py index 4301806a3..4538cfb34 100644 --- a/services/git/prepare_repo_for_work.py +++ b/services/git/prepare_repo_for_work.py @@ -1,6 +1,7 @@ from services.efs.copy_repo_from_efs_to_tmp import copy_repo_from_efs_to_tmp -from services.efs.get_efs_dir import get_efs_dir from services.efs.extract_dependencies import extract_dependencies +from services.efs.get_efs_dir import get_efs_dir +from services.git.copy_config_templates import copy_config_templates from services.git.get_clone_url import get_clone_url from services.git.git_checkout import git_checkout from services.git.git_clone_to_efs import clone_tasks @@ -34,7 +35,10 @@ async def prepare_repo_for_work( # Step 3: Extract dependencies from EFS tarball to clone_dir extract_dependencies(efs_dir, clone_dir) - # Step 4: Fetch, checkout, and reset PR branch + # Step 4: Copy config templates (e.g., .env.example → .env, preference.inc.default → preference.inc) + copy_config_templates(clone_dir) + + # Step 5: Fetch, checkout, and reset PR branch fetch_ok = await git_fetch(clone_dir, clone_url, pr_branch) if fetch_ok: await git_checkout(clone_dir, pr_branch) diff --git a/services/git/test_copy_config_templates.py b/services/git/test_copy_config_templates.py new file mode 100644 index 000000000..479b936d3 --- /dev/null +++ b/services/git/test_copy_config_templates.py @@ -0,0 +1,93 @@ +# pylint: disable=redefined-outer-name +import os + +import pytest + +from config import UTF8 +from services.git.copy_config_templates import copy_config_templates + + +@pytest.fixture +def clone_dir(tmp_path): + return str(tmp_path) + + +def test_copies_default_file(clone_dir): + template = os.path.join(clone_dir, "conf", "preference.inc.default") + target = os.path.join(clone_dir, "conf", "preference.inc") + os.makedirs(os.path.dirname(template)) + with open(template, "w", encoding=UTF8) as f: + f.write("config content") + + copy_config_templates(clone_dir) + + assert os.path.exists(target) + with open(target, encoding=UTF8) as f: + assert f.read() == "config content" + + +def test_copies_example_file(clone_dir): + template = os.path.join(clone_dir, ".env.example") + target = os.path.join(clone_dir, ".env") + with open(template, "w", encoding=UTF8) as f: + f.write("SECRET=abc") + + copy_config_templates(clone_dir) + + assert os.path.exists(target) + + +def test_copies_sample_file(clone_dir): + template = os.path.join(clone_dir, "config.sample") + target = os.path.join(clone_dir, "config") + with open(template, "w", encoding=UTF8) as f: + f.write("sample") + + copy_config_templates(clone_dir) + + assert os.path.exists(target) + + +def test_skips_when_target_exists(clone_dir): + template = os.path.join(clone_dir, ".env.example") + target = os.path.join(clone_dir, ".env") + with open(template, "w", encoding=UTF8) as f: + f.write("new content") + with open(target, "w", encoding=UTF8) as f: + f.write("existing content") + + copy_config_templates(clone_dir) + + with open(target, encoding=UTF8) as f: + assert f.read() == "existing content" + + +def test_skips_vendor_directory(clone_dir): + vendor_template = os.path.join(clone_dir, "vendor", "pkg", "config.default") + vendor_target = os.path.join(clone_dir, "vendor", "pkg", "config") + os.makedirs(os.path.dirname(vendor_template)) + with open(vendor_template, "w", encoding=UTF8) as f: + f.write("vendor config") + + copy_config_templates(clone_dir) + + assert not os.path.exists(vendor_target) + + +def test_skips_node_modules_directory(clone_dir): + nm_template = os.path.join(clone_dir, "node_modules", "pkg", ".env.example") + nm_target = os.path.join(clone_dir, "node_modules", "pkg", ".env") + os.makedirs(os.path.dirname(nm_template)) + with open(nm_template, "w", encoding=UTF8) as f: + f.write("nm config") + + copy_config_templates(clone_dir) + + assert not os.path.exists(nm_target) + + +def test_no_templates_found(clone_dir): + with open(os.path.join(clone_dir, "README.md"), "w", encoding=UTF8) as f: + f.write("hello") + + copy_config_templates(clone_dir) diff --git a/services/node/ensure_node_packages.py b/services/node/ensure_node_packages.py index 27a3fabad..841a009ef 100644 --- a/services/node/ensure_node_packages.py +++ b/services/node/ensure_node_packages.py @@ -2,67 +2,14 @@ import os from config import UTF8 +from services.aws.run_install_via_codebuild import run_install_via_codebuild from services.git.git_clone_to_efs import clone_tasks from services.node.detect_package_manager import detect_package_manager from services.node.read_file_content import read_file_content -from services.aws.run_install_via_codebuild import run_install_via_codebuild from utils.error.handle_exceptions import handle_exceptions from utils.logging.logging_config import logger -def _file_matches(efs_path: str, source_content: str | None): - # Checks if EFS file matches source file from GitHub - if source_content: - if not os.path.exists(efs_path): - return False - with open(efs_path, "r", encoding=UTF8) as f: - return f.read() == source_content - return not os.path.exists(efs_path) - - -def _can_reuse_packages( - efs_dir: str, - package_json_content: str, - npmrc_content: str | None = None, - lock_file_name: str | None = None, - lock_file_content: str | None = None, -): - node_modules_path = os.path.join(efs_dir, "node_modules") - if not os.path.exists(node_modules_path): - return False - - # Check if .bin directory exists and has binaries (indicates complete install) - bin_path = os.path.join(node_modules_path, ".bin") - if not os.path.exists(bin_path): - logger.info( - "node: Incomplete install detected - .bin directory missing at %s", bin_path - ) - return False - - bin_contents = os.listdir(bin_path) - if not bin_contents: - logger.info( - "node: Incomplete install detected - .bin directory empty at %s", bin_path - ) - return False - - if not _file_matches(os.path.join(efs_dir, "package.json"), package_json_content): - return False - if not _file_matches(os.path.join(efs_dir, ".npmrc"), npmrc_content): - return False - if lock_file_name and not _file_matches( - os.path.join(efs_dir, lock_file_name), lock_file_content - ): - return False - - logger.info( - "node: Reusing existing packages on EFS at %s (%d binaries in .bin)", - efs_dir, - len(bin_contents), - ) - return True - - @handle_exceptions(default_return_value=False, raise_on_error=False) async def ensure_node_packages( owner: str, @@ -111,16 +58,6 @@ async def ensure_node_packages( os.makedirs(efs_dir, exist_ok=True) install_lock_path = os.path.join(efs_dir, ".install.lock") - package_json_path = os.path.join(efs_dir, "package.json") - - if _can_reuse_packages( - efs_dir, - package_json_content, - npmrc_content, - lock_file_name, - lock_file_content, - ): - return True with open(install_lock_path, "w", encoding=UTF8) as lock_file: try: @@ -128,13 +65,65 @@ async def ensure_node_packages( fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) logger.info("node: Lock acquired for %s", efs_dir) - if _can_reuse_packages( - efs_dir, - package_json_content, - npmrc_content, - lock_file_name, - lock_file_content, - ): + # Check if existing packages can be reused + node_modules_path = os.path.join(efs_dir, "node_modules") + bin_path = os.path.join(node_modules_path, ".bin") + package_json_path = os.path.join(efs_dir, "package.json") + can_reuse = os.path.exists(node_modules_path) + + if can_reuse: + if not os.path.exists(bin_path): + logger.info( + "node: Incomplete install - .bin directory missing at %s", + bin_path, + ) + can_reuse = False + else: + bin_contents = os.listdir(bin_path) + if not bin_contents: + logger.info( + "node: Incomplete install - .bin directory empty at %s", + bin_path, + ) + can_reuse = False + + if can_reuse: + # Verify package.json matches + if os.path.exists(package_json_path): + with open(package_json_path, "r", encoding=UTF8) as f: + can_reuse = f.read() == package_json_content + else: + can_reuse = False + + if can_reuse and npmrc_content: + npmrc_path = os.path.join(efs_dir, ".npmrc") + if os.path.exists(npmrc_path): + with open(npmrc_path, "r", encoding=UTF8) as f: + can_reuse = f.read() == npmrc_content + else: + can_reuse = False + + if can_reuse and npmrc_content is None: + # No .npmrc in repo, but if one exists on EFS, it's stale + npmrc_path = os.path.join(efs_dir, ".npmrc") + if os.path.exists(npmrc_path): + can_reuse = False + + if can_reuse and lock_file_name: + lock_path = os.path.join(efs_dir, lock_file_name) + if lock_file_content: + if os.path.exists(lock_path): + with open(lock_path, "r", encoding=UTF8) as f: + can_reuse = f.read() == lock_file_content + else: + can_reuse = False + + if can_reuse: + logger.info( + "node: Reusing existing packages on EFS at %s (%d binaries in .bin)", + efs_dir, + len(os.listdir(bin_path)), + ) return True # Copy to EFS because install runs in EFS to cache node_modules across Lambda invocations diff --git a/services/node/test_ensure_node_packages.py b/services/node/test_ensure_node_packages.py index 0ccabfac8..10a647df3 100644 --- a/services/node/test_ensure_node_packages.py +++ b/services/node/test_ensure_node_packages.py @@ -1,134 +1,37 @@ -from unittest.mock import MagicMock, mock_open, patch +# pylint: disable=redefined-outer-name +import os +from unittest.mock import patch import pytest -from services.node.ensure_node_packages import ( - _can_reuse_packages, - ensure_node_packages, -) +from config import UTF8 +from services.node.ensure_node_packages import ensure_node_packages -def test_can_reuse_packages_returns_false_when_no_node_modules(): - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.return_value = False +@pytest.fixture +def efs_dir(tmp_path): + return str(tmp_path) - result = _can_reuse_packages("/mnt/efs/owner/repo", "{}") - assert result is False - - -def test_can_reuse_packages_returns_true_when_content_matches(): - def exists_side_effect(path): - if "node_modules" in path or "package.json" in path or ".bin" in path: - return True - if ".npmrc" in path: - return False - return False - - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.side_effect = exists_side_effect - with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] - ): - with patch("builtins.open", mock_open(read_data='{"name": "test"}')): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - ) - - assert result is True - - -def test_can_reuse_packages_returns_false_when_content_differs(): - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.return_value = True - with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] - ): - with patch("builtins.open", mock_open(read_data='{"name": "old"}')): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "new"}', - ) - - assert result is False - - -def test_can_reuse_packages_returns_true_when_npmrc_matches(): - file_contents = { - "package.json": '{"name": "test"}', - ".npmrc": "//registry.npmjs.org/:_authToken=${NPM_TOKEN}", - } - - def open_side_effect(path, *_args, **_kwargs): - for filename, content in file_contents.items(): - if filename in path: - return mock_open(read_data=content)() - return mock_open(read_data="")() - - with patch("services.node.ensure_node_packages.os.path.exists", return_value=True): - with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] - ): - with patch("builtins.open", side_effect=open_side_effect): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - "//registry.npmjs.org/:_authToken=${NPM_TOKEN}", - ) - - assert result is True - - -def test_can_reuse_packages_returns_false_when_npmrc_differs(): - file_contents = { - "package.json": '{"name": "test"}', - ".npmrc": "//registry.npmjs.org/:_authToken=${NPM_TOKEN}", - } - - def open_side_effect(path, *_args, **_kwargs): - for filename, content in file_contents.items(): - if filename in path: - return mock_open(read_data=content)() - return mock_open(read_data="")() - - with patch("services.node.ensure_node_packages.os.path.exists", return_value=True): - with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] - ): - with patch("builtins.open", side_effect=open_side_effect): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - "//different-registry.npmjs.org/:_authToken=${NPM_TOKEN}", - ) - - assert result is False - - -def test_can_reuse_packages_returns_false_when_npmrc_missing_on_efs(): - def exists_side_effect(path): - if ".npmrc" in path: - return False - return True - - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.side_effect = exists_side_effect - with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] - ): - with patch("builtins.open", mock_open(read_data='{"name": "test"}')): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - "//registry.npmjs.org/:_authToken=${NPM_TOKEN}", - ) - - assert result is False +def _setup_node_modules( + efs_dir, package_json='{"name": "test"}', npmrc=None, binaries=None +): + """Set up a fake node_modules directory with .bin, package.json, and optional .npmrc.""" + nm = os.path.join(efs_dir, "node_modules") + bin_dir = os.path.join(nm, ".bin") + os.makedirs(bin_dir) + for b in binaries or ["eslint"]: + with open(os.path.join(bin_dir, b), "w", encoding=UTF8) as f: + f.write("") + with open(os.path.join(efs_dir, "package.json"), "w", encoding=UTF8) as f: + f.write(package_json) + if npmrc is not None: + with open(os.path.join(efs_dir, ".npmrc"), "w", encoding=UTF8) as f: + f.write(npmrc) @pytest.mark.asyncio -async def test_ensure_node_packages_returns_false_when_no_package_json(): +async def test_returns_false_when_no_package_json(): with patch("services.node.ensure_node_packages.read_file_content") as mock_get: mock_get.return_value = None @@ -145,235 +48,249 @@ async def test_ensure_node_packages_returns_false_when_no_package_json(): @pytest.mark.asyncio -async def test_ensure_node_packages_reuses_existing_packages(): - with patch("services.node.ensure_node_packages.read_file_content") as mock_get: - with patch("services.node.ensure_node_packages.os.makedirs"): - with patch( - "services.node.ensure_node_packages._can_reuse_packages" - ) as mock_reuse: - mock_get.return_value = '{"name": "test"}' - mock_reuse.return_value = True +async def test_reuses_when_content_matches(efs_dir): + _setup_node_modules(efs_dir) + with patch("services.node.ensure_node_packages.read_file_content") as mock_get: + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = ['{"name": "test"}', None] + with patch("services.node.ensure_node_packages.fcntl.flock"): result = await ensure_node_packages( owner="owner", owner_id=123, repo="repo", branch="main", token="token", - efs_dir="/mnt/efs/owner/repo", + efs_dir=efs_dir, ) assert result is True @pytest.mark.asyncio -async def test_ensure_node_packages_triggers_ssm_install(): - mock_lock_file = MagicMock() - mock_lock_file.fileno.return_value = 1 - +async def test_triggers_codebuild_when_no_node_modules(efs_dir): with patch("services.node.ensure_node_packages.read_file_content") as mock_get: - with patch("services.node.ensure_node_packages.os.makedirs"): - with patch( - "services.node.ensure_node_packages._can_reuse_packages", - return_value=False, - ): - with patch("builtins.open", return_value=mock_lock_file): - with patch("services.node.ensure_node_packages.fcntl.flock"): - with patch( - "services.node.ensure_node_packages.run_install_via_codebuild" - ) as mock_ssm: - mock_get.return_value = '{"name": "test"}' - mock_ssm.return_value = "cmd-123" - - result = await ensure_node_packages( - owner="owner", - owner_id=123, - repo="repo", - branch="main", - token="token", - efs_dir="/mnt/efs/owner/repo", - ) - - mock_ssm.assert_called_once() - # Returns False because packages are installing in background - assert result is False - - -def test_can_reuse_packages_returns_false_when_no_package_json_file(): - def exists_side_effect(path): - if "package.json" in path: - return False - return True - - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.side_effect = exists_side_effect with patch( - "services.node.ensure_node_packages.os.listdir", return_value=["eslint"] + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), ): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - ) - - assert result is False - + mock_get.side_effect = ['{"name": "test"}', None] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ) as mock_codebuild: + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + mock_codebuild.assert_called_once() + assert result is False -def test_can_reuse_packages_returns_false_when_bin_directory_missing(): - def exists_side_effect(path): - if ".bin" in path: - return False - return True - with patch("services.node.ensure_node_packages.os.path.exists") as mock_exists: - mock_exists.side_effect = exists_side_effect - - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - ) +@pytest.mark.asyncio +async def test_triggers_codebuild_when_content_differs(efs_dir): + _setup_node_modules(efs_dir, package_json='{"name": "old"}') - assert result is False + with patch("services.node.ensure_node_packages.read_file_content") as mock_get: + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = ['{"name": "new"}', None] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ) as mock_codebuild: + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + mock_codebuild.assert_called_once() + assert result is False -def test_can_reuse_packages_returns_false_when_bin_directory_empty(): - with patch("services.node.ensure_node_packages.os.path.exists", return_value=True): - with patch("services.node.ensure_node_packages.os.listdir", return_value=[]): - result = _can_reuse_packages( - "/mnt/efs/owner/repo", - '{"name": "test"}', - ) +@pytest.mark.asyncio +async def test_triggers_codebuild_when_bin_missing(efs_dir): + # node_modules exists but no .bin + os.makedirs(os.path.join(efs_dir, "node_modules")) + with open(os.path.join(efs_dir, "package.json"), "w", encoding=UTF8) as f: + f.write('{"name": "test"}') - assert result is False + with patch("services.node.ensure_node_packages.read_file_content") as mock_get: + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = ['{"name": "test"}', None] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ) as mock_codebuild: + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + mock_codebuild.assert_called_once() + assert result is False @pytest.mark.asyncio -async def test_ensure_node_packages_reuses_after_lock(): - mock_lock_file = MagicMock() - mock_lock_file.fileno.return_value = 1 +async def test_triggers_codebuild_when_bin_empty(efs_dir): + os.makedirs(os.path.join(efs_dir, "node_modules", ".bin")) + with open(os.path.join(efs_dir, "package.json"), "w", encoding=UTF8) as f: + f.write('{"name": "test"}') with patch("services.node.ensure_node_packages.read_file_content") as mock_get: - with patch("services.node.ensure_node_packages.os.makedirs"): - with patch( - "services.node.ensure_node_packages._can_reuse_packages" - ) as mock_reuse: - mock_reuse.side_effect = [False, True] - with patch("builtins.open", return_value=mock_lock_file): - with patch("services.node.ensure_node_packages.fcntl.flock"): - mock_get.return_value = '{"name": "test"}' - - result = await ensure_node_packages( - owner="owner", - owner_id=123, - repo="repo", - branch="main", - token="token", - efs_dir="/mnt/efs/owner/repo", - ) - - assert result is True - assert mock_reuse.call_count == 2 + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = ['{"name": "test"}', None] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ) as mock_codebuild: + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + mock_codebuild.assert_called_once() + assert result is False @pytest.mark.asyncio -async def test_ensure_node_packages_sanitizes_http_to_https_in_npmrc(): - mock_lock_file = MagicMock() - mock_lock_file.fileno.return_value = 1 +async def test_reuses_when_npmrc_matches(efs_dir): + _setup_node_modules(efs_dir, npmrc="//registry.npmjs.org/:_authToken=${NPM_TOKEN}") - written_content = {} + with patch("services.node.ensure_node_packages.read_file_content") as mock_get: + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = [ + '{"name": "test"}', + "//registry.npmjs.org/:_authToken=${NPM_TOKEN}", + ] + with patch("services.node.ensure_node_packages.fcntl.flock"): + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) - def mock_open_side_effect(path, *_args, **_kwargs): - mock_file = MagicMock() - mock_file.fileno.return_value = 1 - mock_file.__enter__ = MagicMock(return_value=mock_file) - mock_file.__exit__ = MagicMock(return_value=False) + assert result is True - def write_side_effect(content): - written_content[path] = content - mock_file.write = MagicMock(side_effect=write_side_effect) - return mock_file +@pytest.mark.asyncio +async def test_triggers_codebuild_when_npmrc_differs(efs_dir): + _setup_node_modules(efs_dir, npmrc="//registry.npmjs.org/:_authToken=${NPM_TOKEN}") with patch("services.node.ensure_node_packages.read_file_content") as mock_get: - with patch("services.node.ensure_node_packages.os.makedirs"): - with patch( - "services.node.ensure_node_packages._can_reuse_packages", - return_value=False, - ): - with patch("builtins.open", side_effect=mock_open_side_effect): - with patch("services.node.ensure_node_packages.fcntl.flock"): - with patch( - "services.node.ensure_node_packages.run_install_via_codebuild" - ): - mock_get.side_effect = [ - '{"name": "test"}', - "registry=http://registry.npmjs.org/", - None, - None, - ] - - await ensure_node_packages( - owner="owner", - owner_id=123, - repo="repo", - branch="main", - token="token", - efs_dir="/mnt/efs/owner/repo", - ) - - npmrc_path = "/mnt/efs/owner/repo/.npmrc" - assert npmrc_path in written_content - assert ( - written_content[npmrc_path] - == "registry=https://registry.npmjs.org/" - ) + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = [ + '{"name": "test"}', + "//different-registry.npmjs.org/:_authToken=${NPM_TOKEN}", + ] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ) as mock_codebuild: + result = await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + mock_codebuild.assert_called_once() + assert result is False @pytest.mark.asyncio -async def test_ensure_node_packages_preserves_https_in_npmrc(): - written_content = {} - - def mock_open_side_effect(path, *_args, **_kwargs): - mock_file = MagicMock() - mock_file.fileno.return_value = 1 - mock_file.__enter__ = MagicMock(return_value=mock_file) - mock_file.__exit__ = MagicMock(return_value=False) - - def write_side_effect(content): - written_content[path] = content +async def test_sanitizes_http_to_https_in_npmrc(efs_dir): + with patch("services.node.ensure_node_packages.read_file_content") as mock_get: + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = [ + '{"name": "test"}', + "registry=http://registry.npmjs.org/", + ] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ): + await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + npmrc_path = os.path.join(efs_dir, ".npmrc") + assert os.path.exists(npmrc_path) + with open(npmrc_path, encoding=UTF8) as f: + assert f.read() == "registry=https://registry.npmjs.org/" - mock_file.write = MagicMock(side_effect=write_side_effect) - return mock_file +@pytest.mark.asyncio +async def test_preserves_https_in_npmrc(efs_dir): with patch("services.node.ensure_node_packages.read_file_content") as mock_get: - with patch("services.node.ensure_node_packages.os.makedirs"): - with patch( - "services.node.ensure_node_packages._can_reuse_packages", - return_value=False, - ): - with patch("builtins.open", side_effect=mock_open_side_effect): - with patch("services.node.ensure_node_packages.fcntl.flock"): - with patch( - "services.node.ensure_node_packages.run_install_via_codebuild" - ): - mock_get.side_effect = [ - '{"name": "test"}', - "registry=https://registry.npmjs.org/", - None, - None, - ] - - await ensure_node_packages( - owner="owner", - owner_id=123, - repo="repo", - branch="main", - token="token", - efs_dir="/mnt/efs/owner/repo", - ) - - npmrc_path = "/mnt/efs/owner/repo/.npmrc" - assert npmrc_path in written_content - assert ( - written_content[npmrc_path] - == "registry=https://registry.npmjs.org/" - ) + with patch( + "services.node.ensure_node_packages.detect_package_manager", + return_value=("npm", None, None), + ): + mock_get.side_effect = [ + '{"name": "test"}', + "registry=https://registry.npmjs.org/", + ] + with patch("services.node.ensure_node_packages.fcntl.flock"): + with patch( + "services.node.ensure_node_packages.run_install_via_codebuild" + ): + await ensure_node_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir=efs_dir, + ) + + npmrc_path = os.path.join(efs_dir, ".npmrc") + with open(npmrc_path, encoding=UTF8) as f: + assert f.read() == "registry=https://registry.npmjs.org/" diff --git a/services/php/ensure_php_packages.py b/services/php/ensure_php_packages.py new file mode 100644 index 000000000..5c166493b --- /dev/null +++ b/services/php/ensure_php_packages.py @@ -0,0 +1,109 @@ +import fcntl +import os + +from config import UTF8 +from services.aws.run_install_via_codebuild import run_install_via_codebuild +from services.git.git_clone_to_efs import clone_tasks +from services.node.read_file_content import read_file_content +from utils.error.handle_exceptions import handle_exceptions +from utils.logging.logging_config import logger + + +@handle_exceptions(default_return_value=False, raise_on_error=False) +async def ensure_php_packages( + owner: str, + owner_id: int, + repo: str, + branch: str, + token: str, + efs_dir: str, +): + # Wait for clone to complete before installing + clone_task = clone_tasks.get(efs_dir) + if clone_task: + logger.info("php: Waiting for clone task: %s", efs_dir) + result = await clone_task + if result: + logger.info("php: Clone task completed: %s", efs_dir) + else: + logger.warning("php: Clone task failed: %s", efs_dir) + + composer_json_content = read_file_content( + "composer.json", + local_dir=efs_dir, + owner=owner, + repo=repo, + branch=branch, + token=token, + ) + if not composer_json_content: + logger.info("php: No composer.json found, skipping installation") + return False + + composer_lock_content = read_file_content( + "composer.lock", + local_dir=efs_dir, + owner=owner, + repo=repo, + branch=branch, + token=token, + ) + + os.makedirs(efs_dir, exist_ok=True) + + install_lock_path = os.path.join(efs_dir, ".install-php.lock") + + with open(install_lock_path, "w", encoding=UTF8) as lock_file: + try: + logger.info("php: Acquiring lock for %s", efs_dir) + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + logger.info("php: Lock acquired for %s", efs_dir) + + # Check if existing packages can be reused + vendor_path = os.path.join(efs_dir, "vendor") + autoload_path = os.path.join(vendor_path, "autoload.php") + composer_json_path = os.path.join(efs_dir, "composer.json") + can_reuse = os.path.exists(vendor_path) and os.path.exists(autoload_path) + + if can_reuse: + # Verify composer.json matches + if os.path.exists(composer_json_path): + with open(composer_json_path, "r", encoding=UTF8) as f: + can_reuse = f.read() == composer_json_content + else: + can_reuse = False + + if can_reuse and composer_lock_content: + lock_path = os.path.join(efs_dir, "composer.lock") + if os.path.exists(lock_path): + with open(lock_path, "r", encoding=UTF8) as f: + can_reuse = f.read() == composer_lock_content + else: + can_reuse = False + + if can_reuse: + logger.info("php: Reusing existing packages on EFS at %s", efs_dir) + return True + + if not os.path.exists(autoload_path) and os.path.exists(vendor_path): + logger.info( + "php: Incomplete install - vendor/autoload.php missing at %s", + autoload_path, + ) + + with open(composer_json_path, "w", encoding=UTF8) as f: + f.write(composer_json_content) + + if composer_lock_content: + lock_path = os.path.join(efs_dir, "composer.lock") + with open(lock_path, "w", encoding=UTF8) as f: + f.write(composer_lock_content) + logger.info("php: Wrote composer.lock to %s", lock_path) + + run_install_via_codebuild(efs_dir, owner_id, "composer") + logger.info("php: Triggered CodeBuild install for %s", efs_dir) + return False + + finally: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + logger.info("php: Lock released for %s", efs_dir) diff --git a/services/php/test_ensure_php_packages.py b/services/php/test_ensure_php_packages.py new file mode 100644 index 000000000..605ddf708 --- /dev/null +++ b/services/php/test_ensure_php_packages.py @@ -0,0 +1,176 @@ +# pylint: disable=redefined-outer-name +import os +from unittest.mock import patch + +import pytest + +from config import UTF8 +from services.php.ensure_php_packages import ensure_php_packages + + +@pytest.fixture +def efs_dir(tmp_path): + return str(tmp_path) + + +@pytest.mark.asyncio +async def test_returns_false_when_no_composer_json(): + with patch("services.php.ensure_php_packages.read_file_content") as mock_get: + mock_get.return_value = None + + result = await ensure_php_packages( + owner="owner", + owner_id=123, + repo="repo", + branch="main", + token="token", + efs_dir="/mnt/efs/owner/repo", + ) + + assert result is False + + +@pytest.mark.asyncio +async def test_reuses_when_vendor_and_content_match(efs_dir): + # Set up EFS with matching vendor, autoload, composer.json + vendor_path = os.path.join(efs_dir, "vendor") + os.makedirs(os.path.join(vendor_path, "autoload.php").rsplit("/", 1)[0]) + with open(os.path.join(vendor_path, "autoload.php"), "w", encoding=UTF8) as f: + f.write(" 0 + assert "tests/Unit/MyTest.php" in result.error_files + + +@pytest.mark.asyncio +@patch("services.phpunit.run_phpunit_test.subprocess.run") +@patch("services.phpunit.run_phpunit_test.os.path.exists") +async def test_run_phpunit_test_exit_code_nonzero_but_ok(mock_exists, mock_subprocess): + def exists_side_effect(path): + if path == "/tmp/clone/vendor/bin/phpunit": + return True + if path.endswith("phpunit.xml"): + return True + return False + + mock_exists.side_effect = exists_side_effect + mock_subprocess.return_value = MagicMock( + returncode=1, + stdout="OK (10 tests, 25 assertions)\nDeprecation notices...", + stderr="", + ) + + base_args = cast( + BaseArgs, {"owner": "test", "repo": "test", "clone_dir": "/tmp/clone"} + ) + result = await run_phpunit_test( + base_args=base_args, + test_file_paths=["tests/Unit/MyTest.php"], + ) + assert result.success is True + assert result.errors == [] + assert result.error_files == set() + + +@pytest.mark.asyncio +@patch("services.phpunit.run_phpunit_test.subprocess.run") +@patch("services.phpunit.run_phpunit_test.os.path.exists") +async def test_run_phpunit_test_multiple_failures(mock_exists, mock_subprocess): + def exists_side_effect(path): + if path == "/tmp/clone/vendor/bin/phpunit": + return True + if path.endswith("phpunit.xml"): + return True + return False + + mock_exists.side_effect = exists_side_effect + mock_subprocess.return_value = MagicMock( + returncode=1, + stdout="FAILURES!\nTests: 2, Assertions: 3, Failures: 2.", + stderr="", + ) + + base_args = cast( + BaseArgs, {"owner": "test", "repo": "test", "clone_dir": "/tmp/clone"} + ) + result = await run_phpunit_test( + base_args=base_args, + test_file_paths=["tests/Unit/ATest.php", "tests/Unit/BTest.php"], + ) + assert result.success is False + assert "tests/Unit/ATest.php" in result.error_files + assert "tests/Unit/BTest.php" in result.error_files + + +@pytest.mark.asyncio +@patch("services.phpunit.run_phpunit_test.subprocess.run") +@patch("services.phpunit.run_phpunit_test.os.path.exists") +async def test_run_phpunit_test_uses_bootstrap_when_no_config( + mock_exists, mock_subprocess +): + def exists_side_effect(path): + if path == "/tmp/clone/vendor/bin/phpunit": + return True + if path == "/tmp/clone/vendor/autoload.php": + return True + # No phpunit.xml or phpunit.xml.dist + return False + + mock_exists.side_effect = exists_side_effect + mock_subprocess.return_value = MagicMock( + returncode=0, stdout="OK (1 test)", stderr="" + ) + + base_args = cast( + BaseArgs, {"owner": "test", "repo": "test", "clone_dir": "/tmp/clone"} + ) + await run_phpunit_test( + base_args=base_args, + test_file_paths=["tests/Unit/MyTest.php"], + ) + + cmd = mock_subprocess.call_args[0][0] + assert "--bootstrap" in cmd + assert "/tmp/clone/vendor/autoload.php" in cmd + + +@pytest.mark.asyncio +@patch("services.phpunit.run_phpunit_test.subprocess.run") +@patch("services.phpunit.run_phpunit_test.os.path.exists") +async def test_run_phpunit_test_suppresses_deprecation_warnings( + mock_exists, mock_subprocess +): + def exists_side_effect(path): + if path == "/tmp/clone/vendor/bin/phpunit": + return True + if path.endswith("phpunit.xml"): + return True + return False + + mock_exists.side_effect = exists_side_effect + mock_subprocess.return_value = MagicMock( + returncode=0, stdout="OK (1 test)", stderr="" + ) + + base_args = cast( + BaseArgs, {"owner": "test", "repo": "test", "clone_dir": "/tmp/clone"} + ) + await run_phpunit_test( + base_args=base_args, + test_file_paths=["tests/Unit/MyTest.php"], + ) + + cmd = mock_subprocess.call_args[0][0] + assert "-d" in cmd + assert "error_reporting=E_ALL&~E_DEPRECATED" in cmd + + +@pytest.mark.asyncio +async def test_run_phpunit_test_filters_non_php_files(): + base_args = cast( + BaseArgs, {"owner": "test", "repo": "test", "clone_dir": "/tmp/clone"} + ) + result = await run_phpunit_test( + base_args=base_args, + test_file_paths=["src/index.test.ts", "README.md", "config.json"], + ) + assert result.success is True + assert result.errors == [] diff --git a/services/webhook/check_suite_handler.py b/services/webhook/check_suite_handler.py index 77b9b70ca..3d6bad088 100644 --- a/services/webhook/check_suite_handler.py +++ b/services/webhook/check_suite_handler.py @@ -23,6 +23,7 @@ from services.efs.get_efs_dir import get_efs_dir from services.node.ensure_node_packages import ensure_node_packages from services.node.set_npm_token_env import set_npm_token_env +from services.php.ensure_php_packages import ensure_php_packages from services.git.get_clone_dir import get_clone_dir from services.git.get_clone_url import get_clone_url from services.git.git_clone_to_efs import clone_tasks, git_clone_to_efs @@ -261,6 +262,11 @@ async def handle_check_suite( ) logger.info("node: ready=%s", node_ready) + php_ready = await ensure_php_packages( + owner_name, owner_id, repo_name, target_branch, token, efs_dir + ) + logger.info("php: ready=%s", php_ready) + # Check if permission comment or stumbled comment already exists if has_comment_with_text( owner=owner_name, @@ -354,7 +360,11 @@ async def handle_check_suite( f["filename"] for f in changed_files if f["status"] != "removed" ] validation_result = await verify_task_is_ready( - base_args=base_args, file_paths=files_to_validate, run_tsc=True, run_jest=True + base_args=base_args, + file_paths=files_to_validate, + run_tsc=True, + run_jest=True, + run_phpunit=True, ) base_args["baseline_tsc_errors"] = set(validation_result.tsc_errors) pre_existing_errors = "" diff --git a/services/webhook/new_pr_handler.py b/services/webhook/new_pr_handler.py index eb6332d80..48adebd3b 100644 --- a/services/webhook/new_pr_handler.py +++ b/services/webhook/new_pr_handler.py @@ -18,6 +18,7 @@ from services.efs.get_efs_dir import get_efs_dir from services.node.ensure_node_packages import ensure_node_packages from services.node.set_npm_token_env import set_npm_token_env +from services.php.ensure_php_packages import ensure_php_packages from services.git.get_clone_dir import get_clone_dir from services.git.get_clone_url import get_clone_url from services.git.git_clone_to_efs import clone_tasks, git_clone_to_efs @@ -222,6 +223,11 @@ async def handle_new_pr( ) logger.info("node: ready=%s", node_ready) + php_ready = await ensure_php_packages( + owner_name, owner_id, repo_name, base_branch, token, efs_dir + ) + logger.info("php: ready=%s", php_ready) + # Create a usage record usage_id = create_user_request( user_id=sender_id, @@ -476,6 +482,7 @@ async def handle_new_pr( file_paths=files_to_validate, run_tsc=True, run_jest=False, + run_phpunit=False, ) base_args["baseline_tsc_errors"] = set(validation_result.tsc_errors) pre_existing_errors = "" diff --git a/services/webhook/review_run_handler.py b/services/webhook/review_run_handler.py index 7549faa82..c8d9f1507 100644 --- a/services/webhook/review_run_handler.py +++ b/services/webhook/review_run_handler.py @@ -21,6 +21,7 @@ from services.efs.get_efs_dir import get_efs_dir from services.node.ensure_node_packages import ensure_node_packages from services.node.set_npm_token_env import set_npm_token_env +from services.php.ensure_php_packages import ensure_php_packages from services.git.get_clone_dir import get_clone_dir from services.git.get_clone_url import get_clone_url from services.git.git_clone_to_efs import clone_tasks, git_clone_to_efs @@ -199,6 +200,11 @@ async def handle_review_run( ) logger.info("node: ready=%s", node_ready) + php_ready = await ensure_php_packages( + owner_name, owner_id, repo_name, base_branch, token, efs_dir + ) + logger.info("php: ready=%s", php_ready) + # Create a usage record usage_id = create_user_request( user_id=sender_id, @@ -245,7 +251,11 @@ async def handle_review_run( # Validate files for syntax issues before editing files_to_validate = [f["filename"] for f in pr_files if f["status"] != "removed"] validation_result = await verify_task_is_ready( - base_args=base_args, file_paths=files_to_validate, run_tsc=True, run_jest=True + base_args=base_args, + file_paths=files_to_validate, + run_tsc=True, + run_jest=True, + run_phpunit=True, ) base_args["baseline_tsc_errors"] = set(validation_result.tsc_errors) pre_existing_errors = ""