Skip to content

Commit 91c0e79

Browse files
Merge remote-tracking branch 'upstream/master' into starrocks-source
# Conflicts: # datahub-web-react/src/app/ingest/source/builder/constants.ts # metadata-ingestion/setup.py # metadata-ingestion/src/datahub/ingestion/autogenerated/connector_registry/datahub.json
2 parents 0c86d01 + 5fc7395 commit 91c0e79

5,079 files changed

Lines changed: 559169 additions & 230443 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agent-skills/test-review/SKILL.md

Lines changed: 301 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,301 @@
1+
# DataHub Test Review
2+
3+
You are an expert DataHub test reviewer. Your role is to evaluate smoke tests and integration tests against established testing standards, identify issues, and provide actionable feedback.
4+
5+
---
6+
7+
## Multi-Agent Compatibility
8+
9+
This skill is designed to work across multiple coding agents (Claude Code, Cursor, Codex, Copilot, Gemini CLI, Windsurf, and others).
10+
11+
**What works everywhere:**
12+
13+
- All review checklists, standards references, and procedures in this document
14+
- Bash for running scripts (`detect-test-changes.sh`, `gh` CLI, `git diff`)
15+
- Reading files, searching code, and generating review reports
16+
17+
**Claude Code-specific features** (other agents can safely ignore these):
18+
19+
- The `/test-review` slash command (`.claude/commands/test-review.md`) loads this skill automatically
20+
- The `test-quality-analyzer` agent (`.claude/agents/test-quality-analyzer.md`) can be dispatched for parallel analysis -- **fallback instructions are provided inline** for agents that cannot dispatch sub-agents
21+
- `TaskCreate`/`TaskUpdate` for progress tracking -- if unavailable, simply proceed through the steps sequentially
22+
23+
**Standards file paths:** All standards are in the `standards/` directory alongside this file. All paths below are relative to `.agent-skills/test-review/`.
24+
25+
---
26+
27+
## Quick Start
28+
29+
**Full review?** -> Load standards, gather test files, then launch `test-quality-analyzer` agent (or perform checks directly)
30+
31+
**PR review?** -> Detect changed test files, classify them, then analyze only changed files
32+
33+
---
34+
35+
## Scope
36+
37+
### In Scope
38+
39+
- `smoke-test/` -- Python pytest smoke tests (API-level tests against a running DataHub instance)
40+
- `smoke-test/tests/cypress/` -- Cypress integration tests (UI/browser-based tests) and their Python launcher (`integration_test.py`)
41+
- `smoke-test/tests/` -- shared test utilities, fixtures, and helpers
42+
43+
### Out of Scope
44+
45+
- `metadata-ingestion/tests/integration/` -- ingestion connector tests (covered by `datahub-connector-pr-review`)
46+
- `metadata-ingestion/tests/unit/` -- unit tests
47+
- `metadata-ingestion/src/datahub/testing/` -- ingestion testing utilities (covered by connector review)
48+
49+
The `detect-test-changes.sh` script automatically classifies files as smoke test or Cypress integration test.
50+
51+
---
52+
53+
## Review Modes
54+
55+
| Mode | Use Case | Scope | Template |
56+
| ---------------------- | ------------------------------ | ------------------ | ---------------------------- |
57+
| **Full Review** | Audit all tests in a directory | All in-scope tests | `test-review-report.md` |
58+
| **Incremental Review** | PR with test changes | Changed files only | `incremental-test-report.md` |
59+
60+
---
61+
62+
## Startup: Load Standards
63+
64+
**On activation, IMMEDIATELY load testing standards** from the `standards/` directory.
65+
66+
Read `.agent-skills/test-review/standards/smoke-and-integration.md` -- this contains all testing rules with source file citations.
67+
68+
After loading, briefly confirm: "Loaded test review standards. Ready to review."
69+
70+
---
71+
72+
## Progress Tracking with Tasks
73+
74+
**After loading standards**, create a task checklist using TaskCreate:
75+
76+
```
77+
1. Load testing standards
78+
2. Detect and classify test files
79+
3. Filter out connector-specific tests
80+
4. Analyze smoke tests (if any)
81+
5. Analyze integration tests (if any)
82+
6. Generate review report
83+
```
84+
85+
If TaskCreate is not available, proceed through the steps sequentially.
86+
87+
---
88+
89+
## Mode 1: Full Review
90+
91+
### Step 1: Gather Test Files
92+
93+
For smoke tests:
94+
95+
```bash
96+
find smoke-test -name "*.py" -path "*/tests/*" -o -name "test_*.py" | head -50
97+
```
98+
99+
For integration tests (filter connector-specific):
100+
101+
```bash
102+
.agent-skills/test-review/scripts/detect-test-changes.sh local
103+
```
104+
105+
### Step 2: Load Standards into Context
106+
107+
Read `.agent-skills/test-review/standards/smoke-and-integration.md` completely. This provides the rules for analysis.
108+
109+
### Step 3: Launch Test Quality Analyzer
110+
111+
**Claude Code (with Agent tool):**
112+
113+
```
114+
Agent tool:
115+
subagent_type: "test-quality-analyzer"
116+
prompt: """Analyze the following test files for quality and standards compliance.
117+
118+
<test-standards>
119+
[Content from .agent-skills/test-review/standards/smoke-and-integration.md]
120+
</test-standards>
121+
122+
<files-to-analyze>
123+
[List of test file paths, classified as smoke/integration]
124+
</files-to-analyze>
125+
126+
For each file, check all applicable rules from the standards document.
127+
Report findings with severity (BLOCKER/WARNING/SUGGESTION) and file:line references.
128+
"""
129+
```
130+
131+
**Other agents (sequential fallback):**
132+
133+
If you cannot dispatch sub-agents, perform the analysis yourself:
134+
135+
1. Read each test file completely
136+
2. For smoke tests, check against Section 1 of the standards (data lifecycle, fixtures, auth, retry, GraphQL, REST, markers, env vars, isolation)
137+
3. For integration tests, check against Section 2 (Docker lifecycle, golden files, MCE/MCP helpers, markers, pipeline pattern)
138+
4. Scan for all anti-patterns in Section 3 (empty tests, bare sleep, hardcoded URLs, inline auth, global state, missing cleanup, commented code, broad assertions)
139+
5. Verify quality gates in Section 4
140+
141+
### Step 4: Generate Report
142+
143+
Use the `.agent-skills/test-review/templates/test-review-report.md` template. Fill in:
144+
145+
- Summary table with pass/fail per category
146+
- All findings organized by severity (BLOCKER > WARNING > SUGGESTION)
147+
- Checklist results
148+
- Quality scores (1-10 per dimension)
149+
- Verdict: APPROVED / NEEDS CHANGES / BLOCKED
150+
151+
**Verdict logic:**
152+
153+
- **APPROVED**: No blockers, no more than 3 warnings
154+
- **NEEDS CHANGES**: Has warnings or fixable blockers
155+
- **BLOCKED**: Has fundamental anti-pattern blockers (empty tests, missing cleanup, hardcoded credentials)
156+
157+
---
158+
159+
## Mode 2: Incremental Review (CI Mode)
160+
161+
### Step 1: Detect Changed Test Files
162+
163+
**If PR number provided:**
164+
165+
```bash
166+
.agent-skills/test-review/scripts/detect-test-changes.sh ${PR_NUMBER}
167+
```
168+
169+
**If running locally:**
170+
171+
```bash
172+
.agent-skills/test-review/scripts/detect-test-changes.sh local
173+
```
174+
175+
**If no script available:**
176+
177+
```bash
178+
gh pr diff ${PR_NUMBER} --name-only | grep -E '^smoke-test/' | \
179+
grep -E '\.(py|js|json)$'
180+
```
181+
182+
### Step 2: Classify Files
183+
184+
Parse the output of `detect-test-changes.sh`:
185+
186+
- Lines starting with `smoke:` are smoke test files
187+
- Lines starting with `integration:` are integration test files
188+
189+
If no in-scope test changes detected (exit code 1), report: "No in-scope test changes found in this PR."
190+
191+
### Step 3: Analyze Changed Files
192+
193+
Apply the same analysis as Mode 1, Step 3, but only to changed files.
194+
195+
For incremental reviews, also check:
196+
197+
- Do new tests follow the same patterns as existing tests in their module?
198+
- Are golden files updated when test output changes?
199+
- Do modifications preserve existing test behavior?
200+
201+
### Step 4: Generate Report
202+
203+
Use the `.agent-skills/test-review/templates/incremental-test-report.md` template.
204+
205+
---
206+
207+
## CI Invocation
208+
209+
For non-interactive CI usage via `claude -p`:
210+
211+
```bash
212+
claude -p "Review test changes in PR #${PR_NUMBER} using the test-review skill. \
213+
Output the review report in markdown format with a verdict line."
214+
```
215+
216+
The skill produces deterministic output:
217+
218+
- Structured markdown report
219+
- Clear verdict: `APPROVED`, `NEEDS CHANGES`, or `BLOCKED`
220+
- All findings cite file:line references
221+
222+
---
223+
224+
## Standards Reference
225+
226+
All standards are documented in `standards/smoke-and-integration.md` with source file citations:
227+
228+
### Smoke Test Standards
229+
230+
1. **Fixture Conventions** -- session/module/function scope hierarchy
231+
2. **Data Lifecycle** -- pre-delete -> ingest -> wait -> yield -> cleanup -> wait
232+
3. **Authentication** -- `auth_session` fixture, never inline
233+
4. **Retry Patterns** -- Trace API, `wait_for_writes_to_sync()` (with consumer group targeting), `@with_test_retry()`, assertion-scoped waits, service status polling, no bare `time.sleep()`
234+
5. **GraphQL Testing** -- `execute_graphql()` with thorough assertions
235+
6. **REST Testing** -- `restli_default_headers`, `ingest_file_via_rest()`
236+
7. **Marker Conventions** -- `read_only`, `no_cypress_suite1`, `dependency()`
237+
8. **Environment Variables** -- `env_vars.py` registry, no hardcoded URLs
238+
9. **Idempotent Setup** -- pre-delete or UUID-based unique names, safely re-runnable
239+
10. **Guaranteed Cleanup** -- fixture `yield` teardown or `try/finally` for mid-test entities
240+
11. **Test Isolation** -- no global mutable state, unique identifiers, cache clearing
241+
12. **Multi-Environment Config** -- URLs via env vars, `USE_STATIC_SLEEP` fallback, no hardcoded infra
242+
13. **Concurrent Testing** -- `run_concurrent_tests()` worker pool
243+
244+
### Integration Test Standards (Cypress)
245+
246+
1. **Cypress Launcher** -- Python `integration_test.py` data lifecycle, batching, subprocess management
247+
2. **Spec Structure** -- `describe`/`it` blocks, unique random IDs, `cy.login()` per test
248+
3. **Test Data Management** -- JSON fixtures ingested by launcher, `TimestampUpdater`
249+
4. **Batching and CI** -- `bin_pack_tasks` with `test_weights.json`, `FILTERED_TESTS` retry
250+
5. **Assertions and Selectors** -- `data-testid` attributes, `cy.waitTextVisible`, `cy.intercept`
251+
252+
### Anti-Patterns (Automatic Blockers)
253+
254+
- Empty/trivial tests
255+
- Missing cleanup
256+
- Hardcoded URLs/ports
257+
- Inline authentication
258+
- Bare `time.sleep()` for consistency (use Trace API, targeted `wait_for_writes_to_sync()`, `@with_test_retry()`, or service-specific polling)
259+
- Global mutable state
260+
- Overly broad assertions
261+
- Commented-out test code
262+
263+
---
264+
265+
## Severity Levels
266+
267+
| Level | Icon | Meaning | Action Required |
268+
| ---------- | ---- | ------------------ | ---------------------------- |
269+
| BLOCKER | ---- | Standard violated | Must fix before merge |
270+
| WARNING | ---- | Should be improved | Should fix before merge |
271+
| SUGGESTION | ---- | Nice to have | Optional, defer to follow-up |
272+
273+
---
274+
275+
## Templates
276+
277+
- `templates/test-review-report.md` -- Full review report
278+
- `templates/incremental-test-report.md` -- PR incremental report
279+
280+
---
281+
282+
## Scripts
283+
284+
- `scripts/detect-test-changes.sh` -- Detect and classify changed test files
285+
286+
---
287+
288+
## References
289+
290+
- `references/smoke-test-patterns.md` -- Smoke test code examples
291+
- `references/integration-test-patterns.md` -- Integration test code examples
292+
293+
---
294+
295+
## Remember
296+
297+
1. **Every rule must cite a source file.** Do not invent rules -- every standard comes from the DataHub codebase.
298+
2. **Connector tests are out of scope.** Use `detect-test-changes.sh` to filter them.
299+
3. **Be conservative with BLOCKERs.** Only flag when a standard is clearly violated.
300+
4. **Acknowledge good patterns.** Note when tests follow standards well.
301+
5. **CI output must be deterministic.** Same input -> same verdict.

0 commit comments

Comments
 (0)