ci: use PCOV instead of Xdebug #359
Conversation
WalkthroughThe changes update the project's continuous integration (CI) workflow and Composer configuration related to PHPUnit test execution and code coverage. The CI workflow switches the PHP code coverage driver from "xdebug" to "pcov" and modifies the test step to run PHPUnit tests individually per test file instead of all at once. The Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
composer.json (1)
74-74: Approve removal of explicit XDEBUG_MODE env var. This change aligns the PHPUnit script with the new PCOV-based coverage in CI. Consider updating your developer documentation (e.g., README or CONTRIBUTING) with instructions to enable PCOV locally (for example,pecl install pcov && echo "pcov.enabled=1" >> php.ini).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)composer.json(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
65-65: Switching the coverage driver to PCOV. Leveraging PCOV can significantly speed up coverage reporting. Please verify that the pinnedshivammathur/setup-phpaction commit supports PCOV for all PHP versions in your matrix and that the PCOV extension is actually loaded during CI runs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ci/phpunit/run-split-tests.sh (2)
38-38: Split declaration and assignment forcoverage_fileto avoid masking return values
Shellcheck SC2155 suggests declaring and assigning separately when using command substitution.Proposed diff:
- local coverage_file="split/$(basename "$test_file" .php).xml" + local coverage_file + coverage_file="split/$(basename "$test_file" .php).xml"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
9-13: Consider makingTEST_DIRSconfigurable via args or env
Currently test directories are hard‑coded for a single namespace. Allowing an environment variable or CLI option (e.g.,--dirs) would make the script reusable across different projects or directory layouts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)ci/phpunit/run-split-tests.sh(1 hunks)composer.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ci/phpunit/run-split-tests.sh
[error] 17-17: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
116-116: shellcheck reported issue in this script: SC2044:warning:1:13: For loops over find output are fragile. Use find -exec or a while read loop
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (3)
ci/phpunit/run-split-tests.sh (2)
54-56: Good use offind -print0withread -r -d ''
This pattern safely handles file names with spaces or special characters, avoiding the pitfalls of simplefor file in $(find ...)loops.
62-73: Coverage merging placeholder clearly documents future enhancement
Noting that a tool likephpcovis required and guiding users toward installation is valuable. When ready, integrating an automated merge command will complete this workflow..github/workflows/ci.yml (1)
63-67:❓ Verification inconclusive
Switched PHPUnit coverage driver to
pcov
Using PCOV offers faster, more efficient coverage reporting compared to Xdebug, especially for per-file runs. Please verify that theshivammathur/setup-phpaction correctly enables PCOV and that yourci/phpunit/config.xmlhas no lingering Xdebug references.To confirm, run:
🏁 Script executed:
#!/bin/bash grep -R "xdebug" ci/phpunit/config.xml || echo "No xdebug references found"Length of output: 101
Confirm PCOV Coverage Setup
I’ve verified thatci/phpunit/config.xmlcontains no lingering Xdebug references. Next, please ensure that theshivammathur/setup-phpaction’scoverage: pcovoption actually installs and enables the PCOV extension in your CI environment (e.g. check the job logs for “pcov” or add a step to runphp -m).
| if [[ "$@" == *"--coverage-clover"* ]]; then | ||
| COVERAGE_ARGS="--coverage-clover=./var/logs/test-coverage/clover.xml" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace wildcard comparison on "$@" with explicit loop to parse flags
Shellcheck SC2199 warns about using [[ "$@" == *"--coverage-clover"* ]]. Instead of relying on wildcard matching over the entire argument string, iterate through the arguments to detect the coverage flag safely.
Proposed diff:
- if [[ "$@" == *"--coverage-clover"* ]]; then
- COVERAGE_ARGS="--coverage-clover=./var/logs/test-coverage/clover.xml"
- fi
+ COVERAGE_ARGS=""
+ for arg in "$@"; do
+ if [ "$arg" = "--coverage-clover" ]; then
+ COVERAGE_ARGS="--coverage-clover=./var/logs/test-coverage/clover.xml"
+ break
+ fi
+ done🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 17-17: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
| run: | | ||
| for file in $(find tests -name "*Test.php"); do | ||
| echo "Running tests from $file" | ||
| php ./bin/phpunit --configuration=./ci/phpunit/config.xml "$file" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace inline test loop with the centralized split-tests script
Duplicating loop logic here risks drift over time. Invoke the shared run-split-tests.sh script to ensure consistency between local and CI environments, and automatically handle coverage flags.
Proposed diff:
- - name: Run test suite
- run: |
- for file in $(find tests -name "*Test.php"); do
- echo "Running tests from $file"
- php ./bin/phpunit --configuration=./ci/phpunit/config.xml "$file"
- done
+ - name: Run split test suite (no coverage)
+ if: matrix.calculate-code-coverage != 'true'
+ run: bash ci/phpunit/run-split-tests.sh
+
+ - name: Run split test suite (with coverage)
+ if: matrix.calculate-code-coverage == 'true'
+ run: bash ci/phpunit/run-split-tests.sh --coverage-clover📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| for file in $(find tests -name "*Test.php"); do | |
| echo "Running tests from $file" | |
| php ./bin/phpunit --configuration=./ci/phpunit/config.xml "$file" | |
| done | |
| - name: Run split test suite (no coverage) | |
| if: matrix.calculate-code-coverage != 'true' | |
| run: bash ci/phpunit/run-split-tests.sh | |
| - name: Run split test suite (with coverage) | |
| if: matrix.calculate-code-coverage == 'true' | |
| run: bash ci/phpunit/run-split-tests.sh --coverage-clover |
🧰 Tools
🪛 actionlint (1.7.4)
116-116: shellcheck reported issue in this script: SC2044:warning:1:13: For loops over find output are fragile. Use find -exec or a while read loop
(shellcheck)
Summary by CodeRabbit