Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughAdded a dedicated gRPC integration workflow; removed gRPC extension caching and related steps from other workflows; composer-install now ignores ext-grpc; PHPUnit runs in other workflows exclude the grpc group; FirestoreTest annotated with grpc group and grpc extension requirement. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Runner as Runner
participant PHP as PHP Setup
participant Composer as Composer
participant PHPUnit as PHPUnit
participant Codecov as Codecov
Dev->>GH: push / pull_request / workflow_dispatch
GH->>Runner: start grpc-tests job
Runner->>PHP: setup PHP 8.5 (install/cache ext-grpc)
Runner->>Composer: install deps (--ignore-platform-req=ext-grpc)
Composer-->>Runner: deps installed
Runner->>PHPUnit: run tests (group: grpc, produce JUnit/coverage)
PHPUnit-->>Runner: results & coverage
Runner->>Codecov: upload reports
Codecov-->>GH: report uploaded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## 8.x #1063 +/- ##
=========================================
Coverage 87.37% 87.37%
Complexity 1469 1469
=========================================
Files 145 145
Lines 4141 4141
=========================================
Hits 3618 3618
Misses 523 523 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/static-analysis.yml:
- Around line 52-54: The workflow step that uses "ramsey/composer-install@v3"
includes an undefined matrix variable in the composer-options input; remove the
"${{ matrix.composer-options }}" reference (leaving composer-options as
"--ignore-platform-req=ext-grpc") or alternatively add a composer-options entry
to the job matrix so the variable is defined; update the step that declares the
composer-options input accordingly to avoid referencing an undefined matrix
variable.
In @.github/workflows/tests.yml:
- Around line 55-58: The workflow references an undefined matrix variable
matrix.composer-options in the ramsey/composer-install step; update the
composer-options input for the action (uses: "ramsey/composer-install@v3") to
remove ${{ matrix.composer-options }} and pass only the intended flag (e.g.
"--ignore-platform-req=ext-grpc"), or alternatively add matrix.composer-options
to the matrix if you intended it to be configurable.
🧹 Nitpick comments (1)
.github/workflows/grpc-tests.yml (1)
78-86: Consider specifying--testsuitefor clarity.The PHPUnit command uses
--group grpcbut doesn't specify a testsuite. Other workflows likeintegration-tests.ymluse--testsuite=integration. If the gRPC tests are only in the integration testsuite, adding--testsuite=integrationwould make the intent explicit and potentially speed up test discovery.Optional: Add testsuite specification
- run: vendor/bin/phpunit --group grpc --coverage-clover=coverage.xml --log-junit=test-report.xml --testdox + run: vendor/bin/phpunit --testsuite=integration --group grpc --coverage-clover=coverage.xml --log-junit=test-report.xml --testdox
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/grpc-tests.yml:
- Around line 34-35: The workflow currently sets continue-on-error: true for the
gRPC tests, which will hide test failures; either remove or set
continue-on-error to false on that job to ensure failures fail the PR, or if
this is intentional add an inline YAML comment explaining why gRPC test failures
are non-blocking and the criteria/timeline for removing that exception
(reference the continue-on-error: true key and the surrounding if condition used
to gate PR runs).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/grpc-tests.yml:
- Around line 7-20: The workflow path filters under the pull_request and push
jobs in .github/workflows/grpc-tests.yml are missing composer.lock, so updates
to dependencies won't trigger the gRPC tests; update the path arrays used in the
pull_request and push sections (the paths entries shown in the diff) to include
'composer.lock' alongside 'src/**', 'tests/**',
'.github/workflows/grpc-tests.yml', 'composer.json', and 'phpstan.neon.dist'.
In @.github/workflows/integration-tests.yml:
- Around line 4-6: The workflow currently triggers on pull_request which doesn't
provide secrets for forked PRs; update the integration-tests job to guard
against forked PRs by either switching the trigger back to pull_request_target
or adding a runtime condition that ensures the PR comes from the same repository
(use the context expression github.event.pull_request.head.repo.full_name ==
github.repository) before running steps that read secrets, or alternatively add
a guard that checks for required secrets presence and skips the job when they're
absent; change the job-level "if" expression (or the top-level trigger)
accordingly so jobs referencing secrets (the integration tests) are never
executed on secret-less forked PRs.
| paths: | ||
| - 'src/**' | ||
| - 'tests/**' | ||
| - '.github/workflows/grpc-tests.yml' | ||
| - 'composer.json' | ||
| - 'phpstan.neon.dist' | ||
| push: | ||
| branches: ['8.x'] | ||
| paths: | ||
| - 'src/**' | ||
| - 'tests/**' | ||
| - '.github/workflows/grpc-tests.yml' | ||
| - 'composer.json' | ||
| - 'phpstan.neon.dist' |
There was a problem hiding this comment.
Add composer.lock to the workflow path filters.
Line 7-20: dependency updates via composer.lock alone won’t trigger these gRPC tests, so regressions can slip through. Add it to both pull_request and push path filters.
🔧 Suggested diff
paths:
- 'src/**'
- 'tests/**'
- '.github/workflows/grpc-tests.yml'
- 'composer.json'
+ - 'composer.lock'
- 'phpstan.neon.dist'
...
paths:
- 'src/**'
- 'tests/**'
- '.github/workflows/grpc-tests.yml'
- 'composer.json'
+ - 'composer.lock'
- 'phpstan.neon.dist'📝 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.
| paths: | |
| - 'src/**' | |
| - 'tests/**' | |
| - '.github/workflows/grpc-tests.yml' | |
| - 'composer.json' | |
| - 'phpstan.neon.dist' | |
| push: | |
| branches: ['8.x'] | |
| paths: | |
| - 'src/**' | |
| - 'tests/**' | |
| - '.github/workflows/grpc-tests.yml' | |
| - 'composer.json' | |
| - 'phpstan.neon.dist' | |
| paths: | |
| - 'src/**' | |
| - 'tests/**' | |
| - '.github/workflows/grpc-tests.yml' | |
| - 'composer.json' | |
| - 'composer.lock' | |
| - 'phpstan.neon.dist' | |
| push: | |
| branches: ['8.x'] | |
| paths: | |
| - 'src/**' | |
| - 'tests/**' | |
| - '.github/workflows/grpc-tests.yml' | |
| - 'composer.json' | |
| - 'composer.lock' | |
| - 'phpstan.neon.dist' |
🤖 Prompt for AI Agents
In @.github/workflows/grpc-tests.yml around lines 7 - 20, The workflow path
filters under the pull_request and push jobs in .github/workflows/grpc-tests.yml
are missing composer.lock, so updates to dependencies won't trigger the gRPC
tests; update the path arrays used in the pull_request and push sections (the
paths entries shown in the diff) to include 'composer.lock' alongside 'src/**',
'tests/**', '.github/workflows/grpc-tests.yml', 'composer.json', and
'phpstan.neon.dist'.
| # TEMPORARY: Use pull_request to run integration tests in this PR. Revert to pull_request_target before merging. | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
Guard integration tests on forked PRs to avoid secret-less failures.
pull_request runs do not receive secrets from forks, but this job requires several secrets (Line 63–67). That will hard-fail external PRs. Consider gating the job to same-repo PRs (or skip when secrets are absent).
✅ Suggested guard to skip forked PRs
jobs:
integration-tests:
+ if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
name: Integration Tests🤖 Prompt for AI Agents
In @.github/workflows/integration-tests.yml around lines 4 - 6, The workflow
currently triggers on pull_request which doesn't provide secrets for forked PRs;
update the integration-tests job to guard against forked PRs by either switching
the trigger back to pull_request_target or adding a runtime condition that
ensures the PR comes from the same repository (use the context expression
github.event.pull_request.head.repo.full_name == github.repository) before
running steps that read secrets, or alternatively add a guard that checks for
required secrets presence and skips the job when they're absent; change the
job-level "if" expression (or the top-level trigger) accordingly so jobs
referencing secrets (the integration tests) are never executed on secret-less
forked PRs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/grpc-tests.yml:
- Around line 4-5: Update the GitHub Actions workflow trigger: replace the
temporary pull_request trigger key with pull_request_target (i.e., change
"pull_request:" to "pull_request_target:") and remove the TODO comment so the
workflow uses the secure target trigger for fork PRs; verify any related
conditional or permissions blocks still align with the new trigger.
🧹 Nitpick comments (1)
.github/workflows/grpc-tests.yml (1)
94-102: Consider gating test execution on gRPC availability for clearer CI feedback.Tests run unconditionally, relying on PHPUnit's
RequiresPhpExtensionto skip when gRPC is absent. While functionally correct, this can produce misleading "passed" results with empty coverage. Consider adding an explicit condition or log message.♻️ Optional: Add condition to skip tests when gRPC is unavailable
- name: Run gRPC Tests + if: steps.grpc.outputs.grpc_present == 'true' env: GOOGLE_APPLICATION_CREDENTIALS: ${{secrets.GOOGLE_APPLICATION_CREDENTIALS}}Alternatively, add explicit logging when tests are skipped:
- name: Skip gRPC Tests (extension unavailable) if: steps.grpc.outputs.grpc_present != 'true' run: echo "::warning::Skipping gRPC tests - extension not available"
| # TEMPORARY: Use pull_request to run gRPC tests in this PR. Revert to pull_request_target before merging. | ||
| pull_request: |
There was a problem hiding this comment.
Address the TODO before merging: revert pull_request to pull_request_target.
Line 4 contains a reminder to revert the trigger type. Ensure this is completed before merging to maintain proper security boundaries for fork PRs accessing secrets.
🤖 Prompt for AI Agents
In @.github/workflows/grpc-tests.yml around lines 4 - 5, Update the GitHub
Actions workflow trigger: replace the temporary pull_request trigger key with
pull_request_target (i.e., change "pull_request:" to "pull_request_target:") and
remove the TODO comment so the workflow uses the secure target trigger for fork
PRs; verify any related conditional or permissions blocks still align with the
new trigger.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/grpc-tests.yml:
- Around line 40-69: The workflow uses env.php-version and env.extensions in the
setup-php step but never defines them; update the job-level env or the setup-php
inputs so those variables are explicit (e.g., set env.php-version to 8.5 and
env.extensions to grpc) or inline the values directly into the setup-php action;
modify the env block or the setup-php step (referencing setup-php,
env.php-version, env.extensions, and grpc) so the job deterministically installs
the intended PHP version and grpc extension.
- Around line 32-33: The setup-php step is referencing undefined workflow
environment variables `${{ env.php-version }}` and `${{ env.extensions }}`;
either add those keys to the workflow-level env (e.g., set `php-version: '8.5'`
and `extensions: 'grpc'`) or change the setup-php step to use explicit values
(for example replace `${{ env.php-version }}` with `'8.5'` and `${{
env.extensions }}` with `'grpc'`) so they match the cache-extensions
configuration and avoid undefined variable errors.
1 participant
grpcandRequiresExtension('grpc')ext-grpcplatform reqgrpcflagSummary by CodeRabbit