(shardingsphere) branch master updated: Add guardrails against testing through layers (#38993)

[zh...@apache.org]

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

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


The following commit(s) were added to refs/heads/master by this push:
     new d097ab4fb1d Add guardrails against testing through layers (#38993)
d097ab4fb1d is described below

commit d097ab4fb1d1cf863aee57d9d7e1c3422f0f4479
Author: Liang Zhang <zh...@apache.org>
AuthorDate: Sat Jul 4 18:53:48 2026 +0800

    Add guardrails against testing through layers (#38993)
    
    Define testing-through-layers in AGENTS.md and update the gen-ut skill
    to classify SUT-owned versus collaborator-owned behavior before branch
    mapping, mocking, parameterization, and test trimming.
---
 .codex/skills/gen-ut/SKILL.md | 10 ++++++++++
 AGENTS.md                     |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/.codex/skills/gen-ut/SKILL.md b/.codex/skills/gen-ut/SKILL.md
index 72fc98ff739..40364c14f40 100644
--- a/.codex/skills/gen-ut/SKILL.md
+++ b/.codex/skills/gen-ut/SKILL.md
@@ -42,6 +42,9 @@ Missing input handling:
 - `Gate reuse state`: persisted mapping from logical gate names (for example `target-test`, `coverage`, `rule-scan`) to the latest green digest for that gate.
 - `Latest green target-test digest`: compatibility alias for the `target-test` entry in `Gate reuse state`.
 - `Consolidated hard-gate scan`: one script execution that enforces `R8`, `R14`, and all file-content-based `R15` rules while still reporting results per rule.
+- `SUT-owned behavior`: branches, state changes, call choices, returned values, or exception handling that the target class itself decides.
+- `Collaborator-owned behavior`: rules a dependency computes, classifies, parses, loads, matches, or defaults, such as SPI, registry, factory, parser, loader, metadata option, driver option, dialect default, exception classification, message parsing, or SQLState parsing.
+- `Testing through layers`: a unit test for the target class whose inputs, branch triggers, or assertions encode collaborator-owned behavior instead of mocking the collaborator result consumed by the target class.
 
 Module resolution order:
 1. If the user explicitly provides modules, use them first.
@@ -74,6 +77,8 @@ Module resolution order:
 
 - `R4`: branch list and mapping
   - Before coding, `MUST` enumerate branches/paths of target public methods and build branch-to-test mappings.
+  - Branch mapping `MUST` classify each planned branch trigger and assertion as `SUT-owned behavior` or `Collaborator-owned behavior`.
+  - Branch mapping `MUST NOT` count collaborator-owned rules as target-class branches; when the target class only consumes a collaborator result, map the target branch from the consumed result, not from a real collaborator implementation that happens to produce it.
   - Branch mapping scope `MUST` exclude Lombok-generated methods without custom logic.
   - By default, one branch/path maps to one test method.
   - Whether to keep additional tests on the same branch is determined by `R13`.
@@ -98,6 +103,8 @@ Module resolution order:
   - If such tests are added without explicit request, they `MUST` be removed before completion.
   - If not instantiated via SPI, `MUST` record the reason before implementation.
   - Test dependencies `SHOULD` use Mockito mocks by default.
+  - When a target-class branch depends on collaborator-owned behavior, tests `MUST` mock the nearest stable collaborator boundary to return the needed result and `MUST NOT` choose a real collaborator implementation only to trigger the branch.
+  - Simple value objects, pure data fixtures, and explicitly scoped integration, contract, or E2E tests are exceptions to the collaborator-boundary mocking rule.
   - Reflection access `MUST` use `Plugins.getMemberAccessor()`, and field access only.
 
 - `R7`: related test class strategy
@@ -116,6 +123,7 @@ Module resolution order:
     - D. parameter sample count is at least 3;
     - E. parameterized test body does not require dispatch logic via `switch`.
   - "Declared assertion differences" means differences explicitly recorded in the delivery report.
+  - Scenarios whose differences mainly encode collaborator-owned behavior are not high-fit target-class parameterization candidates, even when the method and assertion skeletons are otherwise consistent.
   - If a candidate requires `switch` in a `@ParameterizedTest` body to distinguish argument rows, it is not high-fit and `MUST NOT` be refactored to parameterized form.
   - High-fit candidates `MUST` be refactored directly to parameterized form.
   - For high-fit candidates, a "do not recommend refactor" conclusion is allowed only when refactoring causes significant readability/diagnosability regression, and the exception `MUST` include a `Necessity reason tag` with concrete evidence.
@@ -161,6 +169,7 @@ Module resolution order:
   - Each retained item `MUST` carry a `KEEP:<id>:<reason>` tag and be recorded in the delivery report; items without tags are treated as redundant.
   - Each test method `MUST` provide unique value: cover a new branch/path, or add assertion differences.
   - If deleting a test method does not change line/branch coverage and has no assertion differences, `MUST` delete it.
+  - If a test method, mock setup, data row, or assertion only verifies collaborator-owned behavior, it `MUST` be removed from the target-class unit test; if collaborator coverage is missing, report the need for a separate collaborator-focused test scope instead of editing outside `R3`.
   - Unless scenario requires otherwise, `SHOULD` use Mockito default return values instead of extra stubs.
 
 - `R14`: boolean assertion hard gate
@@ -195,6 +204,7 @@ Module resolution order:
     - Use the baseline summary to identify current branch-miss lines, existing `R15` risks, and likely `R8-CANDIDATES` before editing.
     - `SHOULD` fix deterministic precheck warnings from the baseline summary before the first standalone target-test run; these warnings are advisory only and do not replace final `checkstyle` / `spotless` / hard-gate verification.
 5. Decide whether `R12` is triggered; if not, output `R4` branch mapping.
+   - Before outputting `R4`, record the target-class ownership boundary: `SUT owns`, `Collaborator owns`, and the mocked collaborator boundary used for each collaborator-owned result consumed by the target.
    - For parser / utility classes that return context or value objects, `SHOULD` align planned assertions with the returned object's public API before the first target-test run, to avoid internal-branch coverage assertions that do not match externally observable behavior.
 6. Execute `R8` parameterized optimization analysis, output `R8-CANDIDATES`, and apply required refactoring.
 7. Execute `R9` dead-code checks and record evidence.
diff --git a/AGENTS.md b/AGENTS.md
index b5cfbc1bc55..2d4ecff185f 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -121,6 +121,10 @@ This guide is written **for AI coding agents only**. Follow it literally; improv
 - **Test Naming Simplicity**: keep test names concise and scenario-focused (avoid “ReturnsXXX”/overly wordy or AI-like phrasing); describe the scenario directly.
 - **Coverage Discipline**: follow the dedicated coverage & branch checklist before coding when coverage targets are stated.
 - **Dedicated and scoped tests**: each public production method must be covered by dedicated test methods; each test method covers only one scenario and invokes the target public method at most once (repeat only when the same scenario needs extra assertions), and different branches/inputs belong in separate test methods.
+- **No testing through layers**: a unit test must not appear to test the current class under test while its inputs, branch triggers, or assertions encode collaborator implementation rules such as SPI, registry, factory, parser, loader, metadata option, driver option, dialect defaults, exception classification, or message/SQLState parsing.
+  Unit tests must distinguish behavior owned by the class under test from behavior owned by collaborators.
+  If a collaborator implementation can change while the class under test contract stays unchanged, the current class test must not fail; mock the nearest stable collaborator boundary and cover the collaborator rule in that collaborator's own focused tests.
+  Cross-layer verification belongs only in explicitly scoped integration, contract, or E2E tests.
 - **No interface-only tests**: do not create unit tests for interfaces themselves; cover behavior through concrete implementations instead, and avoid dedicated test classes for pure contracts or SPI interfaces.
 - **Parameterized tests naming**: all parameterized tests must set an explicit `name` and use the `"{0}"` template for display names.
 - **Mocking Rule**: default to mocks; see Mocking & SPI Guidance for static/constructor mocking and spy avoidance details.