doc: clarify test placement guidance #35260

pull l0rinc wants to merge 1 commits into bitcoin:master from l0rinc:l0rinc/document-test-placement changing 2 files +26 −1

l0rinc commented at 10:32 AM on May 11, 2026: contributor

Problem

The developer notes do not currently explain how test coverage should be placed across a commit stack, or how that relates to the simpler single-commit case where a new test can be run against the old implementation. This leaves room for review friction around current-behavior tests, regression tests, and behavior-preserving refactors.

Fix

Add a short General Testing section with a Commit Structure for Tests subsection. It clarifies that behavior-changing commits should carry the relevant coverage, that behavior-preserving refactors can omit tests when there is no externally observable behavior to test, and that characterization tests can make stacked changes easier to review by recording uncovered current behavior before a later behavior change.
DrahtBot added the label Docs on May 11, 2026
DrahtBot commented at 10:32 AM on May 11, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35260.



Reviews

See the guideline for information on the review process.

Type Reviewers

Stale ACK optout21

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
maflcko commented at 10:37 AM on May 11, 2026: member

Seems fine, but for single commit prs, testing can also be done by not compiling the cpp code changes (or reverting them) and then running the test as-is and observing a failure.
in doc/developer-notes.md:301 in 737e5a488a
```
 297 | @@ -298,6 +298,25 @@ Linux: `sudo apt install doxygen graphviz`
 298 |  
 299 |  MacOS: `brew install doxygen graphviz`
 300 |  
 301 | +## Tests and Commit Structure
```
optout21 commented at 10:52 AM on May 11, 2026:

I have doubts that allocating a full top-level ("##") section to this issue is the best. Maybe the section could be called "General Testing", with the first paragraph, and the second paragraph demoted by one level to "### Commit Structure for Tests".

l0rinc commented at 11:17 AM on May 11, 2026:

Done, thanks
in doc/developer-notes.md:308 in 737e5a488a outdated
```
 303 | +Tests should be added or updated for new features, bug fixes, and other behavior
 304 | +changes. A behavior-changing commit should include the relevant test coverage.
 305 | +Test changes can be omitted for behavior-preserving work with no externally
 306 | +observable behavior to test, such as moving code, renaming, or mechanical
 307 | +refactors.
 308 | +
```
optout21 commented at 10:55 AM on May 11, 2026:

The general points on testing could also include that tests should check results or post-conditions (a test without proper checks increases code coverage, but has limited value). But this is already outside of the scope of the original intent.

l0rinc commented at 11:17 AM on May 11, 2026:

Partially done
optout21 commented at 11:01 AM on May 11, 2026: contributor

reACK b2cb955d518e03321e06fd0516c48a2b8e762b6b

Prev: ACK 737e5a488aa0e9ad1245e087c87aa3c465327c15 I consider the proposed approach good software craftsmanship. The text is well explained, and the 'why' question is supported by references.

There is a relevant quote in the mentioned blog post that I feel worthwhile to reproduce here:

When a system goes into production, in a way, it becomes its own specification. We need to know when we are changing existing behavior regardless of whether we think it’s right or not.
l0rinc commented at 11:17 AM on May 11, 2026: contributor

testing can also be done by not compiling the cpp code changes

I agree, especially for a small single-commit fix where reverting the code change is straightforward.

The proposed split is meant to make the evidence more granular. With a combined fix+test commit, reviewers can check that the bundle fails without the fix and passes with it, but the test itself does not stand alone as a record of prior behavior. With a separate current-behavior test commit, that commit documents what the code did before, and the following fix commit shows the minimal expectation change.

I agree that this matters most once the fix is larger than a one-line change. For refactors or multi-file changes, there often is no clean "just revert the cpp" recipe, and mixing the fix with several new assertions can make it harder to see which assertion changed and which ones were already true.

That is the characterization-testing angle: the existing system "becomes its own specification", so the split helps show exactly when behavior is being changed.

Adjusted the PR description, covered this angle, added @optout21's suggestions, thanks.
l0rinc force-pushed on May 11, 2026
DrahtBot added the label CI failed on May 11, 2026

Type	Reviewers
Stale ACK	optout21

doc: clarify test placement guidance

Add a short developer-notes section describing where test coverage usually belongs in a commit stack.

Clarify that a single behavior-changing commit can often be checked by running the new test against the old implementation, while a separate current-behavior test commit can make stacked changes easier to review by recording the prior behavior before the expectation changes.

Replace the existing CONTRIBUTING.md sentence with a link to the new developer-notes section, so the detailed guidance lives in one place.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>

3df19b4e0a

in doc/developer-notes.md:323 in b2cb955d51 outdated

 318 | +style keeps the evidence more granular: the test-only commit records prior
 319 | +behavior, and the behavior-changing commit shows the minimal expectation change;
 320 | +see also [Michael Feathers' original
 321 | +write-up](https://michaelfeathers.silvrback.com/characterization-testing).
 322 | +When prior behavior is already covered, update those tests in the same commit as
 323 | +the behavior change.

maflcko commented at 11:36 AM on May 11, 2026:

Reminds me of:

CONTRIBUTING.md:125:This means tests must be updated in the same commit that changes the behavior.

Maybe that line can be replaced by a link to this section?

Not sure what should go in CONTRIBUTING.md vs this file here.

l0rinc commented at 11:43 AM on May 11, 2026:

Good point, replaced the CONTRIBUTING.md duplication with a link to the new developer-notes section.

l0rinc force-pushed on May 11, 2026

Contributors

Labels

Linked (view graph)

#35251 wallet: Fix for duplicate external signers case

doc: clarify test placement guidance #35260

Problem

Fix

Code Coverage & Benchmarks

Reviews