From c31bd85bd5990dec29f4c32732f1ce20061ab20f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Gonz=C3=A1lez=20L=C3=B3pez?= Date: Tue, 20 Oct 2026 19:22:35 +0200 Subject: [PATCH] doc: change: require protocol test tasks for each planned repair Before this commit, an implementation plan can group several future commits into one step. It need not assign concrete protocol tests or a reviewer document to each protocol repair. After this commit, each future commit has a separate entry with dependencies, affected components, expected behavior, and verification. Each planned protocol repair or feature assigns explicit test tasks and a reviewer Markdown document. The entry connects the current gap, test assertions, and proposed code correction. The reviewer document explains each test and includes the diagrams that its scenarios need. The guide provides templates and a hypothetical IPv4 TTL example. Current content requirements apply independently of the draft approval process. The change affects contributor requirements only. It changes no simulation code, configuration, or recorded test expectation. Change: doc.project | behavior.change | - --- doc/project/README.md | 2 +- doc/project/audit/seal-list.md | 4 +- doc/project/enforcement/README.md | 2 + doc/project/enforcement/checklist/general.md | 2 + doc/project/guide/contribute-a-change.md | 2 + doc/project/guide/review-a-pull-request.md | 2 +- .../guide/write-an-implementation-plan.md | 140 +++++++++++++++++- doc/project/rule/planning.md | 8 +- doc/project/rule/pull-request.md | 29 +++- doc/project/rule/testing.md | 38 ++++- 10 files changed, 213 insertions(+), 16 deletions(-) diff --git a/doc/project/README.md b/doc/project/README.md index f0dd5c41a8c..2e3ef5f93a6 100644 --- a/doc/project/README.md +++ b/doc/project/README.md @@ -46,7 +46,7 @@ history/ How did the current state come to be? 7. **Asking "why does INET not do X?"** — [design/rejected-designs.md](design/rejected-designs.md). 8. **About to derive protocol tests from a standard** — [guide/derive-tests-from-a-standard.md](guide/derive-tests-from-a-standard.md). 9. **Writing in this folder** — [rule/documentation.md](rule/documentation.md). -10. **About to write or review an implementation plan** — [guide/write-an-implementation-plan.md](guide/write-an-implementation-plan.md) and its [plan rules](rule/planning.md). Both documents are drafts. +10. **About to write or review an implementation plan** — [guide/write-an-implementation-plan.md](guide/write-an-implementation-plan.md) and its [plan rules](rule/planning.md). Both documents are drafts. Current requirements for [future commits](rule/pull-request.md#pr-series-plan) and [protocol tests with reviewer documents](rule/testing.md#tr-plan-protocol) apply independently of draft approval. ## Every document diff --git a/doc/project/audit/seal-list.md b/doc/project/audit/seal-list.md index 8421c088ed0..6ba3dee60fc 100644 --- a/doc/project/audit/seal-list.md +++ b/doc/project/audit/seal-list.md @@ -64,11 +64,11 @@ A whole-document seal is declared in the document's own header, as `**Seal:** wh | `rule/documentation.md` | whole | 0 | 0 | 1 | | `rule/naming.md` | by rule | 0 | 0 | 23 | | `rule/planning.md` | by rule | 0 | 0 | 8 | -| `rule/pull-request.md` | by rule | 0 | 0 | 26 | +| `rule/pull-request.md` | by rule | 0 | 0 | 27 | | `rule/quality.md` | by rule | 0 | 0 | 15 | | `rule/release.md` | by rule | 0 | 0 | 11 | | `rule/sealing.md` | whole | 0 | 0 | 1 | -| `rule/testing.md` | by rule | 0 | 0 | 14 | +| `rule/testing.md` | by rule | 0 | 0 | 15 | Nothing is closed yet, and that is the honest state: no document in this set has been audited against [documentation.md](../rule/documentation.md). The requirements are the one document that carries flags on every unit, because it declares `by requirement, complete` — its 30 requirements are all ⬜, which reads as *considered, and deliberately still open*. diff --git a/doc/project/enforcement/README.md b/doc/project/enforcement/README.md index 787e29e39ff..dc897723600 100644 --- a/doc/project/enforcement/README.md +++ b/doc/project/enforcement/README.md @@ -26,6 +26,8 @@ The draft [plan procedure](../guide/write-an-implementation-plan.md) includes a This is a manual review procedure. No automated gate checks plan approval. A successful document check establishes neither design quality nor human approval. +Current [series requirements](../rule/pull-request.md#pr-series-plan) and [protocol test requirements](../rule/testing.md#tr-plan-protocol) apply to plan content independently of draft approval. T4 review checks future commit entries, explicit test tasks, and reviewer documents with the necessary diagrams. No automated gate verifies these explanations or their technical correctness. + ## The gate inventory One row per check that exists. The table lists **machinery, not intent**, so it cannot drift from the rules: a row disappears when its check is deleted, and coverage is read from the side that can be measured. diff --git a/doc/project/enforcement/checklist/general.md b/doc/project/enforcement/checklist/general.md index 44d0e8a8d53..2f685e125f8 100644 --- a/doc/project/enforcement/checklist/general.md +++ b/doc/project/enforcement/checklist/general.md @@ -6,6 +6,8 @@ The tier-4 gate from [enforcement/README.md](../README.md). It enforces the **se For a proposed implementation plan, use the draft [plan review checklist](../../guide/write-an-implementation-plan.md#review-checklist). Its [plan rules](../../rule/planning.md) state when that procedure applies. +Check current [commit series requirements](../../rule/pull-request.md#pr-series-plan) and [protocol test requirements](../../rule/testing.md#tr-plan-protocol) independently of draft approval. Flag absent commit entries or missing protocol test and reviewer document tasks for applicable commits. For implementation, check that the assigned commits contain the required artifacts and explanations. Check necessary diagrams against the scenarios and assertions. + ## How to run Input: the change under review (a diff, commit, range, branch, pull request, staged diff, or working tree). For **each** checklist item that can be evaluated, output one of: diff --git a/doc/project/guide/contribute-a-change.md b/doc/project/guide/contribute-a-change.md index 3bd7a56ccf4..52e34e7c31a 100644 --- a/doc/project/guide/contribute-a-change.md +++ b/doc/project/guide/contribute-a-change.md @@ -22,6 +22,8 @@ Apply [QR-DESIGN-MINIMAL](../rule/quality.md#qr-design-minimal) to design and co For a task that calls for a plan, use [write-an-implementation-plan.md](write-an-implementation-plan.md) before implementation. The plan rules remain a draft. Their [approval rule](../rule/planning.md#plr-approval) applies when the maintainer accepts the policy or explicitly selects it for the task. The guide covers preparation, review, and approval in that order. +Apply the current [series requirement](../rule/pull-request.md#pr-series-plan) to every future commit in that plan. Apply [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol) to explicit protocol test tasks and reviewer documents for applicable commits. These content requirements do not activate the draft approval process. + Establish who owns which state before you edit. Then make the smallest change that satisfies the contracts, the ownership, the observability, the configuration, the determinism and the test rules. ## 5. The mechanisms that already exist diff --git a/doc/project/guide/review-a-pull-request.md b/doc/project/guide/review-a-pull-request.md index 7eb0e9bc09f..8feb0e34333 100644 --- a/doc/project/guide/review-a-pull-request.md +++ b/doc/project/guide/review-a-pull-request.md @@ -70,7 +70,7 @@ It checks the mechanical half of [rule/classification.md](../rule/classification **A trailer is a claim, and the gate checks only the half that is derivable.** Three things stay for you to judge: - **Is the depth honest?** A commit that says `refactor` and rewrites a loop condition is a behavior change wearing the wrong word. Read the diff of every `refactor`, `name` and `comment` commit — there are few, and the gate has already listed them. -- **Is the obligation discharged, and where?** The two obligations have **different units**, and reading them the same way produces a false finding. [TR-SHIP-WITH](../rule/testing.md#tr-ship-with) asks for the test in the same *pull request*, so a batch of tests at the end of a series satisfies the letter of it. [PR-SPLIT-BASELINE](../rule/pull-request.md#pr-split-baseline) asks for the baseline in the same *commit*, so a batch at the end is a violation. A long gap between behavior and test is still worth a **note**: the rule's own reason is that a test written later is written against the code as it turned out. +- **Is the obligation discharged, and where?** Check the unit required by each applicable rule. [TR-SHIP-WITH](../rule/testing.md#tr-ship-with) requires new behavior and its test in the same pull request. For planned protocol repairs and features, apply the stricter [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol). It requires tests and a reviewer document with the behavior commit, except for permitted earlier test commits. Tests deferred to a later commit do not satisfy that rule. [PR-SPLIT-BASELINE](../rule/pull-request.md#pr-split-baseline) requires a baseline update in the commit that causes it. - **Is a `?` still a `?`** A commit that says `?` for its baselines is honest and unfinished. It is a `PARTIAL`, not a `PASS`, and the report says what run would settle it. **Do not report a missing trailer on a commit that predates the rule.** The gate gives a note, not a violation, for a missing `Change:` line on a commit authored before 2026-09-11, the day `classification.md` entered the tree. A rebase keeps the author date, so an old commit on a new branch still gets the note. diff --git a/doc/project/guide/write-an-implementation-plan.md b/doc/project/guide/write-an-implementation-plan.md index 25860f1fb79..2755f2133bc 100644 --- a/doc/project/guide/write-an-implementation-plan.md +++ b/doc/project/guide/write-an-implementation-plan.md @@ -4,6 +4,8 @@ Use this procedure for a task that calls for an implementation plan. Its proposed requirements are in [planning.md](../rule/planning.md). That document states when the draft approval process applies. +The current [commit series rule](../rule/pull-request.md#pr-series-plan) and [protocol test rule](../rule/testing.md#tr-plan-protocol) apply to plan content. Their requirements do not depend on acceptance of the draft approval process. + ## 1. Establish the current behavior Follow the discovery route in [contribute-a-change.md](contribute-a-change.md). Record the checkout revision and relevant local changes. Trace the behavior through its actual caller and effective configuration. Separate observed facts from assumptions under [PLR-GROUNDED](../rule/planning.md#plr-grounded). @@ -59,13 +61,17 @@ Use only the rows that apply. Each linked document owns its technical requiremen ## 4. Define the steps and verification -Order the steps by their dependencies. Map each step to one commit or a short series under [PLR-PROPORTION](../rule/planning.md#plr-proportion). A commit may cover multiple steps when the plan explains why those changes belong together. For example, a hypothetical interface change and its implementation updates need one commit to preserve a buildable tree. Use prose or this table to explain each coherent change: +List every future commit in dependency order under [PR-SERIES-PLAN](../rule/pull-request.md#pr-series-plan). Give each entry a stable identifier, such as `C1`, and a proposed subject. Expand steps that contain several commits. Explain any commit that covers several steps. For example, a hypothetical interface change and its implementation updates need one commit to preserve a buildable tree. + +Explain each commit once. Use compact prose or the [commit entry template](#commit-entry-template), according to the detail that the reviewer needs. For a longer series, this optional index can link to the detailed entries: -| Purpose | Files and responsible component | Proposed change | Expected behavior | Verification | -| --- | --- | --- | --- | --- | -| `` | `` | `` | `` | `` | +| Commit and proposed subject | Dependencies | Detailed entry | +| --- | --- | --- | +| `C1: ` | `` | `` | -Apply [PLR-VERIFICATION](../rule/planning.md#plr-verification) to the verification column. Identify the relevant boundaries and failure paths. For a defect, explain how the proposed check exposes the original failure. Use [run-the-gates.md](run-the-gates.md) for the applicable build and check procedure. +Apply [PLR-VERIFICATION](../rule/planning.md#plr-verification) to every entry. For each protocol repair or feature, include explicit tasks to write the tests and reviewer document under [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol). Name existing tests that expose the gap. State when none exist. Specify how each new or extended case exposes the gap and how the code will change its result. + +Compare later commits with their proposed parents. Label effects of earlier planned commits as assumptions. Explain why protocol tests or reviewer documents do not apply to other commits. Keep their appropriate verification explicit. Use [run-the-gates.md](run-the-gates.md) for the applicable build and check procedure. ## 5. Review the plan @@ -91,6 +97,9 @@ Use the plan's existing explanation as the answer to each applicable row. Add ev | Why does each significant addition need to exist? | [PLR-DESIGN](../rule/planning.md#plr-design) | | Which requirement and reachable failure justify each safeguard? | [QR-DESIGN-MINIMAL](../rule/quality.md#qr-design-minimal) | | Which check reaches each claimed behavior, and what remains unverified? | [PLR-VERIFICATION](../rule/planning.md#plr-verification) | +| Does the plan enumerate every future commit, with dependencies and behavior relative to its proposed parent? | [PR-SERIES-PLAN](../rule/pull-request.md#pr-series-plan) | +| Does each protocol commit explain the gap, tests to write, expected results, and code correction? | [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol) | +| Does each applicable commit include an explicit task for its reviewer `.md`, with its path, test explanations, and necessary diagrams? | [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol) | | Which plan revision and permissions does the approval cover? | [PLR-APPROVAL](../rule/planning.md#plr-approval) | | Does a revision change an approved decision? | [PLR-REVISION](../rule/planning.md#plr-revision) | | Is the detail sufficient for the decision and proportionate to the risk? | [PLR-PROPORTION](../rule/planning.md#plr-proportion) | @@ -117,6 +126,8 @@ This hypothetical example assumes that an edit removed timer cancellation from ` **Change and preserved behavior:** Restore cancellation through the timer owner's existing API in `handleStopOperation()`. Retain the timer object for restart. Preserve the existing socket closure, delayed stop completion, configuration, and cumulative counters. The existing cancellation mechanism prevents the old timeout; no new lifecycle flag or generation counter is necessary. One commit contains the handler repair and its focused regression coverage. +**Commit series:** `C1: UdpBasicApp: prevent send timeouts after shutdown` has no dependencies. It changes the application stop handler and writes the module test below. This hypothetical change repairs an application lifecycle contract, without a protocol exchange change. Protocol tests and a protocol reviewer document therefore do not apply. The module test supplies direct verification. + **Verification:** Derive a proposed `tests/module/udpapp_lifecycle_timer_restart.test` from [udpapp_lifecycle_6.test](../../../tests/module/udpapp_lifecycle_6.test). Choose a send interval that places the pending timeout after shutdown completes. Stop before that timeout, away from its exact timestamp. Advance beyond that timeout while the client remains down. Check that no send callback occurs during that interval. Restart the client. Check that it sends again and releases the timer at final teardown. @@ -129,6 +140,66 @@ inet_run_module_tests -m debug -f 'udpapp_lifecycle_(6|timer_restart)\.test' Confirm that the selector includes both cases after the new test exists. The existing case covers shutdown and teardown; it does not establish restart behavior. The hypothetical edit should fail when the old timeout reaches the stopped application. The repair should suppress that timeout and permit later sends after restart. The new variant's results remain unverified until implementation and execution. +### Protocol worked example + +This hypothetical example assumes that an edit removes the TTL decrement from IPv4 unicast forwarding. TTL means time to live. The current [IPv4 source](../../../src/inet/networklayer/ipv4/Ipv4.cc) contains that decrement in `prepareForForwarding()`. The example reports no current defect or executed test result. Its test extension and reviewer document are proposed changes. + +**Checkout and approval:** ``. + +**C1: Ipv4: decrease TTL before unicast forwarding.** This commit has no dependencies. It restores the decrement in `src/inet/networklayer/ipv4/Ipv4.cc`. It also extends the existing TTL-2 test and writes the reviewer document below. All three artifacts belong to C1. + +**Protocol gap and correction:** The [TTL requirement](../evidence/standard/rfc791/catalog.md#rfc791-ttl-1) requires a gateway to decrease the TTL. Host A sends one datagram with TTL 2 through router R to host B. Under the hypothetical edit, R forwards the datagram with TTL 2. Restore the decrement before the later output path checks the TTL. R will forward this datagram with TTL 1. The correction uses the existing header update path and adds no state. + +**Existing tests:** These fixtures already cover the decrement and its boundary conditions: + +- [Rfc791TtlDecrement.test](../../../tests/protocol/ipv4/Rfc791TtlDecrement.test) sends with TTL 32. It requires TTL 31 and a different header checksum at R's output. It also requires TTL 31 at B's input. +- [Rfc1122TtlOneAtDestination.test](../../../tests/protocol/ipv4/Rfc1122TtlOneAtDestination.test) sends with TTL 2 and captures the datagram's identification. It requires that identification and TTL 1 at R's output. It also checks delivery to B's UDP module and no ICMP Time Exceeded response. +- [Rfc791TtlExpiry.test](../../../tests/protocol/ipv4/Rfc791TtlExpiry.test) sends with TTL 1. It requires R to drop the identified datagram and return ICMP Time Exceeded. It also checks that the datagram does not appear at R's output during the final observation interval. + +Source inspection indicates that the hypothetical edit will fail all three cases. The first two cases cannot observe their required output TTL. The expiry case cannot observe the required drop because the output path still sees TTL 1. These are expected failures, without execution evidence. + +**Task: Extend the protocol test.** Extend `tests/protocol/ipv4/Rfc1122TtlOneAtDestination.test` with a checksum comparison at the existing TTL-2 boundary. Retain `hostA -- router -- hostB`, static ARP, `seed-set = 0`, and the one-second simulation limit. Set `**.checksumMode = "computed"` as in the decrement fixture. Capture the header checksum beside the existing identification capture at A's output. + +Require a different checksum in the existing assertion for identification and TTL 1 at R's output. Retain the UDP delivery assertion and the absence check for ICMP Time Exceeded. This extension adds checksum evidence at TTL 2; the TTL boundary already has coverage. Checksum inequality detects an unchanged checksum but does not establish the correct checksum value. The correction should pass all three cases, including the extended case. + +**Task: Write the reviewer document.** Add `tests/protocol/ipv4/forwarded-ttl-boundary.md` in C1. Associate it with the plan, C1's subject, and the three test paths. Explain each test's setup, observations, assertions, expected results, and limits. Use the two diagrams below for the TTL-2 scenario. Add an expiry diagram for the distinct drop and ICMP response sequence. The diagrams describe expected behavior, without executed evidence. + +Before the correction, the output assertion rejects the unchanged TTL: + +```mermaid +sequenceDiagram + participant A as Host A + participant R as Router R + participant B as Host B + A->>R: Datagram with TTL 2 + Note over R: Hypothetical defect: no decrement + R->>B: Same datagram with TTL 2 + Note over R,B: Output assertion fails: expected TTL 1 +``` + +After the correction, the router decreases the TTL before output: + +```mermaid +sequenceDiagram + participant A as Host A + participant R as Router R + participant B as Host B + A->>R: Datagram with TTL 2 + Note over R: Decrease TTL from 2 to 1; update checksum + R->>B: Same datagram with TTL 1 + Note over R: Output assertion checks TTL, identification, and changed checksum + Note over B: UDP receives the datagram + Note over A: No ICMP Time Exceeded response +``` + +**Verification and limits:** Follow [run-the-gates.md](run-the-gates.md) for a fresh debug build before execution. Run this command from the checkout root after the proposed changes: + +```sh +inet_run_protocol_tests -p inet -m debug -w '^tests/protocol/ipv4$' -f '^(Rfc791Ttl(Decrement|Expiry)|Rfc1122TtlOneAtDestination)\.test$' +``` + +Confirm selection of all three files. Record their actual outcomes under [TR-FOCUSED-EVIDENCE](../rule/testing.md#tr-focused-evidence). The cases cover unicast TTL decrement, the TTL-1 expiry response, and delivery to B's UDP module after the TTL-2 boundary. They do not establish multicast behavior, fragmentation, application delivery, or complete checksum correctness. The proposed extension has no executed result. Check diagram rendering before the reviewer document is complete. + ### Expanded template ```markdown @@ -153,12 +224,65 @@ Approval: ## 5. Affected components and compatibility -## 6. Implementation steps and dependencies - +## 6. Future commits and dependencies + + ## 7. Verification methods and expected results - + ## 8. Risks, unresolved decisions, and approval scope ``` + +### Commit entry template + +Use this template when its fields help explain the commit. Compact prose can supply the same information. Retain explicit test and document tasks for each applicable protocol commit. + +```markdown +### C: + +Dependencies: . +Purpose and scope: . + +**Current protocol bug or gap:** + +**Proposed correction:** + +**Existing tests:** + +**Task: Write or extend protocol tests.** + +**Task: Write the reviewer document.** + +**Verification:** +``` + +### Reviewer document template + +The plan names this future artifact and assigns its creation to the applicable commit. Implementation writes the document under [TR-PLAN-PROTOCOL](../rule/testing.md#tr-plan-protocol). It explains that commit's protocol tests to human reviewers. It must make sense without the conversation or test source. + +```markdown +# : tests for + +Plan and commit: . +Included tests: . + +## Protocol requirement and current gap + + +## Code correction and protocol result + + +## Test scenarios and assertions + + +## Protocol diagrams + + +## Execution and evidence + + +## Coverage limits + +``` diff --git a/doc/project/rule/planning.md b/doc/project/rule/planning.md index fa33936e958..4431ebb8116 100644 --- a/doc/project/rule/planning.md +++ b/doc/project/rule/planning.md @@ -6,6 +6,8 @@ This document proposes requirements for tasks that call for an implementation pl These rules govern the plan. The linked technical rules govern the implementation. The procedure and template are in [write-an-implementation-plan.md](../guide/write-an-implementation-plan.md). +The current [commit series rule](pull-request.md#pr-series-plan) requires explicit future commits. The current [protocol test rule](testing.md#tr-plan-protocol) requires test tasks and reviewer documents for applicable commits. Those content requirements apply independently of this draft approval process. + ## Index | Rule | Statement | @@ -75,6 +77,8 @@ Map each acceptance criterion to evidence under [TR-CAT-MATCH](testing.md#tr-cat Distinguish existing checks from checks that the implementation must add. Verify existing commands and selectors in the checkout. Label proposed commands whose test targets do not yet exist. State what each check cannot establish. +For each applicable commit, define the protocol tests and reviewer document under [TR-PLAN-PROTOCOL](testing.md#tr-plan-protocol). Include explicit tasks to write both artifacts. Explain how the code change will correct the observed or source-grounded protocol gap. Identify absent tests directly. + Separate planned checks from observed results. A plan does not need successful results from code that does not yet exist. An observed result needs the reproducible evidence required by the test rules. Identify expected changes to recorded expectations under [TR-BASELINE-DELIBERATE](testing.md#tr-baseline-deliberate). *Enforced at T4 — agent review through the [plan review checklist](../guide/write-an-implementation-plan.md#review-checklist).* @@ -113,6 +117,8 @@ Explain difficult decisions and material risks in detail. Keep routine edits bri Use an existing plan explanation to answer a review question when it already supplies the required evidence. Do not duplicate that explanation in a separate checklist report. Choose the [compact example or expanded template](../guide/write-an-implementation-plan.md#plan-template) according to the interactions that need explanation. -Order implementation steps by their dependencies under [PR-SERIES-ORDER](pull-request.md#pr-series-order). Give each step a coherent purpose and a verification method. Each step becomes one commit, or a short series of commits, under [PR-SPLIT-ONE-CHANGE](pull-request.md#pr-split-one-change). A commit does not span two steps unless the plan says why. When the implementation shows that a step divides further, or that two steps cannot be divided, update the plan; [PLR-REVISION](#plr-revision) decides whether that needs approval. Avoid empty sections and repeated policy text. +Order implementation steps by their dependencies under [PR-SERIES-ORDER](pull-request.md#pr-series-order). Enumerate every future commit under [PR-SERIES-PLAN](pull-request.md#pr-series-plan), even when several commits serve one step. Give each commit a coherent purpose and verification method. Explain any commit that spans two steps. + +Update the entries when implementation changes their boundaries. [PLR-REVISION](#plr-revision) decides whether that revision needs approval. Avoid empty sections and repeated policy text. *Enforced at T4 — agent review through the [plan review checklist](../guide/write-an-implementation-plan.md#review-checklist).* diff --git a/doc/project/rule/pull-request.md b/doc/project/rule/pull-request.md index b5fc2f2fba3..df1385aad2d 100644 --- a/doc/project/rule/pull-request.md +++ b/doc/project/rule/pull-request.md @@ -48,6 +48,7 @@ Every rule in document order. The identifier links to the rule; the statement is | [PR-SERIES-BUILDS](#pr-series-builds) | Every commit builds and passes its tests | | [PR-SERIES-ORDER](#pr-series-order) | Prerequisites first, no fixup commits | | [PR-SERIES-LINEAR](#pr-series-linear) | Rebase the topic branch; do not merge into it | +| [PR-SERIES-PLAN](#pr-series-plan) | An implementation plan defines each future commit and its verification. | **Commit messages (PR-MSG)** @@ -224,6 +225,22 @@ Keep the series linear on top of the target branch. Do not merge the target bran A merge commit inside the series mixes other developers' changes into the diff of the pull request, and a bisect crosses into unrelated history. How the finished branch enters the target branch afterwards is the maintainer's decision; the branch you submit stays linear. +### PR-SERIES-PLAN + +**An implementation plan defines each future commit and its verification.** + +List the future commits in dependency order. Give each commit a stable plan identifier, proposed subject, purpose, dependencies, responsible component, affected files, expected behavior, and verification. Expand any step that contains several commits into separate commit entries. Explain boundaries under [PR-SPLIT-ONE-CHANGE](#pr-split-one-change). + +For each protocol repair or feature, explain the current bug or missing capability through a concrete scenario. Cite the protocol requirement and current source or observed evidence. State the initial condition, trigger, incorrect or absent result, and required result. Explain how the proposed component action will produce that result. State uncertainty when the cause remains unknown. + +Compare each proposed commit with its proposed parent. Separate verified current behavior from assumed effects of earlier planned commits. Use [TR-PLAN-PROTOCOL](testing.md#tr-plan-protocol) for explicit test tasks and the associated reviewer document. For other commits, explain their structural purpose and verification. State why protocol tests and a protocol reviewer document do not apply. Do not invent a protocol defect for a refactor or documentation change. + +For example, a hypothetical series first adds a shared queue operation and next repairs protocol service order through that operation. The first entry explains the generic contract and its direct tests. The second entry explains the protocol gap, changed caller behavior, protocol tests, and reviewer document. The plan identifies the dependency between those entries. + +Keep the commit entries current when boundaries or dependencies change. This content requirement applies whenever a task calls for an implementation plan. It does not activate the separate draft [plan approval process](planning.md#plr-approval). + +*Enforced at T4 — review of the plan and its mapping to the commit series.* + ## Commit messages (PR-MSG) ### PR-MSG-SUBJECT @@ -323,9 +340,17 @@ A commit that repairs a defect carries, in its body, the way to see the defect h 1. **Steps.** The configuration, the scenario, and what goes wrong — *"run `examples/inet/tcpwindowscale` with `sackSupport=true`; the sender stalls at t=2.4 s because `nextSeg` rule (2) underflows"*. A few lines, and it is enough for most defects. 2. **A standalone regression test**, committed beside the repair. -**The test is justified only when the defect is serious enough and likely to bite again.** Most are not. A defect earns a regression test when it sits on a path many things cross, when a future refactor could reintroduce it without noticing, or when it came from a misreading of a standard that the next reader could repeat. A one-off typo in a log message earns steps and nothing more. +**Outside planned protocol repairs, select a regression test according to the defect's risk.** A test is justified when the defect is serious enough and likely to recur. These conditions justify a regression test: + +- Many components use the affected path. +- A later refactor could reintroduce the defect without detection. +- A repeated misinterpretation of the standard could cause the defect again. + +A one-off typo in a log message needs reproduction steps alone. + +For planned protocol repairs, [TR-PLAN-PROTOCOL](testing.md#tr-plan-protocol) requires explicit protocol tests and a reviewer document. Reproduction steps alone do not satisfy that specific requirement. -**Why the steps are required even when the test is not.** A fix is not new behavior, so [TR-SHIP-WITH](testing.md#tr-ship-with) does not reach it, and nothing else in the rule set asks a fix for evidence. Without the steps a reviewer must take the defect on trust, and a later reader who suspects a regression in the same area has no way to tell whether it is the old defect returning. +**Every fix needs reproducible evidence.** For other fixes without a regression test, the reproduction steps supply the evidence. They let a reviewer check the defect and compare a later failure with the original scenario. Write the symptom in the words a future reader will search for: the error message, the wrong packet, the failed assertion. That is [PR-MSG-WHY](#pr-msg-why) applied to a fix, and this rule says the searchable symptom is not optional. diff --git a/doc/project/rule/testing.md b/doc/project/rule/testing.md index 6420aa38103..05dbaa770bd 100644 --- a/doc/project/rule/testing.md +++ b/doc/project/rule/testing.md @@ -19,6 +19,7 @@ Every test rule in document order. | [TR-CAT-MATCH](#tr-cat-match) | The test category matches the kind of claim the change makes | | [TR-FOCUSED-EVIDENCE](#tr-focused-evidence) | Validation runs the directly related cases and records reproducible evidence | | [TR-SHIP-WITH](#tr-ship-with) | New behavior ships with its test, in the same pull request | +| [TR-PLAN-PROTOCOL](#tr-plan-protocol) | Planned protocol changes include explicit protocol test tasks and a reviewer document for each applicable commit. | | [TR-FP-NOT-ENOUGH](#tr-fp-not-enough) | A fingerprint is never the only test of new behavior | | [TR-VALIDATE-EXTERNAL](#tr-validate-external) | A claim about the real world is checked against something outside INET | @@ -67,7 +68,9 @@ Every test rule in document order. The category identifies suitable infrastructure, not proof by itself. Verify that the fixture reaches the claimed production path and that its observations establish the expected result. A module fixture can check integration when it connects the required production components. A recorded scalar cannot establish frame encoding unless it directly measures the required byte-level property. A distributional claim needs appropriate samples and a statistical check, regardless of the directory that contains the test. The limits of statistical baseline evidence are in [test-anatomy.md](../design/test-anatomy.md#the-recorded-expectations). -For example, a hypothetical module test drives a production sender and checks the emitted header's receiver address. That test can establish the address claim. Its directory does not require a second protocol test with the same assertion. +For example, a hypothetical module test drives a production sender and checks the emitted header's receiver address. That test can establish the address claim. The category guidance alone does not require another test with the same assertion. + +For planned protocol repairs and features, [TR-PLAN-PROTOCOL](#tr-plan-protocol) adds a specific requirement for protocol tests. Sufficient module coverage does not remove that requirement. Retain useful module tests as additional evidence. *Enforced at T4 — agent review: does the test type match the claim?* @@ -89,6 +92,39 @@ Not in a follow-up, and not in an issue. A test written later is written against *Enforced at T4 — agent review; T2 once coverage is gated.* +### TR-PLAN-PROTOCOL + +**Planned protocol changes include explicit protocol test tasks and a reviewer document for each applicable commit.** + +Apply this rule to each planned commit that repairs a protocol bug or adds a missing protocol capability. The [commit series](pull-request.md#pr-series-plan) identifies those commits. The plan must include explicit tasks to write or extend protocol tests and to write the associated `.md` reviewer document. A general instruction to add regression coverage is insufficient. + +For each applicable commit, the plan must specify these items: + +1. The protocol requirement, current bug or gap, and source or observed evidence. +2. The existing tests that expose the gap, or an explicit statement that no such test exists. +3. The exact test paths and cases to add or extend, with existing and proposed cases clearly distinguished. +4. Each test's initial state, configuration, stimulus, production path, protocol observation, and assertion. +5. The expected failure or missing behavior before the change, the expected result after it, and the reason for that difference. +6. The code action that changes the result, with its responsible component and relevant state changes. +7. The test runner, explicit selectors, build requirements, and expected coverage limits under [TR-FOCUSED-EVIDENCE](#tr-focused-evidence). +8. The reviewer document's exact path and planned commit identifier. + +Use the protocol test infrastructure described by [derive-tests-from-a-standard.md](../guide/derive-tests-from-a-standard.md). Add required fixture support to the plan when the current infrastructure cannot express the scenario. A helper test or fingerprint alone cannot replace the protocol test task. Keep tests faithful to the requirement. Apply the guide's failure classes before any expected-failure declaration. + +Include the tests and reviewer document in the commit that changes the protocol behavior. An earlier test commit is permitted when it respects [PR-SERIES-BUILDS](pull-request.md#pr-series-builds) and the expected-failure rules. In that case, identify both commits in the plan. The behavior commit must update its reviewer document and any test declaration that the repair changes. Do not leave test implementation or documentation to an unspecified later commit. + +Write one reviewer document for each applicable commit. Store it beside the protocol tests, for example at `tests/protocol//.md`. Associate it through the stable plan identifier, proposed subject, and test paths. A future commit has no SHA; do not require one. Link the document from the plan and commit body. The document supplements the standalone explanation required by [PR-MSG-STANDALONE](pull-request.md#pr-msg-standalone). + +The document must explain the protocol requirement, original scenario, cause or uncertainty, code correction, and every included protocol test. Explain the test setup, stimulus, expected exchanges or state, assertions, result before and after the change, and coverage limits. Use the human audience and explanatory style in [DR-EXPLAIN](documentation.md#dr-explain). A test list or command log alone is insufficient. + +Include as many diagrams as the explanation needs. Show relevant participants, exchanges, event order, timer boundaries, or state transitions. Use separate diagrams when distinct scenarios or before-and-after behavior need separate explanations. Do not impose a diagram count. State why diagrams add no useful information if none are necessary. + +Check the diagram rendering and its agreement with the test. Report unavailable rendering as unverified. + +Keep planned results separate from executed evidence in both artifacts. Record test results under [TR-FOCUSED-EVIDENCE](#tr-focused-evidence) when execution occurs. A plan-only task requires concrete test designs and explicit future document tasks; it does not authorize code or test implementation. + +*Enforced at T4 — review of each planned protocol commit, its test changes, and its reviewer document.* + ### TR-FP-NOT-ENOUGH **A fingerprint is never the only test of new behavior.**