Skip to content

Commit f2f42c4

Browse files
sampaiodiegoclaude
sampaiodiego
and
claude
committed
test: remove redundant schema validation tests from banners
These tests only validated payload schema rejection (missing/invalid params) which is now enforced at build time by the new API.v1.get/post pattern with AJV-compiled validators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 92a44ee commit f2f42c4

2 files changed

Lines changed: 223 additions & 79 deletions

File tree

‎apps/meteor/tests/end-to-end/api/banners.ts‎

Lines changed: 0 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -21,33 +21,6 @@ describe('banners', () => {
2121
.end(done);
2222
});
2323

24-
it('should fail if missing bannerId key', (done) => {
25-
void request
26-
.post(api('banners.dismiss'))
27-
.set(credentials)
28-
.send({})
29-
.expect(400)
30-
.expect((res) => {
31-
expect(res.body).to.have.property('success', false);
32-
expect(res.body).to.have.property('errorType', 'invalid-params');
33-
})
34-
.end(done);
35-
});
36-
37-
it('should fail if bannerId is empty', (done) => {
38-
void request
39-
.post(api('banners.dismiss'))
40-
.set(credentials)
41-
.send({
42-
bannerId: '',
43-
})
44-
.expect(400)
45-
.expect((res) => {
46-
expect(res.body).to.have.property('success', false);
47-
})
48-
.end(done);
49-
});
50-
5124
it('should fail if bannerId is invalid', (done) => {
5225
void request
5326
.post(api('banners.dismiss'))
@@ -77,32 +50,6 @@ describe('banners', () => {
7750
});
7851
});
7952

80-
it('should fail if missing platform', async () => {
81-
return request
82-
.get(api('banners'))
83-
.set(credentials)
84-
.query({})
85-
.expect(400)
86-
.expect((res) => {
87-
expect(res.body).to.have.property('success', false);
88-
expect(res.body).to.have.property('errorType', 'error-invalid-params');
89-
});
90-
});
91-
92-
it('should fail if platform is invalid', async () => {
93-
return request
94-
.get(api('banners'))
95-
.set(credentials)
96-
.query({
97-
platform: 'invalid-platform',
98-
})
99-
.expect(400)
100-
.expect((res) => {
101-
expect(res.body).to.have.property('success', false);
102-
expect(res.body).to.have.property('errorType', 'error-invalid-params');
103-
});
104-
});
105-
10653
it('should succesfully return web banners', async () => {
10754
return request
10855
.get(api('banners'))
@@ -146,32 +93,6 @@ describe('banners', () => {
14693
});
14794
});
14895

