From db74d3390a391a2a76d7b4d676342a9d1489059b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 11 May 2026 12:20:26 +0200 Subject: [PATCH] doc: clarify test placement guidance The existing `CONTRIBUTING.md` rule requires tests to be updated in the same commit that changes behavior, but it does not explain how to handle existing behavior without coverage or refactors whose preserved behavior is not covered. Add a `General Testing` section under the development guidelines explaining when to use automated tests or a manual testing guide and when behavior-preserving work is easy to validate without new tests. The `Commit Structure for Tests` subsection distinguishes existing coverage, simple uncovered changes, non-trivial changes to uncovered behavior, and non-trivial refactors whose preserved behavior is not covered. Add `TODO` comments next to characterization assertions that record incorrect or temporary behavior, then remove the comments when the behavior-changing commit updates the assertions. Replace the blanket `CONTRIBUTING.md` rule with a link to the detailed guidance. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Co-authored-by: optout <13562139+optout21@users.noreply.github.com> Co-authored-by: ryanofsky Co-authored-by: sedited Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com> Co-authored-by: Ava Chow Co-authored-by: Pablo Martin --- CONTRIBUTING.md | 3 ++- doc/developer-notes.md | 29 +++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bcf0815d285..93be1099515 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -106,7 +106,8 @@ fixes or code moves with actual code changes. Make sure each individual commit is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures. -This means tests must be updated in the same commit that changes the behavior. +See the [developer notes](doc/developer-notes.md#commit-structure-for-tests) +for guidance on where test coverage belongs in a commit stack. Commit messages should be verbose by default consisting of a short subject line (50 chars max), a blank line and detailed explanatory text as separate diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 0bc1bd25027..b31d42a1124 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -673,6 +673,35 @@ Additional resources: A few non-style-related recommendations for developers, as well as points to pay attention to for reviewers of Bitcoin Core code. +## General Testing + +As a rule of thumb, an externally observable change (new feature, bug fix, or +changed default) should be accompanied by an automated test. +New tests are usually not needed for behavior-preserving work that is easy to +validate, such as moving code, renaming, or mechanical refactors. +When an automated test would be brittle or have limited long-term value, +a manual testing guide in the commit message or PR description is an +acceptable alternative. + +### Commit Structure for Tests + +Test placement depends on existing coverage and the type of change: + +* When existing tests already cover the behavior being changed, update them in + the same commit as the change. The diff records the old and new expectations + together and shows the change was intentional. +* For a simple feature or bug fix without existing coverage, the change and its + test can often be in the same commit. +* For a non-trivial refactor, if the relevant invariant is not already covered + by automated tests, first add that coverage in a separate test commit. The + refactor commit should not need to update test expectations. +* For a non-trivial change to existing behavior without coverage, consider + adding a preceding commit with a [characterization test](https://en.wikipedia.org/wiki/Characterization_test) + to document the current behavior. Mark assertions whose expected values will + change with `TODO` comments so they are not mistaken for intended behavior. + Remove the comments when updating those assertions in the behavior-changing + commit. + ## Locking/mutex usage notes The code is multi-threaded and uses mutexes and the