chore(ci): report coverage after tests only #37467
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughCentralizes CI coverage handling: adds COVERAGE_DIR/COVERAGE_FILE_NAME/COVERAGE_REPORTER, consolidates per-shard uploads into a single "Store coverage" step, adds a "Merge ui coverage files" step, updates docker-compose coverage mounting, adjusts nyc reporter, and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Runner
participant E2E as E2E Jobs (api/ui)
participant Merge as Merge UI Coverage
participant Store as Store Coverage (artifact)
participant Report as report-coverage Job
participant Codecov
Note over E2E: Each test shard writes .nyc_output into COVERAGE_DIR
Runner->>E2E: run tests → write .nyc_output to COVERAGE_DIR
E2E->>Merge: run nyc merge for UI shards → produce COVERAGE_FILE_NAME per shard
Merge->>Store: place coverage-{type}-{shard} under artifact path
Store->>Runner: upload artifact coverage-{type}-{shard}
Runner->>Report: download coverage-* artifacts
Report->>Report: nyc combine/report → generate LCOV for API and UI
Report->>Codecov: upload LCOV (flag: e2e-api)
Report->>Codecov: upload LCOV (flag: e2e)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
🪛 actionlint (1.7.8).github/workflows/ci.yml623-623: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) 633-633: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) ⏰ 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). (11)
🔇 Additional comments (7)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37467 +/- ##
===========================================
+ Coverage 68.10% 68.99% +0.88%
===========================================
Files 3364 3357 -7
Lines 115755 114236 -1519
Branches 20910 20558 -352
===========================================
- Hits 78838 78819 -19
+ Misses 34220 33329 -891
+ Partials 2697 2088 -609
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
rodrigok
force-pushed
the
chore/ci-improvements
branch
from
919c824 to
dd64bc9
Compare
rodrigok
force-pushed
the
chore/ci-improvements-2
branch
2 times, most recently
from
bdf7389 to
d3d6a00
Compare
rodrigok
changed the title
![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 (1)
apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js (1)
10-10: Consider documenting the optional COVERAGE_FILE_NAME variable.The
fileNamevariable is read from the environment but isn't validated or documented. Consider adding a comment explaining that this is optional and what happens when it's not set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci-test-e2e.yml(5 hunks).github/workflows/ci.yml(1 hunks)apps/meteor/package.json(1 hunks)apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js(2 hunks)docker-compose-ci.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/package.json.github/workflows/ci-test-e2e.yml
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/package.json
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/package.json.github/workflows/ci-test-e2e.yml
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/package.json
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
623-623: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
633-633: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (10)
apps/meteor/package.json (1)
38-38: LGTM! Optimized reporter for CI.The change from
lcovtolcovonlyis appropriate for CI environments. Thelcovonlyreporter generates only thelcov.infofile needed for coverage reporting tools, skipping the HTML report generation, which improves efficiency.docker-compose-ci.yml (2)
4-4: LGTM! Centralized coverage directory configuration.The use of
${COVERAGE_DIR:-/tmp/coverage}with a default value provides flexibility for CI configuration while maintaining a sensible fallback.
26-26: LGTM! Coverage file name now configurable.Adding
COVERAGE_FILE_NAMEas an environment variable enables per-test-type or per-shard coverage file naming, which aligns with the centralized coverage approach in this PR.apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js (1)
49-50: LGTM! Enhanced coverage file naming.The addition of customizable file naming via
COVERAGE_FILE_NAMEand the debug logging will help with troubleshooting coverage generation. Thefileoption is supported by istanbul-reports for the reporters used in this codebase..github/workflows/ci-test-e2e.yml (5)
81-83: LGTM! Centralized coverage configuration.The introduction of
COVERAGE_DIR,COVERAGE_FILE_NAME, andCOVERAGE_REPORTERenvironment variables successfully centralizes coverage handling across the workflow, making it easier to maintain and configure.
91-91: LGTM! Improved job name clarity.The conditional coverage suffix in the job name makes it immediately clear which jobs are collecting coverage data.
163-166: LGTM! Enhanced debugging and directory setup.The addition of
set -o xtraceaids in debugging, and always creating/chmod-ing the coverage directory ensures consistency regardless of whether coverage is enabled.
243-248: LGTM! Essential coverage merging for UI tests.The
nyc mergestep correctly consolidates multiple coverage files from.nyc_output(generated during Playwright test execution) into a single coverage file per shard, which is necessary for proper coverage aggregation.
266-271: LGTM! Consolidated coverage artifact upload.The unified coverage upload step with a consistent naming pattern (
coverage-${type}-${shard}) simplifies artifact management and aligns with the centralized coverage approach..github/workflows/ci.yml (1)
608-613: Verify the nyc report command arguments.The
--temp-dirpaths are correct. Coverage artifacts are uploaded from ci-test-e2e.yml with namescoverage-${{ inputs.type }}-${{ matrix.shard }}containing path/tmp/coverage, whereCOVERAGE_DIR: '/tmp/coverage/${{ inputs.type }}'structures data by type (api/ui). When downloaded with patterncoverage-*to/tmp/coverageusingmerge-multiple: true, the result is:
/tmp/coverage/api/(merged from all coverage-api-* shards)/tmp/coverage/ui/(merged from all coverage-ui-* shards)The nyc commands at lines 612-613 correctly reference these paths.
| report-coverage: | ||
| name: 📊 Report Coverage | ||
| runs-on: ubuntu-24.04 | ||
| needs: [release-versions, test-api-ee, test-ui-ee, test-ui-ee-watcher] | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v5 | ||
|
|
||
| - name: Use Node.js | ||
| uses: actions/setup-node@v6.0.0 | ||
| with: | ||
| node-version: ${{ needs.release-versions.outputs.node-version }} | ||
|
|
||
| - name: Restore coverage folder | ||
| uses: actions/download-artifact@v6 | ||
| with: | ||
| pattern: coverage-* | ||
| path: /tmp/coverage | ||
| merge-multiple: true | ||
|
|
||
| - name: Generate lcov report | ||
| run: | | ||
| set -o xtrace | ||
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/api --temp-dir=/tmp/coverage/api | ||
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/ui --temp-dir=/tmp/coverage/ui | ||
| - name: Store coverage-reports | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: reports-coverage | ||
| path: /tmp/coverage_report | ||
| include-hidden-files: true | ||
|
|
||
| - name: Report API coverage | ||
| uses: codecov/codecov-action@v3 | ||
| with: | ||
| fail_ci_if_error: true | ||
| files: /tmp/coverage_report/api/lcov.info | ||
| working-directory: . | ||
| flags: e2e-api | ||
| verbose: true | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
|
|
||
| - name: Report UI coverage | ||
| uses: codecov/codecov-action@v3 | ||
| with: | ||
| fail_ci_if_error: true | ||
| files: /tmp/coverage_report/ui/lcov.info | ||
| working-directory: . | ||
| flags: e2e | ||
| verbose: true | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
Excellent coverage aggregation workflow, but update Codecov action version.
The new coverage reporting job successfully centralizes coverage handling by downloading artifacts, merging them with nyc, and uploading to Codecov. However, the static analysis correctly identifies that codecov/codecov-action@v3 is deprecated.
Update to the latest version to ensure compatibility with current GitHub Actions runners:
- - name: Report API coverage
- uses: codecov/codecov-action@v3
+ - name: Report API coverage
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/api/lcov.info
working-directory: .
flags: e2e-api
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}
- - name: Report UI coverage
- uses: codecov/codecov-action@v3
+ - name: Report UI coverage
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/ui/lcov.info
working-directory: .
flags: e2e
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}Note: You may need to verify the API compatibility between v3 and v5, as some parameters might have changed.
📝 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.
| report-coverage: | |
| name: 📊 Report Coverage | |
| runs-on: ubuntu-24.04 | |
| needs: [release-versions, test-api-ee, test-ui-ee, test-ui-ee-watcher] | |
| steps: | |
| - uses: actions/checkout@v5 | |
| - name: Use Node.js | |
| uses: actions/setup-node@v6.0.0 | |
| with: | |
| node-version: ${{ needs.release-versions.outputs.node-version }} | |
| - name: Restore coverage folder | |
| uses: actions/download-artifact@v6 | |
| with: | |
| pattern: coverage-* | |
| path: /tmp/coverage | |
| merge-multiple: true | |
| - name: Generate lcov report | |
| run: | | |
| set -o xtrace | |
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/api --temp-dir=/tmp/coverage/api | |
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/ui --temp-dir=/tmp/coverage/ui | |
| - name: Store coverage-reports | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: reports-coverage | |
| path: /tmp/coverage_report | |
| include-hidden-files: true | |
| - name: Report API coverage | |
| uses: codecov/codecov-action@v3 | |
| with: | |
| fail_ci_if_error: true | |
| files: /tmp/coverage_report/api/lcov.info | |
| working-directory: . | |
| flags: e2e-api | |
| verbose: true | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| - name: Report UI coverage | |
| uses: codecov/codecov-action@v3 | |
| with: | |
| fail_ci_if_error: true | |
| files: /tmp/coverage_report/ui/lcov.info | |
| working-directory: . | |
| flags: e2e | |
| verbose: true | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| report-coverage: | |
| name: 📊 Report Coverage | |
| runs-on: ubuntu-24.04 | |
| needs: [release-versions, test-api-ee, test-ui-ee, test-ui-ee-watcher] | |
| steps: | |
| - uses: actions/checkout@v5 | |
| - name: Use Node.js | |
| uses: actions/setup-node@v6.0.0 | |
| with: | |
| node-version: ${{ needs.release-versions.outputs.node-version }} | |
| - name: Restore coverage folder | |
| uses: actions/download-artifact@v6 | |
| with: | |
| pattern: coverage-* | |
| path: /tmp/coverage | |
| merge-multiple: true | |
| - name: Generate lcov report | |
| run: | | |
| set -o xtrace | |
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/api --temp-dir=/tmp/coverage/api | |
| npx nyc report --reporter=lcovonly --report-dir=/tmp/coverage_report/ui --temp-dir=/tmp/coverage/ui | |
| - name: Store coverage-reports | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: reports-coverage | |
| path: /tmp/coverage_report | |
| include-hidden-files: true | |
| - name: Report API coverage | |
| uses: codecov/codecov-action@v5 | |
| with: | |
| fail_ci_if_error: true | |
| files: /tmp/coverage_report/api/lcov.info | |
| working-directory: . | |
| flags: e2e-api | |
| verbose: true | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| - name: Report UI coverage | |
| uses: codecov/codecov-action@v5 | |
| with: | |
| fail_ci_if_error: true | |
| files: /tmp/coverage_report/ui/lcov.info | |
| working-directory: . | |
| flags: e2e | |
| verbose: true | |
| token: ${{ secrets.CODECOV_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.8)
623-623: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
633-633: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml lines 588-640: the workflow uses the deprecated
codecov/codecov-action@v3; update the action reference to the current stable
major (e.g., codecov/codecov-action@v5) and adjust input names to match the new
API (verify token handling, files path, flags, fail_ci_if_error or equivalent,
working-directory and verbose flags) — replace the version tag, run a quick dry
CI to confirm no input name changes break the job, and update any parameter
names or behavior according to the v5 action docs.
rodrigok
force-pushed
the
chore/ci-improvements-2
branch
from
d3d6a00 to
bbdcfb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
633-685: Excellent centralized coverage reporting job.The new
report-coveragejob successfully consolidates coverage handling by:
- Downloading all coverage artifacts (
coverage-*)- Generating LCOV reports separately for API and UI coverage
- Uploading to Codecov with appropriate flags (
e2e-apiande2e)This centralization aligns perfectly with the PR objective of reporting coverage after tests.
However, as previously noted, the
codecov/codecov-action@v3is deprecated (lines 668, 678).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci-test-e2e.yml(5 hunks).github/workflows/ci.yml(3 hunks)apps/meteor/package.json(1 hunks)apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js(2 hunks)docker-compose-ci.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/package.json
- docker-compose-ci.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
.github/workflows/ci-test-e2e.yml
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
.github/workflows/ci-test-e2e.yml
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
668-668: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
678-678: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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: CodeQL-Build
🔇 Additional comments (6)
.github/workflows/ci-test-e2e.yml (5)
81-83: LGTM: Centralized coverage environment variables.The introduction of
COVERAGE_DIR,COVERAGE_FILE_NAME, andCOVERAGE_REPORTERsuccessfully centralizes coverage handling. The conditional logic forCOVERAGE_REPORTERensures coverage is only generated wheninputs.coverage == matrix.mongodb-version.
162-166: LGTM: Unconditional coverage directory preparation.Making the coverage directory preparation unconditional simplifies the workflow logic. The
chmod 777ensures Docker containers can write coverage files regardless of user permissions.
243-248: LGTM: UI coverage merging consolidates shard-specific coverage.The new step merges per-test coverage files from
.nyc_outputinto a single shard-specific JSON file. This aligns with the centralized coverage handling introduced in this PR.
266-271: LGTM: Consolidated coverage storage simplifies artifact management.Replacing multiple per-type coverage uploads with a single consolidated step improves maintainability. The artifact naming scheme
coverage-${type}-${shard}ensures proper identification downstream.
91-91: LGTM: Job name enhancement improves visibility.Appending the coverage indicator to the job name makes it easier to identify coverage runs in the GitHub Actions UI.
apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js (1)
10-10: Based on my verification, I can now generate the final response:Ensure
fileNamehas a default value or is validated when reporter is specified.The
create()function acceptsPartial<ReportOptions[T]>options, whereFileOptionsdefinesfile: string. Although the type signature suggestsfileis required, thePartialwrapper makes it optional at runtime. IfCOVERAGE_REPORTERis set butCOVERAGE_FILE_NAMEis not provided,fileNamewill beundefined, and passing{ file: undefined }toreports.create()may cause unexpected behavior or silent failures in report generation.Add a default value or validate that
fileNameis set when a reporter is configured:const dir = process.env.COVERAGE_DIR; -const fileName = process.env.COVERAGE_FILE_NAME; +const fileName = process.env.COVERAGE_FILE_NAME || 'coverage.json'; const reporter = process.env.COVERAGE_REPORTER || 'lcov';Alternatively, validate before use:
if (reporter && !fileName) { throw new Error('COVERAGE_FILE_NAME must be set when COVERAGE_REPORTER is specified'); }
| - name: Image ${{ matrix.service[1] || '"skipped"' }} | ||
| uses: ./.github/actions/build-docker | ||
| if: matrix.service[1] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' | ||
| env: | ||
| DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} | ||
| with: | ||
| CR_USER: ${{ secrets.CR_USER }} | ||
| CR_PAT: ${{ secrets.CR_PAT }} | ||
| deno-version: ${{ needs.release-versions.outputs.deno-version }} | ||
| arch: ${{ matrix.arch }} | ||
| service: ${{ matrix.service[1] }} | ||
| type: ${{ matrix.type }} | ||
| setup-docker: false | ||
|
|
||
| - name: Image ${{ matrix.service[2] || '"skipped"' }} | ||
| uses: ./.github/actions/build-docker | ||
| if: matrix.service[2] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' | ||
| env: | ||
| DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} | ||
| with: | ||
| CR_USER: ${{ secrets.CR_USER }} | ||
| CR_PAT: ${{ secrets.CR_PAT }} | ||
| deno-version: ${{ needs.release-versions.outputs.deno-version }} | ||
| arch: ${{ matrix.arch }} | ||
| service: ${{ matrix.service[2] }} | ||
| type: ${{ matrix.type }} | ||
| setup-docker: false | ||
|
|
||
| - name: Image ${{ matrix.service[3] || '"skipped"' }} | ||
| uses: ./.github/actions/build-docker | ||
| if: matrix.service[3] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' | ||
| env: | ||
| DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} | ||
| with: | ||
| CR_USER: ${{ secrets.CR_USER }} | ||
| CR_PAT: ${{ secrets.CR_PAT }} | ||
| deno-version: ${{ needs.release-versions.outputs.deno-version }} | ||
| arch: ${{ matrix.arch }} | ||
| service: ${{ matrix.service[3] }} | ||
| type: ${{ matrix.type }} | ||
| setup-docker: false |
There was a problem hiding this comment.
Remove duplicate build steps for matrix.service[1-3].
The workflow now contains duplicate build steps:
- Lines 328-368: New steps for
matrix.service[1],[2], and[3] - Lines 370-413: Existing steps for the same services
This duplication will cause each service to be built twice per matrix configuration, wasting CI resources and potentially causing race conditions or artifact conflicts.
Apply this diff to remove the duplicate steps:
- - name: Image ${{ matrix.service[1] || '"skipped"' }}
- uses: ./.github/actions/build-docker
- if: matrix.service[1] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]'
- env:
- DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }}
- with:
- CR_USER: ${{ secrets.CR_USER }}
- CR_PAT: ${{ secrets.CR_PAT }}
- deno-version: ${{ needs.release-versions.outputs.deno-version }}
- arch: ${{ matrix.arch }}
- service: ${{ matrix.service[1] }}
- type: ${{ matrix.type }}
- setup-docker: false
-
- - name: Image ${{ matrix.service[2] || '"skipped"' }}
- uses: ./.github/actions/build-docker
- if: matrix.service[2] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]'
- env:
- DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }}
- with:
- CR_USER: ${{ secrets.CR_USER }}
- CR_PAT: ${{ secrets.CR_PAT }}
- deno-version: ${{ needs.release-versions.outputs.deno-version }}
- arch: ${{ matrix.arch }}
- service: ${{ matrix.service[2] }}
- type: ${{ matrix.type }}
- setup-docker: false
-
- - name: Image ${{ matrix.service[3] || '"skipped"' }}
- uses: ./.github/actions/build-docker
- if: matrix.service[3] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]'
- env:
- DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }}
- with:
- CR_USER: ${{ secrets.CR_USER }}
- CR_PAT: ${{ secrets.CR_PAT }}
- deno-version: ${{ needs.release-versions.outputs.deno-version }}
- arch: ${{ matrix.arch }}
- service: ${{ matrix.service[3] }}
- type: ${{ matrix.type }}
- setup-docker: false
-Also applies to: 370-413
🤖 Prompt for AI Agents
.github/workflows/ci.yml lines 328-368 (and corresponding 370-413): duplicate
GitHub Actions steps for building images for matrix.service[1-3] cause each
service to be built twice. Remove one of the duplicated blocks (either the new
block at 328-368 or the existing one at 370-413) so each service build step
appears only once; ensure the remaining block keeps the correct if condition,
env and with keys unchanged and that service indexes [1],[2],[3] are present
only in that single set.
bbdcfb6 to
8e80445
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
328-368: Remove duplicate build steps with incorrect suffix logic.Lines 328-368 duplicate the service builds from lines 370-413. Additionally, the simplified DOCKER_TAG_SUFFIX logic in the new block (lines 332, 346, 360) is incorrect—it applies
-covsuffix for any coverage build, while the correct logic (lines 375, 390, 405) restricts it to release/develop contexts.Remove the entire duplicated block (lines 328-368) and keep only lines 370-413 with the correct conditional logic:
- - name: Image ${{ matrix.service[1] || '"skipped"' }} - uses: ./.github/actions/build-docker - if: matrix.service[1] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' - env: - DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} - with: - CR_USER: ${{ secrets.CR_USER }} - CR_PAT: ${{ secrets.CR_PAT }} - deno-version: ${{ needs.release-versions.outputs.deno-version }} - arch: ${{ matrix.arch }} - service: ${{ matrix.service[1] }} - type: ${{ matrix.type }} - setup-docker: false - - - name: Image ${{ matrix.service[2] || '"skipped"' }} - uses: ./.github/actions/build-docker - if: matrix.service[2] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' - env: - DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} - with: - CR_USER: ${{ secrets.CR_USER }} - CR_PAT: ${{ secrets.CR_PAT }} - deno-version: ${{ needs.release-versions.outputs.deno-version }} - arch: ${{ matrix.arch }} - service: ${{ matrix.service[2] }} - type: ${{ matrix.type }} - setup-docker: false - - - name: Image ${{ matrix.service[3] || '"skipped"' }} - uses: ./.github/actions/build-docker - if: matrix.service[3] && (github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'release' || github.ref == 'refs/heads/develop') && github.actor != 'dependabot[bot]' - env: - DOCKER_TAG_SUFFIX_ROCKETCHAT: ${{ matrix.type == 'coverage' && '-cov' || '' }} - with: - CR_USER: ${{ secrets.CR_USER }} - CR_PAT: ${{ secrets.CR_PAT }} - deno-version: ${{ needs.release-versions.outputs.deno-version }} - arch: ${{ matrix.arch }} - service: ${{ matrix.service[3] }} - type: ${{ matrix.type }} - setup-docker: false -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/ci-test-e2e.yml(5 hunks).github/workflows/ci.yml(2 hunks)apps/meteor/package.json(1 hunks)apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js(2 hunks)docker-compose-ci.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/package.json
- apps/meteor/packages/rocketchat-coverage/plugin/compile-version.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
.github/workflows/ci-test-e2e.yml
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
.github/workflows/ci-test-e2e.yml
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
668-668: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
678-678: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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: CodeQL-Build
🔇 Additional comments (6)
docker-compose-ci.yml (1)
4-4: LGTM!The volume mapping and environment variable changes correctly implement dynamic coverage directory handling with sensible defaults. This properly supports the centralized coverage workflow.
Also applies to: 26-26
.github/workflows/ci-test-e2e.yml (5)
81-83: LGTM!The new environment variables properly centralize coverage configuration with per-shard organization and conditional reporter setup.
91-91: LGTM!The updated job name template clearly indicates when coverage is being collected, improving workflow visibility.
161-166: LGTM!The coverage directory initialization properly creates the path and sets permissions for container write access.
243-248: LGTM!The coverage merge step correctly consolidates per-test UI coverage using nyc and conditions execution appropriately.
266-271: LGTM!The artifact storage properly names coverage artifacts per shard and type, enabling correct collection in the report-coverage job.
| ls -la $COVERAGE_DIR | ||
| exit $s |
There was a problem hiding this comment.
Fix undefined variable in exit statement.
Line 214 attempts to exit with an undefined $s variable, causing unpredictable behavior. The variable is never defined in the script.
Capture the test exit status before the stop command and use it properly, or remove the exit statement if the step should pass:
npm run testapi
+ TEST_STATUS=$?
docker compose -f ../../docker-compose-ci.yml stop
ls -la $COVERAGE_DIR
- exit $s
+ exit $TEST_STATUSAlternatively, if the step should always succeed after test completion:
npm run testapi
docker compose -f ../../docker-compose-ci.yml stop
ls -la $COVERAGE_DIR || true
- exit $sCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/ci-test-e2e.yml around lines 213 to 214, the script calls
"exit $s" but $s is never defined; capture the test step exit status into a
variable immediately after the test command (e.g., s=$? or similar) before any
subsequent commands that might change $?, then use that variable in the exit
statement (exit $s), or if the step should always succeed simply remove the exit
statement so the job does not fail.
| include-hidden-files: true | ||
|
|
||
| - name: Report API coverage | ||
| uses: codecov/codecov-action@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
Update deprecated Codecov action to v5.
The Codecov action v3 is deprecated and failing static analysis checks. Update to the latest v5 release:
- name: Report API coverage
- uses: codecov/codecov-action@v3
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/api/lcov.info
working-directory: .
flags: e2e-api
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}
- name: Report UI coverage
- uses: codecov/codecov-action@v3
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/ui/lcov.info
working-directory: .
flags: e2e
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}Verify that input parameters (fail_ci_if_error, files, working-directory, flags, verbose, token) remain compatible with v5 API.
Also applies to: 678-678
I'll verify the key claims in the review comment about the Codecov action deprecation and version compatibility.
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the exact content at lines 668 and 678 in the CI workflow
find . -name "ci.yml" -path "*/.github/workflows/*" -type f | head -1 | xargs -I {} sed -n '665,682p' {}
Length of output: 539
🌐 Web query:
codecov/codecov-action v5 release notes breaking changes
💡 Result:
Key v5 breaking / changed items (from the official release notes):
- Action now uses the Codecov Wrapper (encapsulates the CLI) — migration recommended to @v5. [1]
- Deprecated/renamed inputs:
- file → files (file deprecated). [1]
- plugin → plugins (plugin deprecated). [1]
- New inputs added: binary, gcov_args, gcov_executable, gcov_ignore, gcov_include, report_type, skip_validation, swift_project. [1]
- Token/auth behavior: v5 coincides with the opt‑out global upload token feature for public repos (tokenless uploads behavior changed; see Codecov docs). OIDC is supported via use_oidc (id-token write permission required). [1][2]
References:
[1] codecov/codecov-action (v5 release notes) — GitHub.
[2] Codecov Action listing (Marketplace) — GitHub Marketplace.
🌐 Web query:
codecov-action v3 deprecation status
💡 Result:
Short answer: v3 is effectively deprecated/obsolete — Codecov has moved to newer actions (v4 uses the Codecov CLI; the repo/marketplace recommend using v5 or v4) and v3 (and earlier) will not receive the new CLI features/updates. Upgrade your workflows to v4/v5. (github.com)
Citations:
Update deprecated Codecov action from v3 to v5.
The Codecov action v3 is effectively deprecated; Codecov recommends upgrading to v4 or v5. The parameters you're using (fail_ci_if_error, files, working-directory, flags, verbose, token) remain compatible with v5, as the workflow already uses the files parameter—the correct naming for v5.
- name: Report API coverage
- uses: codecov/codecov-action@v3
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/api/lcov.info
working-directory: .
flags: e2e-api
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}
- name: Report UI coverage
- uses: codecov/codecov-action@v3
+ uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
files: /tmp/coverage_report/ui/lcov.info
working-directory: .
flags: e2e
verbose: true
token: ${{ secrets.CODECOV_TOKEN }}Also applies to: 678-678
🧰 Tools
🪛 actionlint (1.7.8)
668-668: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 668 and 678: the workflow uses the
deprecated codecov/codecov-action@v3; update both occurrences to
codecov/codecov-action@v5 (or @v4 if preferred) keeping the existing parameters
(fail_ci_if_error, files, working-directory, flags, verbose, token) unchanged
since they are compatible with v5; ensure the action version string is replaced
and run a quick workflow lint/validate to confirm no further syntax changes are
required.
rodrigok
force-pushed
the
chore/ci-improvements-2
branch
from
8e80445 to
55ff3dd
Compare
rodrigok
force-pushed
the
chore/ci-improvements-2
branch
from
55ff3dd to
4ba7db6
Compare
https://rocketchat.atlassian.net/browse/ARCH-1854
Depends on #37466
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Chores
New Features
Bug Fixes