test: point oembed API tests at the CI mock-server instead of real providers - #41482
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 |
|
WalkthroughThe E2E workflow now starts the mock server for API tests. Test mode registers a mock OEmbed provider, and chat API tests use deterministic mock URLs to validate previews, metadata extraction, provider errors, and URL limits. ChangesMock-backed OEmbed E2E coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OEmbedE2ETest
participant RocketChat
participant MockServer
OEmbedE2ETest->>MockServer: Configure mock responses
OEmbedE2ETest->>RocketChat: Post message with mock URL
RocketChat->>MockServer: Request OEmbed data or page HTML
MockServer-->>RocketChat: Return configured response
RocketChat-->>OEmbedE2ETest: Return preview metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41482 +/- ##
===========================================
+ Coverage 68.58% 68.61% +0.02%
===========================================
Files 4134 4134
Lines 160802 160842 +40
Branches 29244 29336 +92
===========================================
+ Hits 110294 110369 +75
+ Misses 45403 45378 -25
+ Partials 5105 5095 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…onses take effect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/server/services/messages/lib/oembed/providers.ts (1)
103-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline comments here may run against the "avoid code comments" guideline — likely fine to keep as-is.
Both files add explanatory comments (mock-provider rationale, SSRF-allowlist/TLD quirks, reset-cadence rationale). As per coding guidelines,
**/*.{ts,tsx,js}says to "Avoid code comments in the implementation," but that guideline block's own text frames it around "Playwright tests," and neither file here is a Playwright spec (providers.ts is server code; chat.ts is mocha/chai/supertest). Given the ambiguity and that these specific comments explain non-obvious cross-file behavior, I'd lean toward keeping them.
apps/meteor/server/services/messages/lib/oembed/providers.ts#L103-L110: comment at line 104 explaining the TEST_MODE provider's purpose.apps/meteor/tests/end-to-end/api/chat.ts#L1266-L1320: comments at lines 1266, 1270, and 1313 explaining mock URL routing, the fixed-wait rationale, and mock-server reset semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/services/messages/lib/oembed/providers.ts` around lines 103 - 110, Keep the explanatory comments at the TEST_MODE provider registration in providers.ts and the mock-server setup/reset flow in chat.ts (lines 1266-1320); no code changes are required because these comments document non-obvious behavior.Source: Coding guidelines
.github/workflows/ci-test-e2e.yml (1)
194-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerified: explicit service targeting starts
mock-servercorrectly despite theapiprofile not being activated.Docker Compose explicitly documents that "when you explicitly target a service on the command line that has one or more profiles assigned, ... Compose runs that service regardless of whether its profile is activated," so this script works as intended without needing
COMPOSE_PROFILES.Separately: this step now gates
docker compose up --waitonmock-server's health, but that service's healthcheck (docker-compose-ci.yml, unchanged) has nostart_periodand only 5 retries at a 2s interval — unlikerocketchat's own healthcheck in the same file, which hasstart_period: 60s. Since--waitfails the whole invocation the moment a targeted service reportsunhealthy, a slow CI runner could intermittently fail this step even ifmock-serverwould have come up healthy moments later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-test-e2e.yml around lines 194 - 201, Increase the resilience of the mock-server healthcheck used by the services array in the CI workflow: update its healthcheck configuration in docker-compose-ci.yml to allow slow CI startup, using an appropriate start_period and/or more retries while preserving the existing readiness check. Keep the rocketchat healthcheck and explicit service targeting unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci-test-e2e.yml:
- Around line 194-201: Increase the resilience of the mock-server healthcheck
used by the services array in the CI workflow: update its healthcheck
configuration in docker-compose-ci.yml to allow slow CI startup, using an
appropriate start_period and/or more retries while preserving the existing
readiness check. Keep the rocketchat healthcheck and explicit service targeting
unchanged.
In `@apps/meteor/server/services/messages/lib/oembed/providers.ts`:
- Around line 103-110: Keep the explanatory comments at the TEST_MODE provider
registration in providers.ts and the mock-server setup/reset flow in chat.ts
(lines 1266-1320); no code changes are required because these comments document
non-obvious behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3a22e11-3c0f-41c9-ae0a-316c328a4604
📒 Files selected for processing (4)
.github/workflows/ci-test-e2e.ymlapps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/tests/end-to-end/api/chat.tsdocker-compose-ci.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
🧰 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:
apps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/tests/end-to-end/api/chat.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/tests/end-to-end/api/chat.ts
🔇 Additional comments (1)
docker-compose-ci.yml (1)
230-235: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
KevLehman
added
the
stat: QA assured
|
/jira CORE |
2 participants
Proposed changes (including videos or screenshots)
The
chat.sendMessageoembed API tests fetched real external sites (YouTube's oembed endpoint, rocket.chat) at test time, so slow or failed external fetches flaked the suite — e.g.should generate previews of chosen URL when the previewUrls array is providedfailing withexpected {} not to be emptyafter its fixed 1s sleep.This points the whole oembed suite at the mock-server already used by the ABAC API tests:
providers.tsregisters aTEST_MODE-only oembed provider that resolveshttp://mock-server.local:8080/video/*URLs throughGET /oembedon the mock-server; tests program that endpoint per case with captured real provider payloads (YouTube video, Spotify rich).docker-compose-ci.ymladds amock-server.localnetwork alias, since single-label hostnames failserver-fetchSSRF domain validation.mock-server(the EE job already does viaCOMPOSE_PROFILES=api).sleep(1000)is replaced with the existingretrypolling helper.New coverage on top of the converted tests, using real captured oembed responses:
oembedHtmlmax-width injection, non-string values dropped)oembedUrlLocally verified against the pinned mockserver image: path matching ignores query strings, string bodies work for the generic-page (
pageTitle) case, and the dotted alias resolves via docker DNS from a peer container.Steps to test or reproduce
Run the api E2E suite; the
[Chat] /chat.sendMessage oembedblock no longer performs any external network request.Further comments
Test-only change, no changeset.
Summary by CodeRabbit
Bug Fixes
Tests
Task: CORE-2457