highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-09-11 21:20:39 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#35260: doc: clarify test placement guidance

Browse Source 
db74d3390a doc: clarify test placement guidance (Lőrinc)

Pull request description:

  **Problem:** `doc/developer-notes.md` does not explain where test coverage belongs in a commit stack, especially when existing behavior is uncovered or a refactor depends on uncovered behavior.
  This has led to review questions about whether tests should record current behavior before a change or be added with the final behavior, for example in [#35251](https://github.com/bitcoin/bitcoin/pull/35251#discussion_r3217842286) and [#31212](https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854105033).

  **Fix:** 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.
  Add a `Commit Structure for Tests` subsection distinguishing existing coverage, simple uncovered changes, non-trivial changes to uncovered behavior, and non-trivial refactors whose preserved behavior is not covered.
  Replace the blanket `CONTRIBUTING.md` rule with a link to the detailed guidance.

ACKs for top commit:
  maflcko:
    lgtm ACK db74d3390a
  pablomartin4btc:
    ACK db74d3390a
  LarryRuane:
    ACK db74d3390a
  w0xlt:
    ACK db74d3390a
  sedited:
    ACK db74d3390a

Tree-SHA512: a8f3629b9bd59d20b1bc597d1b43fbb1d3cca9f500a8d79a7b62b417cd6b91c7b92cf6e32946171c76eb54e882a58eac94045753d8bb5899acdb48a7d1ccb2bd
This commit is contained in:
merge-script
2026-08-08 15:00:53 +02:00
parent 05a7c470d2 db74d3390a
commit 128456b62d
2 changed files with 31 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
doc/developer-notes.md
View File

@@ -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