149-
it('should fail if missing platform', async () => {
150-
return request
151-
.get(api('banners/some-id'))
152-
.set(credentials)
153-
.query({})
154-
.expect(400)
155-
.expect((res) => {
156-
expect(res.body).to.have.property('success', false);
157-
expect(res.body).to.have.property('errorType', 'error-invalid-params');
158-
});
159-
});
160-
161-
it('should fail if platform is invalid', async () => {
162-
return request
163-
.get(api('banners/some-id'))
164-
.set(credentials)
165-
.query({
166-
platform: 'invalid-platform',
167-
})
168-
.expect(400)
169-
.expect((res) => {
170-
expect(res.body).to.have.property('success', false);
171-
expect(res.body).to.have.property('errorType', 'error-invalid-params');
172-
});
173-
});
174-
17596
it('should succesfully return a web banner by id', async () => {
17697
return request
17798
.get(api('banners/some-id'))

‎docs/removing-redundant-schema-validation-tests.md‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
# Removing Redundant Schema Validation E2E Tests
2+
3+
## Context
4+
5+
Rocket.Chat has migrated many API endpoints from the legacy `API.v1.addRoute()` pattern to the new `API.v1.[method]()` pattern (`API.v1.get`, `API.v1.post`, `API.v1.put`, `API.v1.delete`). The new pattern enforces **build-time input validation** (via AJV-compiled schemas on `query`/`body` options) and **runtime response validation** (in test mode). This makes many E2E tests that only verify payload schema rejection redundant.
6+
7+
This document describes a repeatable process for identifying and removing those redundant tests.
8+
9+
---
10+
11+
## Step 1: Identify fully-migrated endpoint files
12+
13+
An endpoint file is "fully migrated" when it uses **only** `API.v1.get()`, `API.v1.post()`, `API.v1.put()`, or `API.v1.delete()` and has **zero** calls to `API.v1.addRoute()`.
14+
15+
### How to check
16+
17+
```bash
18+
# List files that ONLY use the new pattern (no addRoute calls)
19+
# First, find all endpoint files using the new pattern:
20+
grep -rl 'API\.v1\.\(get\|post\|put\|delete\)(' apps/meteor/app/api/server/v1/ \
21+
apps/meteor/app/livechat/server/api/ \
22+
apps/meteor/ee/ \
23+
--include='*.ts' | sort > /tmp/new-pattern-files.txt
24+
25+
# Then, find files that still use addRoute:
26+
grep -rl 'API\.v1\.addRoute(' apps/meteor/app/api/server/v1/ \
27+
apps/meteor/app/livechat/server/api/ \
28+
apps/meteor/ee/ \
29+
--include='*.ts' | sort > /tmp/old-pattern-files.txt
30+
31+
# Files in new but NOT in old are fully migrated:
32+
comm -23 /tmp/new-pattern-files.txt /tmp/old-pattern-files.txt
33+
```
34+
35+
### Partially migrated files
36+
37+
Some files use **both** patterns (e.g., `push.ts` has 3 new-pattern endpoints and 2 old-pattern endpoints). For these, you must check **per-endpoint** whether the specific endpoint being tested has been migrated. Only remove schema validation tests for endpoints that use the new pattern.
38+
39+
To check a specific endpoint:
40+
41+
```bash
42+
# Check if a specific endpoint uses the new or old pattern
43+
grep -n 'push.token' apps/meteor/app/api/server/v1/push.ts
44+
# If the line shows API.v1.post('push.token', ...) → migrated
45+
# If the line shows API.v1.addRoute('push.token', ...) → NOT migrated
46+
```
47+
48+
---
49+
50+
## Step 2: Identify schema-validation-only tests in the corresponding test file
51+
52+
Test files live in `apps/meteor/tests/end-to-end/api/` and typically match the endpoint file name (e.g., `banners.ts` endpoint → `banners.ts` test file).
53+
54+
### Indicators that a test is ONLY validating schema (safe to remove)
55+
56+
A test is a schema-validation-only test if it matches **ALL** of these criteria:
57+
58+
1. **Sends an incomplete, empty, or wrong-typed payload** — e.g., `.send({})`, omits required fields, sends `true` where `string` is expected
59+
2. **Expects a 400 response** with one of these error patterns:
60+
- `errorType: 'invalid-params'` (the AJV validation error type)
61+
- Error message contains AJV phrases: `"must have required property"`, `"must be equal to one of the allowed values"`, `"must NOT have fewer than 1 characters"`, `"must be string"`, `"must be number"`, `"must match pattern"`
62+
- Error message is `'Body parameter "X" is required.'` or similar parameter-presence checks
63+
3. **No resource setup or teardown** — no `createUser`, `createRoom`, `createTeam`, database operations, or `before`/`after` hooks specific to the test
64+
4. **Single request-response cycle** — one HTTP call, check the error, done
65+
5. **Uses the `expectInvalidParams` helper** from `tests/data/validation.helper.ts` (automatic indicator)
66+
67+
### Common test name patterns for schema-only tests
68+
69+
```
70+
should fail if missing X
71+
should fail if X is not provided
72+
should fail if X is empty
73+
should return an error when the required "X" parameter is not sent
74+
should return bad request if X is not provided
75+
should fail if X param is unknown/invalid (when testing enum values)
76+
should return an error when the X is not provided
77+
```
78+
79+
### How to find them programmatically
80+
81+
```bash
82+
# Find tests that assert 'invalid-params' errorType (strong signal)
83+
grep -n "errorType.*invalid-params\|invalid-params.*errorType" \
84+
apps/meteor/tests/end-to-end/api/*.ts \
85+
apps/meteor/tests/end-to-end/api/**/*.ts
86+
87+
# Find tests that assert AJV error messages (strong signal)
88+
grep -n "must have required property\|must be equal to one of the allowed values\|must NOT have fewer than" \
89+
apps/meteor/tests/end-to-end/api/*.ts \
90+
apps/meteor/tests/end-to-end/api/**/*.ts
91+
92+
# Find tests named with schema-validation patterns
93+
grep -n "should fail if missing\|should fail if.*not provided\|should fail if.*empty\|should return.*error.*required.*parameter" \
94+
apps/meteor/tests/end-to-end/api/*.ts \
95+
apps/meteor/tests/end-to-end/api/**/*.ts
96+
```
97+
98+
---
99+
100+
## Step 3: Cross-reference — only remove tests for migrated endpoints
101+
102+
**Critical rule:** Only remove a schema-validation test if the endpoint it tests has been migrated to the new `API.v1.[method]()` pattern.
103+
104+
### Decision matrix
105+
106+
| Endpoint uses new API? | Test is schema-validation-only? | Action |
107+
| ---------------------- | ------------------------------- | ------------------------------------------------ |
108+
| Yes | Yes | **REMOVE** the test |
109+
| Yes | No (integration test) | **KEEP** the test |
110+
| No | Yes | **KEEP** the test (still needed until migration) |
111+
| No | No | **KEEP** the test |
112+
113+
---
114+
115+
## Step 4: What to KEEP (do NOT remove these)
116+
117+
Even if an endpoint is migrated, keep tests that:
118+
119+
1. **Test business logic validation** — errors like `'error-invalid-room'`, `'error-contact-manager-not-found'`, `'error-unauthorized'`, `'Not allowed'`. These validate application logic, not schema.
120+
2. **Test authentication guards** (`should fail if not logged in`, expects 401) — while `authRequired: true` is enforced by the framework too, auth tests serve as a security regression gate and are cheap to run. Use team judgment on whether to keep or remove.
121+
3. **Test authorization/permissions** — errors about missing permissions, role checks
122+
4. **Test with semantically valid payloads** — the payload passes schema validation but fails a business rule (e.g., `userId` is valid format but user doesn't exist)
123+
5. **Create/verify resources or side effects** — multi-step tests that verify state changes
124+
125+
### Gray area: how to distinguish business logic from schema validation
126+
127+
- If `errorType` is `'invalid-params'` → **schema validation** (AJV layer)
128+
- If `errorType` is anything else (e.g., `'error-invalid-room'`, `'error-not-found'`, custom error) → **business logic** (keep)
129+
- If the error message contains AJV vocabulary (`"must have required property"`, `"must be"`, `"must NOT"`) → **schema validation**
130+
- If the error message is application-specific → **business logic** (keep)
131+
132+
---
133+
134+
## Step 5: Execute the removal
135+
136+
For each test file with removable tests:
137+
138+
1. Read the test file
139+
2. Identify the `it(...)` blocks that are schema-validation-only (per Step 2 criteria)
140+
3. Remove the `it(...)` blocks
141+
4. If removing all tests in a `describe(...)` block, remove the entire `describe` block
142+
5. If removing all tests in a file, delete the file entirely
143+
6. Clean up unused imports that were only used by removed tests
144+
7. Run the remaining tests to ensure nothing is broken:
145+
```bash
146+
yarn workspace @rocket.chat/meteor testapi --grep "endpoint-name"
147+
```
148+
149+
---
150+
151+
## Step 6: Verify with the OpenAPI docs endpoint
152+
153+
You can verify which endpoints are fully migrated by querying the OpenAPI endpoint:
154+
155+
```bash
156+
# Get only migrated (documented) endpoints
157+
curl http://localhost:3000/api/docs/json | jq '.paths | keys'
158+
159+
# Get ALL endpoints including unmigrated ones
160+
curl http://localhost:3000/api/docs/json?withUndocumented=true | jq '.paths | keys'
161+
162+
# Difference = unmigrated endpoints
163+
```
164+
165+
Endpoints tagged with `'Missing Documentation'` in the OpenAPI output are still using the old pattern.
166+
167+
---
168+
169+
## Already Fully Migrated Endpoint Files (as of 2026-03-31)
170+
171+
These files use **only** the new `API.v1.[method]()` pattern:
172+
173+
| Endpoint file | Test file | Schema-only tests to remove |
174+
| ------------------------------------ | --------------------------------------- | --------------------------- |
175+
| `app/api/server/v1/banners.ts` | `tests/end-to-end/api/banners.ts` | 6 tests |
176+
| `app/api/server/v1/custom-sounds.ts` | `tests/end-to-end/api/custom-sounds.ts` | 3 tests |
177+
| `app/api/server/v1/moderation.ts` | `tests/end-to-end/api/moderation.ts` | 9 tests |
178+
179+
### Partially migrated (check per-endpoint):
180+
181+
| Endpoint file | Migrated endpoints | Not migrated |
182+
| --------------------------- | ------------------------------------------------------- | ----------------------- |
183+
| `app/api/server/v1/push.ts` | `push.token` (POST), `push.token` (DELETE), `push.test` | `push.get`, `push.info` |
184+
185+
For `push.ts`, the 7 schema-validation tests for `push.token` POST and DELETE can be removed. Tests for `push.get` and `push.info` should be kept.
186+
187+
---
188+
189+
## Automation Script (for future runs)
190+
191+
To repeat this process after more endpoints are migrated, run these steps:
192+
193+
```bash
194+
# 1. Generate list of fully-migrated endpoint files
195+
grep -rl 'API\.v1\.\(get\|post\|put\|delete\)(' apps/meteor/app/api/server/v1/ --include='*.ts' | \
196+
while read f; do
197+
if ! grep -q 'API\.v1\.addRoute(' "$f"; then
198+
echo "$f"
199+
fi
200+
done
201+
202+
# 2. For each migrated file, find the corresponding test file
203+
# Endpoint: apps/meteor/app/api/server/v1/XXXX.ts
204+
# Test: apps/meteor/tests/end-to-end/api/XXXX.ts
205+
206+
# 3. In each test file, find schema-validation tests
207+
# Look for the patterns described in Step 2
208+
209+
# 4. Review and remove, then run tests
210+
```
211+
212+
---
213+
214+
## Key Files Reference
215+
216+
| Purpose | Path |
217+
| --------------------------------------------------- | ----------------------------------------------- |
218+
| New API class (`get`/`post`/`put`/`delete` methods) | `apps/meteor/app/api/server/ApiClass.ts` |
219+
| API instantiation (`API.v1`) | `apps/meteor/app/api/server/api.ts` |
220+
| Schema validation test helper | `apps/meteor/tests/data/validation.helper.ts` |
221+
| AJV validators (rest-typings) | `packages/rest-typings/src/v1/` |
222+
| OpenAPI docs endpoint | `apps/meteor/app/api/server/default/openApi.ts` |
223+
| E2E API tests | `apps/meteor/tests/end-to-end/api/` |

0 commit comments

Comments
 (0)