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