(pinot) branch master updated: Add review rule for shared integration test clusters (#18362)

[xi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git


The following commit(s) were added to refs/heads/master by this push:
     new aa99b36f54f Add review rule for shared integration test clusters (#18362)
aa99b36f54f is described below

commit aa99b36f54fb064e6ec2b1279423aaf40de927b4
Author: Xiang Fu <xi...@apache.org>
AuthorDate: Tue Apr 28 22:41:02 2026 -0700

    Add review rule for shared integration test clusters (#18362)
---
 .claude/skills/review-testing/SKILL.md | 13 +++++++----
 kb/code-review-principles.md           | 41 +++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/.claude/skills/review-testing/SKILL.md b/.claude/skills/review-testing/SKILL.md
index ed871ecb215..5e0782decd8 100644
--- a/.claude/skills/review-testing/SKILL.md
+++ b/.claude/skills/review-testing/SKILL.md
@@ -1,6 +1,6 @@
 ---
 name: review-testing
-description: Review Apache Pinot diffs for test coverage and test quality — positive + negative cases, real dictionaries vs mocks, rolling-upgrade / mixed-version tests, null-handling toggle coverage, exhaustive type coverage for aggregators/operators, integration-test base-class choice (prefer `CustomDataQueryClusterIntegrationTest` unless cluster-level setup differs), assertion quality, and regression tests that reproduce the bug. Trigger keywords — Test, TestNG, JUnit, Mockito, mock,  [...]
+description: Review Apache Pinot diffs for test coverage and test quality — positive + negative cases, real dictionaries vs mocks, rolling-upgrade / mixed-version tests, null-handling toggle coverage, exhaustive type coverage for aggregators/operators, integration-test base-class choice (reject standalone clusters unless special setup is needed; prefer `CustomDataQueryClusterIntegrationTest`), assertion quality, and regression tests that reproduce the bug. Trigger keywords — Test, TestNG [...]
 domain: kb/code-review-principles.md#6-testing-strategies
 triggers:
   - diff adds or modifies any src/test/** file
@@ -29,7 +29,12 @@ Severity:
   - Missing `@Test(dataProvider=...)` when the production code has a type-dispatch switch — a single type is insufficient coverage.
   - Timing assumptions: `Thread.sleep`, `System.currentTimeMillis()` in assertions → flakiness.
 - Integration tests: new REST / Thrift / wire-format changes need a `*IntegrationTest` case; check `pinot-integration-tests/`.
-- Integration-test base class: any new test under `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` should extend `CustomDataQueryClusterIntegrationTest`. A new test that extends `BaseClusterIntegrationTest` (or spins up its own controller/broker/server) directly is a red flag unless the change requires a non-default cluster topology or component config.
+- Integration-test base class: reject new standalone Pinot integration test classes that use `BaseClusterIntegrationTest`
+  or spin up their own controller/broker/server when no special cluster setup is required. For ordinary schema/data/query
+  behavior, require reusing an existing `CustomDataQueryClusterIntegrationTest` test or adding a focused subclass under
+  `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` with
+  `@Test(suiteName = "CustomClusterIntegrationTest")`, so it is included by
+  `pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml`.
 
 ## 2. Deep analysis
 
@@ -43,7 +48,7 @@ Severity:
 
 ### Core-functionality + integration-test base-class selection
 
-Every non-trivial change must exercise the **core functionality** it introduces. For query-semantics changes (new function, index type, aggregator, transform, SQL construct, stored-type behavior) that can be validated with ordinary table data and the default cluster topology, the integration test **must** extend `CustomDataQueryClusterIntegrationTest` — not a fresh `BaseClusterIntegrationTest` subclass.
+Every non-trivial change must exercise the **core functionality** it introduces. For query-semantics changes (new function, index type, aggregator, transform, SQL construct, stored-type behavior) that can be validated with ordinary table data and the default cluster topology, the integration test **must** reuse an existing `CustomDataQueryClusterIntegrationTest` test or add a focused subclass under `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` — not a [...]
 
 Rationale: `CustomDataQueryClusterIntegrationTest` shares one controller / broker / server / ZK across the whole test suite (`@BeforeSuite`, `_sharedClusterTestSuite`). Spinning up a second cluster per test costs ~30–60 s of ZK/Helix startup and inflates CI time linearly with test count. The custom base lets each test bring its own schema, data, and SQL assertions on top of the shared cluster. Nearly all feature validation (window functions, sketches, vector indexes, geo, JSON, timestamp [...]
 
@@ -53,7 +58,7 @@ A new test may extend `BaseClusterIntegrationTest` (or a specialized base) **onl
 - Different Helix / ZK layout or tenant isolation that can't be simulated with table-level config.
 - Realtime/streaming wiring that the custom base does not already provide, or lifecycle transitions that require cluster restart.
 
-Flag as **MAJOR**: a new test under `pinot-integration-tests/` whose class body only defines schema, data, and SQL assertions but extends `BaseClusterIntegrationTest` directly. The fix is to re-parent to `CustomDataQueryClusterIntegrationTest` and move into the `custom/` package. Require the PR to state, in the description, which of the four "only-when" conditions above applies if the fresh cluster is justified.
+Flag as **MAJOR / C6.10**: a new test under `pinot-integration-tests/` whose class body only defines schema, data, and SQL assertions but extends `BaseClusterIntegrationTest` directly or otherwise creates a dedicated cluster. The fix is to re-parent to `CustomDataQueryClusterIntegrationTest`, move it into the `custom/` package, annotate it with `@Test(suiteName = "CustomClusterIntegrationTest")`, and ensure it is covered by `custom-cluster-integration-test-suite.xml`. Require the PR to s [...]
 
 Independently, flag as **CRITICAL** a change to core functionality (new SQL function, new index type, behavior change of an existing operator/aggregator) that ships with only unit tests and no integration test of any kind — unit coverage alone doesn't prove the feature works end-to-end through planner → broker → server → segment.
 
diff --git a/kb/code-review-principles.md b/kb/code-review-principles.md
index cd2fe8f357b..ac34fbc1ba8 100644
--- a/kb/code-review-principles.md
+++ b/kb/code-review-principles.md
@@ -921,35 +921,56 @@ Investigate CI failures in newly added tests before approval.
 Don't blindly update expected values without understanding root cause.
 - Trigger: Any PR modifying test expected values
 
+**C6.10 — Reject unnecessary standalone integration-test clusters**
+New Pinot integration tests should not spin up a fresh controller / broker / server / ZK cluster unless the test needs
+special setup. Ordinary query/data behavior tests should reuse an existing `CustomDataQueryClusterIntegrationTest`
+subclass or add a focused subclass under
+`pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/`, with
+`@Test(suiteName = "CustomClusterIntegrationTest")`, so it is picked up by
+`pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml`. This keeps CI from paying
+another cluster startup cost for tests that only need custom schema, data, and SQL assertions.
+- Trigger: Any PR adding a new Pinot integration test class that uses `BaseClusterIntegrationTest`, starts Pinot
+  components directly, or creates a dedicated cluster without a distinct topology/component setup requirement
+
+```java
+// BAD: New class pays a fresh cluster startup just to test schema/data/query behavior
+public class JsonFunctionIntegrationTest extends BaseClusterIntegrationTest { ... }
+```
+```java
+// GOOD: Reuse the shared custom-data cluster suite
+@Test(suiteName = "CustomClusterIntegrationTest")
+public class JsonFunctionTest extends CustomDataQueryClusterIntegrationTest { ... }
+```
+
 ### MINOR
 
-**C6.10 — Core concurrent data structures require dedicated concurrent tests**
+**C6.11 — Core concurrent data structures require dedicated concurrent tests**
 - Trigger: Any PR adding or modifying concurrent data structures
 
-**C6.11 — Preserve test randomization**
+**C6.12 — Preserve test randomization**
 Don't fix random seeds without justification.
 - Trigger: Any PR fixing random seeds in tests
 
-**C6.12 — Tests should mimic real-world data states**
+**C6.13 — Tests should mimic real-world data states**
 Don't assume clean initial conditions.
 - Trigger: Any PR with test setup that assumes pristine state
 
-**C6.13 — Remove tests for invalid/unsupported request patterns**
+**C6.14 — Remove tests for invalid/unsupported request patterns**
 - Trigger: Any PR maintaining tests for deprecated functionality
 
-**C6.14 — Avoid expensive test setup changes**
+**C6.15 — Avoid expensive test setup changes**
 Don't change @BeforeClass to @BeforeMethod without justification.
 - Trigger: Any PR changing test lifecycle annotations
 
-**C6.15 — Binary compatibility baselines should use PR base commit**
+**C6.16 — Binary compatibility baselines should use PR base commit**
 Not latest master, to avoid false positives.
 - Trigger: Any PR modifying backward compatibility test configuration
 
-**C6.16 — Place tests in the correct test file**
+**C6.17 — Place tests in the correct test file**
 Tests for ClassB don't belong in ClassA's test file.
 - Trigger: Any PR adding tests to an unrelated test class
 
-**C6.17 — Maintain test scale unless explicitly justified**
+**C6.18 — Maintain test scale unless explicitly justified**
 Smaller tests may miss issues. Use assertions, not logging.
 - Trigger: Any PR reducing test data size or adding logging to tests
 
@@ -1184,7 +1205,7 @@ Approve the immediate fix while deferring broader design to separate threads.
 | 3. Code Architecture & Module Design | 5 | 11 | 8 | 24 |
 | 4. Performance & Efficiency | 2 | 16 | 4 | 22 |
 | 5. Correctness & Safety | 9 | 16 | 3 | 28 |
-| 6. Testing Strategies | 2 | 7 | 8 | 17 |
+| 6. Testing Strategies | 2 | 8 | 8 | 18 |
 | 7. Naming & API Design | 1 | 9 | 10 | 20 |
 | 8. Process & Scope | 2 | 10 | 8 | 20 |
-| **Total** | **32** | **83** | **48** | **163** |
+| **Total** | **32** | **84** | **48** | **164** |


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org