chore: split unit tests from the main CI workflow and report combined coverage to Coveralls #425
Conversation
… coverage to Coveralls
|
Warning Rate limit exceeded@martin-georgiev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces the monolithic matrix CI job with a single PHP 8.4 code-quality job, adds dedicated Unit Tests and Coverage workflows, and moves coverage uploads to conditional per-workflow and a finalizer that waits for both unit and integration runs before uploading aggregated coverage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant CI as ci.yml (Code Quality)
Dev->>GH: Push / PR
GH->>CI: Trigger code-quality
CI->>CI: composer validate
CI->>CI: Cache composer & composer install
CI->>CI: Check code style
CI->>CI: Security checks
sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Gate as should-run
participant UT as unit-tests (matrix)
participant Cov as Coveralls
Dev->>GH: Open/Update PR
GH->>Gate: Run should-run
alt Author is Release Please bot
Gate-->>GH: run=false (skip unit-tests)
else Other author
Gate-->>GH: run=true
GH->>UT: Execute matrix (PHP/ORM/Lexer combos)
UT->>UT: Setup PHP + install deps per-matrix
UT->>UT: Static analysis
UT->>UT: Run unit tests
opt coverage-enabled rows
UT->>Cov: Upload unit coverage (flag "Unit")
end
end
sequenceDiagram
autonumber
actor GH as GitHub Actions
participant IT as integration-tests.yml
participant UT as unit-tests.yml
participant Final as coverage.yml
participant Cov as Coveralls
GH->>IT: Run Integration Tests
GH->>UT: Run Unit Tests
note right of Final: Triggered on workflow_run completions
Final->>Final: Query workflow runs for head SHA
alt Both Integration & Unit completed successfully
Final->>Cov: Upload combined coverage (parallel finish)
else Missing/failed runs
Final-->>Final: Exit (no upload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 1
🧹 Nitpick comments (6)
.github/workflows/integration-tests.yml (2)
45-61: Move POSTGRES_INITDB_ARGS to env; avoid mixing env config betweenenv:andoptions -e.For clarity and portability of the service definition, prefer setting environment variables under the
env:block instead of passing them viaoptions: -e ....Apply this diff:
env: POSTGRES_PASSWORD: postgres POSTGRES_USER: postgres POSTGRES_DB: postgres_doctrine_test + POSTGRES_INITDB_ARGS: "--data-checksums" ... - options: >- + options: >- --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 --mount type=tmpfs,destination=/var/lib/postgresql/data - -e POSTGRES_INITDB_ARGS="--data-checksums"
109-117: Optional: Publish integration clover.xml as an artifact for debugging.When Coveralls is flaky, having the coverage file as a short-retention artifact speeds up troubleshooting.
If you want, I can propose a pinned
actions/upload-artifactstep gated by the same condition and with a small retention window..github/workflows/coveralls-finish.yml (1)
15-23: Guard against premature finalization when one workflow finishes before the other.This workflow runs on completion of either "Unit Tests" or "Integrations". If the faster workflow finishes first,
parallel-finished: truemay signal Coveralls to finalize before the other uploads land (Coveralls often tolerates repeated finalizations, but behavior can vary).Consider limiting finalization to a single workflow (e.g., only when
workflow_run.name == 'Integrations') or add a short polling step that waits for the other workflow (same head SHA) to complete before calling the finalize step. I can provide agh api-based guard if you want to make this deterministic..github/workflows/unit-tests.yml (2)
103-106: actionlint warning: matrix key forcontinue-on-erroris undefined.
continue-on-error: ${{ matrix.continue-on-error || false }}references a non-existent matrix property; actionlint flags this.Apply this diff to introduce an explicit matrix boolean and use it:
matrix: php: ['8.1', '8.2', '8.3', '8.4'] doctrine-lexer: ['2.1', '3.0', 'latest'] doctrine-orm: ['2.14', '2.18', '3.0', 'latest'] + allow-static-analysis-failures: [false] include: - php: '8.1' doctrine-orm: '2.14' doctrine-lexer: '1.2' - php: '8.4' # Run coverage report only based on the latest dependencies doctrine-lexer: 'latest' doctrine-orm: 'latest' calculate-code-coverage: true ... - - name: Run static analysis + - name: Run static analysis run: composer run-static-analysis - continue-on-error: ${{ matrix.continue-on-error || false }} + continue-on-error: ${{ matrix.allow-static-analysis-failures }}If you don't actually need a soft-fail in any combination, you can simply drop the
continue-on-errorline.
79-90: Avoid runningcomposer updatetwice; consolidate “latest” resolution to a single step.Both the Lexer and ORM install steps use
composer updatein their respective “latest” branches, which can cause redundant resolution and longer runtimes.Apply this diff to perform updates only once in the ORM step:
- name: Install Doctrine Lexer dependency run: | if [ "${{ matrix.doctrine-lexer }}" = "1.2" ]; then composer require doctrine/lexer "~1.2" --dev --prefer-dist --no-interaction --no-progress elif [ "${{ matrix.doctrine-lexer }}" = "2.1" ]; then composer require doctrine/lexer "~2.1" --dev --prefer-dist --no-interaction --no-progress elif [ "${{ matrix.doctrine-lexer }}" = "3.0" ]; then composer require doctrine/lexer "~3.0" --dev --prefer-dist --no-interaction --no-progress else - composer update --prefer-dist --no-interaction --no-progress + echo "Using latest lexer as part of the overall dependency resolution" fi- name: Install Doctrine ORM dependency run: | if [ "${{ matrix.doctrine-orm }}" = "2.14" ]; then composer require doctrine/orm "~2.14" --prefer-dist --no-interaction --no-progress --with-all-dependencies elif [ "${{ matrix.doctrine-orm }}" = "2.18" ]; then composer require doctrine/orm "~2.18" --prefer-dist --no-interaction --no-progress --with-all-dependencies elif [ "${{ matrix.doctrine-orm }}" = "3.0" ]; then composer require doctrine/orm "~3.0" --prefer-dist --no-interaction --no-progress --with-all-dependencies else composer update --prefer-dist --no-interaction --no-progress fi.github/workflows/ci.yml (1)
42-48: Optional: enable Xdebug only if code-quality tasks require it.You’ve already removed coverage here (good). If future tools need extensions, keep them minimal to reduce setup time.
No change required now.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks).github/workflows/coveralls-finish.yml(1 hunks).github/workflows/integration-tests.yml(1 hunks).github/workflows/unit-tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/integration-tests.yml
119-119: property "calculate-code-coverage" is not defined in object type {php: number; postgres: number}
(expression)
.github/workflows/unit-tests.yml
105-105: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + 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.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: sloth
🔇 Additional comments (3)
.github/workflows/coveralls-finish.yml (1)
9-11: Double-check required permissions for Coveralls check updates.Most setups work with
contents: read, but some repos needchecks: writefor the action to update check runs cleanly.If you see missing/failed Coveralls check annotations, add:
permissions: - contents: read + contents: read + checks: write.github/workflows/unit-tests.yml (1)
110-118: Confirm PHPUnit outputs coverage to the expected path on the coverage-enabled job.The step points to
./var/logs/test-coverage/unit/clover.xml. Ensure the phpunit config for the PHP 8.4 “latest” combo actually writes there; otherwise Coveralls will silently skip.If needed, I can help align the phpunit XML and composer script so the path is consistent across unit and integration runs.
.github/workflows/ci.yml (1)
33-69: LGTM: clean split of code-quality from tests; pinned actions and sensible steps.The consolidation to a fixed PHP 8.4 “code-quality” job with validate/cache/install + style + audit looks good and aligns with the new dedicated test workflows.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/coverage.yml (1)
22-24: Nice: concise check over required workflow names.The
.everyover['Integration Tests', 'Unit Tests']is clear and maintainable. This also addresses a prior suggestion on an earlier commit.
🧹 Nitpick comments (2)
.github/workflows/coverage.yml (2)
15-24: Also verify both workflows succeeded, not just completed.Right now the script only checks presence of completed runs by name. Include
run.conclusion === 'success'to avoid finalizing when one of the workflows failed or was cancelled. The diff in my earlier comment incorporates this change; no separate action needed if you apply it.
12-13: Supply-chain hardening: consider pinning actions/github-script to a commit SHA.You’ve pinned Coveralls to a commit SHA (good). For consistency and supply-chain safety, pin
actions/github-script@v7to its commit SHA as well. If you prefer to keep the major tag, that’s acceptable for a “Chill” review; treat this as a nice-to-have.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/coverage.yml(1 hunks).github/workflows/integration-tests.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-tests.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sloth
🔇 Additional comments (1)
.github/workflows/coverage.yml (1)
31-35: Coveralls finalization usage looks correct.Using
parallel-finished: truealigns with the per-jobparallel: trueuploads in the Unit/Integration workflows. With the gating fix and concurrency guard in place, this should produce stable combined coverage reports.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/coverage.yml (2)
34-38: LGTM: correct gating over both "Integration Tests" and "Unit Tests".The
every+somecheck withconclusion === 'success'is precise and matches the intent.
43-47: Good fix: final Coveralls upload is gated by the script output.This addresses the earlier issue where
process.exit(0)didn’t stop subsequent steps, preventing premature finalization.
🧹 Nitpick comments (2)
.github/workflows/coverage.yml (2)
11-11: Avoid false negatives: drop thegithub.shaequality check for workflow_run.For workflow_run-triggered workflows,
github.shamay point to the latest commit on the default branch rather than the triggering run’s head SHA. This can cause the job to be skipped when the branch has advanced. Concurrency scoping byhead_shaalready prevents duplicate finalizers.- if: ${{ github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.head_sha == github.sha }} + if: ${{ github.event.workflow_run.conclusion == 'success' }}
3-7: Optional: restrict triggers to target branches to cut noise.If the upstream workflows run on multiple branches, consider limiting the finalizer to your active branches (e.g., main) so it doesn’t trigger for non-target branches/tags.
on: workflow_run: workflows: ["Integration Tests", "Unit Tests"] types: [completed] + branches: + - mainAdjust the branch list to match where you expect coverage to be aggregated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/coverage.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/coverage.yml
16-16: unknown permission scope "workflows". all available permission scopes are "actions", "attestations", "checks", "contents", "deployments", "discussions", "id-token", "issues", "packages", "pages", "pull-requests", "repository-projects", "security-events", "statuses"
(permissions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + 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 latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + 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.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: sloth
🔇 Additional comments (2)
.github/workflows/coverage.yml (2)
18-20: Concurrency keyed by head_sha is the right deduping strategy.This ensures only one finalizer runs per commit even if both upstream workflows finish around the same time.
5-5: All workflow names match exactly
I ran a quick grep over .github/workflows/*.yml and confirmed the defined workflow names:
- Integration Tests (in .github/workflows/integration-tests.yml)
- Unit Tests (in .github/workflows/unit-tests.yml)
These exactly match the entries in coverage.yml, so no changes are needed.
Summary by CodeRabbit
Tests
Chores