chore: Remove debug flag from Jest configuration and adjust error han… #38175
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSeparates federation container startup from test execution, adds CLI modes (--start-containers-only, --ci, --logs), refactors CI workflow to start services and run tests in distinct steps, consolidates log collection, removes Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Script as run-integration-tests.sh
participant Docker as Docker Compose
participant RC as rc1 (Rocket.Chat)
participant Syn as hs1 (Synapse)
participant Jest as Jest tests
GH->>Script: start step (--start-containers-only)
Script->>Docker: docker compose up --profile COMPOSE_PROFILE
Docker->>RC: start rc1
Docker->>Syn: start hs1
Script->>RC: wait_for_service (health check)
Script->>Syn: wait_for_service (health check)
GH->>Script: test step (--ci)
Script->>Jest: run tests with CI env (QASE/JEST tokens)
Jest-->>Script: return TEST_EXIT_CODE
alt tests failed
Script->>Docker: docker_logs() -> collect rc1 & hs1 logs
end
Script->>Docker: docker compose down --profile COMPOSE_PROFILE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
📚 Learning: 2025-11-05T21:04:35.787ZApplied to files:
⏰ 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). (3)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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 #38175 +/- ##
===========================================
- Coverage 70.71% 70.68% -0.04%
===========================================
Files 3147 3147
Lines 108851 108875 +24
Branches 19592 19566 -26
===========================================
- Hits 76977 76960 -17
- Misses 29877 29909 +32
- Partials 1997 2006 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/15 17:30 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 15 Jan 2026 17:30:41 GMT |
ggazzo
force-pushed
the
ci/fed
branch
2 times, most recently
from
b101af9 to
010aefd
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:674">
P2: `yarn test:integration:logs` restarts the federation stack instead of just showing logs because `--logs` doesn’t disable container startup, so the new log step unintentionally rebuilds services rather than tailing existing logs.</violation>
</file>
<file name="ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh">
<violation number="1" location="ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh:152">
P2: The CI-specific early exit inside cleanup() runs before the failure-log and teardown logic, so failing CI runs no longer print container logs (and may skip cleanup). Move the CI exit until after logs/cleanup have run so diagnostics and teardown always happen before exiting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh`:
- Around line 57-61: The --logs option currently sets LOGS=true and NO_TEST=true
but leaves START_CONTAINERS=true, so the script can still build/start
containers; update the option handling for --logs to also set
START_CONTAINERS=false (and any related flags that cause a build/up) so no
containers are started; apply the same change where --logs is handled in the
other option blocks referenced (the blocks around the existing handling of
LOGS/NO_TEST/START_CONTAINERS) and ensure the early exit branch that prints logs
runs without triggering container startup.
- Around line 290-330: The loop in wait_for_service currently runs curl while
the script may be under set -e, so a failing curl can abort the whole script;
wrap the curl invocation with a temporary disable of errexit (set +e) then
re-enable it (set -e) so you can capture curl_output and curl_exit_code and
still retry. Concretely, in wait_for_service (around the curl_output and
curl_exit_code variables) do: set +e; curl_output=$(curl -fsS --cacert
"$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1);
curl_exit_code=$?; set -e; then use curl_exit_code to decide success/failure as
the existing code does.
♻️ Duplicate comments (2)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
142-149:cleanup()CI early-exit skips temp dir cleanup and is redundant. You already exit withTEST_EXIT_CODEat Lines 186-189; the earlyexitat Lines 143-148 preventsBUILD_DIRdeletion and any in-script failure diagnostics.Proposed diff (remove the early CI exit)
cleanup() { - if [ "$CI" = true ]; then - # Exit with the test result code - if [ -n "${TEST_EXIT_CODE:-}" ]; then - exit $TEST_EXIT_CODE - fi - fi # Show container logs if tests failed if [ -n "${TEST_EXIT_CODE:-}" ] && [ "$TEST_EXIT_CODE" -ne 0 ]; then echo "" echo "==========================================" echo "CONTAINER LOGS (Test Failed)" echo "==========================================" docker_logs fi @@ # Exit with the test result code if [ -n "${TEST_EXIT_CODE:-}" ]; then exit $TEST_EXIT_CODE fi }Also applies to: 181-189
.github/workflows/ci.yml (1)
674-678: Ensure the logs step can’t restart/rebuild the stack. As written,yarn test:integration --logsrelies on the script to avoid startup; ifSTART_CONTAINERSisn’t forced off by--logs, this step can do a freshdocker compose upand produce misleading logs.Proposed diff (belt-and-suspenders even if the script is fixed)
- name: Show federation integration tests logs if: failure() working-directory: ./ee/packages/federation-matrix - run: yarn test:integration --logs + env: + START_CONTAINERS: "false" + run: yarn test:integration --logs
🧹 Nitpick comments (2)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (2)
126-139: Makedocker_logs()resilient whenCOMPOSE_PROFILEis unset. If the script exits before Lines 210-214 (whereCOMPOSE_PROFILEis computed),cleanup()can still calldocker_logs()and fail due to an empty profile.Proposed diff
docker_logs() { + local profile_arg=() + if [ -n "${COMPOSE_PROFILE:-}" ]; then + profile_arg=(--profile "$COMPOSE_PROFILE") + fi echo "" echo "ROCKET.CHAT (rc1) LOGS:" echo "----------------------------------------" - docker compose -f "$DOCKER_COMPOSE_FILE" --profile "$COMPOSE_PROFILE" logs rc1 + docker compose -f "$DOCKER_COMPOSE_FILE" "${profile_arg[@]}" logs rc1 @@ - docker compose -f "$DOCKER_COMPOSE_FILE" --profile "$COMPOSE_PROFILE" logs hs1 + docker compose -f "$DOCKER_COMPOSE_FILE" "${profile_arg[@]}" logs hs1Also applies to: 150-157
26-29: Consider deferringmktempuntil you actually build. In--cimode you setSTART_CONTAINERS=false, butBUILD_DIRis still created at startup (and currently can be skipped bycleanup()due to the CI early-exit).Also applies to: 216-218
📜 Review details
Configuration used: Organization 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 (2)
.github/workflows/ci.ymlee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
658-673: Split start vs test execution looks good; verify the “CI mode” assumptions are explicit.--ciwon’t start containers, so this workflow ordering is required for correctness—worth keeping as-is.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| wait_for_service() { | ||
| local url=$1 | ||
| local name=$2 | ||
| local host=$3 | ||
| local elapsed=0 | ||
| local ca_cert="${CA_CERT:-$PACKAGE_ROOT/docker-compose/traefik/certs/ca/rootCA.crt}" | ||
|
|
||
| # Derive host/port from URL when not explicitly provided | ||
| local host_with_port="${url#*://}" | ||
| host_with_port="${host_with_port%%/*}" | ||
| if [ -z "$host" ]; then | ||
| host="${host_with_port%%:*}" | ||
| fi | ||
| local port | ||
| if [[ "$host_with_port" == *:* ]]; then | ||
| port="${host_with_port##*:}" | ||
| else | ||
| if [[ "$url" == https://* ]]; then | ||
| port=443 | ||
| else | ||
| port=80 | ||
| fi | ||
| fi | ||
|
|
||
| # Wait for Synapse | ||
| if ! wait_for_service "https://hs1/_matrix/client/versions" "Synapse" "hs1"; then | ||
| log_error "Last 50 lines of hs1 logs:" | ||
| docker logs --tail 50 "hs1" 2>&1 | sed 's/^/ /' | ||
| exit 1 | ||
| log_info "Checking $name at $url (host $host -> 127.0.0.1:$port)..." | ||
|
|
||
| while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do | ||
| # Capture curl output and error for debugging | ||
| curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) | ||
| curl_exit_code=$? | ||
|
|
||
| if [ $curl_exit_code -eq 0 ]; then | ||
| log_success "$name is ready!" | ||
| return 0 | ||
| fi | ||
|
|
||
| log_info "$name not ready yet, waiting... (${elapsed}s/${MAX_WAIT_TIME}s)" | ||
| log_info "Curl error: $curl_output" | ||
| sleep $CHECK_INTERVAL | ||
| elapsed=$((elapsed + CHECK_INTERVAL)) | ||
| done |
There was a problem hiding this comment.
wait_for_service() will likely exit on the first failed curl due to set -e. curl_output=$(curl ...) (Line 318) is a “simple command”; with set -e, a non-zero curl can terminate the script before your loop can retry.
Proposed diff (preserve retries and still capture output)
while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do
# Capture curl output and error for debugging
- curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1)
- curl_exit_code=$?
+ curl_exit_code=0
+ curl_output="$(
+ curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1
+ )" || curl_exit_code=$?
if [ $curl_exit_code -eq 0 ]; then
log_success "$name is ready!"
return 0
fi📝 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.
| wait_for_service() { | |
| local url=$1 | |
| local name=$2 | |
| local host=$3 | |
| local elapsed=0 | |
| local ca_cert="${CA_CERT:-$PACKAGE_ROOT/docker-compose/traefik/certs/ca/rootCA.crt}" | |
| # Derive host/port from URL when not explicitly provided | |
| local host_with_port="${url#*://}" | |
| host_with_port="${host_with_port%%/*}" | |
| if [ -z "$host" ]; then | |
| host="${host_with_port%%:*}" | |
| fi | |
| local port | |
| if [[ "$host_with_port" == *:* ]]; then | |
| port="${host_with_port##*:}" | |
| else | |
| if [[ "$url" == https://* ]]; then | |
| port=443 | |
| else | |
| port=80 | |
| fi | |
| fi | |
| # Wait for Synapse | |
| if ! wait_for_service "https://hs1/_matrix/client/versions" "Synapse" "hs1"; then | |
| log_error "Last 50 lines of hs1 logs:" | |
| docker logs --tail 50 "hs1" 2>&1 | sed 's/^/ /' | |
| exit 1 | |
| log_info "Checking $name at $url (host $host -> 127.0.0.1:$port)..." | |
| while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do | |
| # Capture curl output and error for debugging | |
| curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) | |
| curl_exit_code=$? | |
| if [ $curl_exit_code -eq 0 ]; then | |
| log_success "$name is ready!" | |
| return 0 | |
| fi | |
| log_info "$name not ready yet, waiting... (${elapsed}s/${MAX_WAIT_TIME}s)" | |
| log_info "Curl error: $curl_output" | |
| sleep $CHECK_INTERVAL | |
| elapsed=$((elapsed + CHECK_INTERVAL)) | |
| done | |
| wait_for_service() { | |
| local url=$1 | |
| local name=$2 | |
| local host=$3 | |
| local elapsed=0 | |
| local ca_cert="${CA_CERT:-$PACKAGE_ROOT/docker-compose/traefik/certs/ca/rootCA.crt}" | |
| # Derive host/port from URL when not explicitly provided | |
| local host_with_port="${url#*://}" | |
| host_with_port="${host_with_port%%/*}" | |
| if [ -z "$host" ]; then | |
| host="${host_with_port%%:*}" | |
| fi | |
| local port | |
| if [[ "$host_with_port" == *:* ]]; then | |
| port="${host_with_port##*:}" | |
| else | |
| if [[ "$url" == https://* ]]; then | |
| port=443 | |
| else | |
| port=80 | |
| fi | |
| fi | |
| log_info "Checking $name at $url (host $host -> 127.0.0.1:$port)..." | |
| while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do | |
| # Capture curl output and error for debugging | |
| curl_exit_code=0 | |
| curl_output="$( | |
| curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1 | |
| )" || curl_exit_code=$? | |
| if [ $curl_exit_code -eq 0 ]; then | |
| log_success "$name is ready!" | |
| return 0 | |
| fi | |
| log_info "$name not ready yet, waiting... (${elapsed}s/${MAX_WAIT_TIME}s)" | |
| log_info "Curl error: $curl_output" | |
| sleep $CHECK_INTERVAL | |
| elapsed=$((elapsed + CHECK_INTERVAL)) | |
| done |
🤖 Prompt for AI Agents
In `@ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh` around
lines 290 - 330, The loop in wait_for_service currently runs curl while the
script may be under set -e, so a failing curl can abort the whole script; wrap
the curl invocation with a temporary disable of errexit (set +e) then re-enable
it (set -e) so you can capture curl_output and curl_exit_code and still retry.
Concretely, in wait_for_service (around the curl_output and curl_exit_code
variables) do: set +e; curl_output=$(curl -fsS --cacert "$ca_cert" --resolve
"${host}:${port}:127.0.0.1" "$url" 2>&1); curl_exit_code=$?; set -e; then use
curl_exit_code to decide success/failure as the existing code does.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh`:
- Line 167: The log message passed to log_info currently nests unescaped double
quotes which leaves $DOCKER_COMPOSE_FILE and $COMPOSE_PROFILE unquoted; fix by
ensuring the docker compose flags wrap those variables in quotes (e.g., escape
the inner quotes or build the string using single-quoted literal parts and
properly quoted variable expansions) so that "$DOCKER_COMPOSE_FILE" and
"$COMPOSE_PROFILE" are quoted in the final string passed to log_info.
♻️ Duplicate comments (2)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (2)
56-60:--logsshould setSTART_CONTAINERS=falseto avoid rebuilding containers.The
--logsoption is intended to print logs and exit, but it doesn't disable container startup. WhenSTART_CONTAINERSremainstrue, the script will attempt to build/start containers before reaching the logs-only exit branch at lines 357-359.Proposed fix
--logs) LOGS=true NO_TEST=true + START_CONTAINERS=false shift ;;
308-322:curlfailure will exit the script due toset -e, breaking retry logic.Under
set -e, a failingcurlcommand on line 310 will terminate the script beforecurl_exit_code=$?on line 311 can capture the exit code. This breaks the intended retry loop.Proposed fix to preserve retries
while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do # Capture curl output and error for debugging - curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) - curl_exit_code=$? + curl_exit_code=0 + curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) || curl_exit_code=$? if [ $curl_exit_code -eq 0 ]; then log_success "$name is ready!"
🧹 Nitpick comments (1)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
84-98: Document the new CLI flags in--help.The help text doesn't mention
--start-containers-only,--ci, or--logs, which makes local usage and debugging harder.Proposed addition to help text
--help|-h) echo "Usage: $0 [OPTIONS]" echo "Options:" echo " --keep-running Keep Docker containers running after tests complete" echo " --element Include Element web client in the test environment" echo " --no-test Start containers and skip running tests" + echo " --start-containers-only Start containers and exit (implies --keep-running)" + echo " --ci CI mode: skip container startup, run tests only" + echo " --logs Print container logs and exit" echo " --image [IMAGE] Use a pre-built Docker image instead of building locally" echo " --help, -h Show this help message"
📜 Review details
Configuration used: Organization 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 (4)
.github/workflows/ci.ymlee/packages/federation-matrix/jest.config.federation.tsee/packages/federation-matrix/tests/helper/synapse-client.tsee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/jest.config.federation.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/tests/helper/synapse-client.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/tests/helper/synapse-client.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/helper/synapse-client.ts
🪛 Shellcheck (0.11.0)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
[warning] 167-167: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 167-167: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ 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: 📦 Meteor Build (coverage)
🔇 Additional comments (6)
ee/packages/federation-matrix/tests/helper/synapse-client.ts (1)
11-16: LGTM! Global logger silencing complements the per-client silent logger.The global override of
matrix-js-sdklogger methods effectively silences SDK-internal logging during tests, while the existingsilentLoggerobject (lines 95-107) handles per-client logging. Both approaches work together to reduce test output noise.ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (2)
125-138: LGTM! Good consolidation of log printing logic.The
docker_logs()function properly centralizes log collection for both failure diagnostics and the--logsCLI mode.
353-356: LGTM! Proper error handling around test execution.The
set +e/set -epattern correctly allows capturing the test exit code without prematurely terminating the script..github/workflows/ci.yml (3)
658-665: LGTM! Good separation of container startup from test execution.Separating the container startup (
--start-containers-only) from test execution (--ci) improves CI reliability and allows better control over each phase. The environment variables for QASE integration and PR tracking are properly configured.
674-677: Verify that--logsworks correctly without starting containers.This step relies on the
--logsflag in the integration test script. As noted in the script review,--logscurrently doesn't setSTART_CONTAINERS=false, which could cause unexpected behavior. Once the script fix is applied, this step will work as intended.
667-672: LGTM! CI test execution step is correctly configured.The
--ciflag properly skips container startup (since containers are already running from the previous step), and the QASE environment variables enable test reporting integration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
Outdated
Show resolved
Hide resolved
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
374-374: Typo in manual test command.
yarn testend-to-endappears to be missing a separator. Based on line 358, this should beyarn test:federation.✏️ Proposed fix
- log_info "To run tests manually, execute: yarn testend-to-end" + log_info "To run tests manually, execute: yarn test:federation"
♻️ Duplicate comments (1)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
312-326:set -ebreaks the retry loop—first failedcurlexits the script.The command substitution
curl_output=$(curl ...)inherits curl's exit code. Withset -eactive (from line 7), a non-zero exit terminates the script immediately, preventing retries.🐛 Proposed fix
while [ $elapsed -lt $MAX_WAIT_TIME ] && [ "$INTERRUPTED" = false ]; do # Capture curl output and error for debugging - curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) - curl_exit_code=$? + curl_exit_code=0 + curl_output=$(curl -fsS --cacert "$ca_cert" --resolve "${host}:${port}:127.0.0.1" "$url" 2>&1) || curl_exit_code=$? if [ $curl_exit_code -eq 0 ]; then log_success "$name is ready!" return 0 fi
📜 Review details
Configuration used: Organization 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 (1)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (6)
37-40: LGTM!Clean initialization of the new mode flags with sensible defaults.
44-60: LGTM!The new CLI options are well-structured with appropriate flag combinations:
--start-containers-onlycorrectly implies--keep-running--cimode properly skips container startup (handled externally) while keeping containers running--logscorrectly setsSTART_CONTAINERS=falseto avoid unintended builds
84-101: LGTM!Help text is comprehensive and documents all the new options clearly.
128-141: LGTM!Clean extraction of log printing into a reusable function. This reduces duplication and ensures consistent log output format.
143-189: LGTM!The cleanup function properly handles different modes:
- CI mode exits early (logs collected separately via
--logsstep)- Non-CI mode shows logs on failure before cleanup
- Proper extraction of
docker_logs()reduces duplication
357-360: LGTM!Correct use of
set +e/set -eto capture the test exit code without terminating the script. This ensures cleanup runs regardless of test outcome.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…dling in integration test script
…ging and test execution flow
…d improve error handling
…er test execution
…tiple package.json and yarn.lock files
…dling in integration test script
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.