Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 1, 2025

Problem

Pre-push hooks were failing when running tests outside DDEV environment because the PostgreSQL database container (db) was not accessible from the host. This caused 34 test failures during git push even when all tests passed via ddev exec.

Error Example:

SQLSTATE[08006] [7] could not translate host name "db" to address

Solution

Enhanced scripts/preflight.sh to auto-detect DDEV and run PHP checks (Pint, PHPStan, tests) inside the container when available.

Changes

  • ✅ Add DDEV detection: command -v ddev && ddev describe
  • ✅ Run Pint via ddev exec when DDEV is available
  • ✅ Run PHPStan via ddev exec when DDEV is available
  • ✅ Run tests via ddev exec when DDEV is available
  • ✅ Fallback to host execution with warning when DDEV not detected
  • ✅ User-friendly output: "✓ DDEV detected - using containerized environment"

Benefits

  • 🎯 Fixes pre-push hook failures - Database-dependent tests now work during git push
  • 🔒 Consistent environment - Same PHP/PostgreSQL versions in hooks as in development
  • No breaking changes - Falls back to host execution for non-DDEV setups
  • 📝 Clear messaging - Users see when DDEV is used vs. host fallback

Testing

# Run preflight manually
./scripts/preflight.sh

# Test pre-push hook
git push origin fix/pre-commit-ddev-tests

# Output:
✓ DDEV detected - using containerized environment for PHP checks
Tests:    39 passed (89 assertions)
Preflight OK · Changed lines: 65

Related

LOC

  • Lines changed: 65
  • Impact: scripts/preflight.sh (pre-push hook logic)

- Add DDEV detection in preflight.sh to run PHP checks in container
- Run Pint, PHPStan, and tests via 'ddev exec' when DDEV is available
- Fallback to host execution with warning when DDEV not detected
- Fixes pre-push hook failures due to missing database connection
- Ensures consistent environment between local dev and CI

Resolves pre-commit outside DDEV issue from PR #55
Copilot AI review requested due to automatic review settings November 1, 2025 19:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the preflight script to support DDEV containerized environments for PHP checks. The script now automatically detects DDEV and routes PHP-related commands (composer, Pint, PHPStan, and tests) through the DDEV container when available, ensuring consistent execution environments.

Key Changes:

  • Added automatic DDEV detection with fallback to host execution
  • Wrapped all PHP tooling commands (Pint, PHPStan, tests) with ddev exec when DDEV is available
  • Added user-facing messages to indicate which environment is being used

- Add comment explaining why composer install is skipped in DDEV path
  (vendor/ is bind-mounted, dependencies managed by DDEV setup)
- Move warning message to only display after test failure
- Add helpful tip about using DDEV for database-dependent tests
- Capture and propagate test exit codes properly
- Addresses both Copilot review comments from PR #57
@kevalyq
Copy link
Contributor Author

kevalyq commented Nov 1, 2025

Addressed Copilot review feedback in commit d38b050:

Comment 1 - composer install in DDEV path:

  • Added comment explaining that composer install is intentionally skipped in DDEV path
  • Rationale: vendor/ directory is bind-mounted from host to container, dependencies are already managed by DDEV setup (ddev composer install during project initialization)
  • No need to run composer in pre-push hook when using DDEV

Comment 2 - Warning message noise:

  • Moved warning to only display after test failure when DDEV not available
  • Added helpful tip: "Tip: Use DDEV for tests requiring PostgreSQL: ddev exec ./vendor/bin/pest"
  • Capture and properly propagate test exit codes using TEST_EXIT variable
  • Cleaner output for users with properly configured host environments

All CI checks passing ✅

@kevalyq kevalyq requested a review from Copilot November 1, 2025 19:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

- Extract run_pint(), run_phpstan(), run_tests() helper functions
- Accept command prefix parameter (empty or 'ddev exec')
- Reduce 70+ lines of duplicated code to reusable functions
- Keep --test flag for Pint (validation-only for pre-push hooks)
- Clarify comment: Pre-push hooks should validate, not auto-fix code

Addresses Copilot review comments:
- ID 2483869455: Extract Pint/PHPStan logic into functions
- ID 2483869457: Extract test execution logic into functions
- ID 2483869460, 2483869463: Clarify Pint --test usage
@kevalyq kevalyq requested a review from Copilot November 1, 2025 19:28
@SecPal SecPal deleted a comment from Copilot AI Nov 1, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

- Change Pint to use --dirty flag instead of --test for auto-fixing
- Update comment: Pint now auto-fixes code style (aligned with project)
- Fix tip message to use 'php artisan test' instead of pest

Addresses final Copilot comments:
- ID 2483869460, 2483869463, 2483880462: Remove --test, use --dirty
- ID 2483880470: Suggest artisan test (Laravel convention)

All 8 review comments now addressed.
@kevalyq kevalyq merged commit 4fb0bfe into main Nov 1, 2025
12 checks passed
@kevalyq kevalyq deleted the fix/pre-commit-ddev-tests branch November 1, 2025 19:38
kevalyq added a commit that referenced this pull request Nov 1, 2025
- Add comprehensive pre-push review checklist to DEVELOPMENT.md
- Document Copilot review comment resolution workflow
- Add RefreshDatabase to SetTenantMiddlewareTest for proper test isolation
- Prevents multi-iteration PRs like #57 (4 commits, 8 review comments)

Fixes identified in internal PR review analysis.
kevalyq added a commit that referenced this pull request Nov 1, 2025
- Add comprehensive pre-push review checklist to DEVELOPMENT.md
- Document Copilot review comment resolution workflow
- Add RefreshDatabase to SetTenantMiddlewareTest for proper test isolation

Prevents multi-iteration PRs like #57 (4 commits, 8 review comments).
Addresses issues identified in internal review analysis.
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