doc: clarify test placement guidance #35260

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. This leaves room for review friction around current-behavior tests, regression tests, and behavior-preserving refactors. This was also discussed a few years ago in #31212 (review)

Fix

Add a short General Testing section with guidance on when tests, manual test instructions, or no test changes are appropriate. Add a Commit Structure for Tests subsection explaining where tests usually belong in a commit stack.

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
ACK	ryanofsky
Concept ACK	hodlinator
Stale ACK	optout21, rkrux

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35296 (doc: Fix broken links in dev notes, move sections by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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

ACK 3df19b4e0aa90206197fcebbb27950133f46b91c

Prev: reACK b2cb955d518e03321e06fd0516c48a2b8e762b6b 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

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.

hodlinator commented at 12:41 PM on May 11, 2026:

Would expect these paragraphs in CONTRIBUTING.md, but I guess developer-notes.md already has advice on scripted diffs etc so this works too.

l0rinc force-pushed on May 11, 2026

rkrux approved

rkrux commented at 12:38 PM on May 11, 2026: contributor

lgtm ACK 3df19b4

I haven't gone through the links in detail but I understand the general idea.

DrahtBot requested review from optout21 on May 11, 2026

DrahtBot removed the label CI failed on May 11, 2026

hodlinator commented at 12:43 PM on May 11, 2026: contributor

Concept ACK

in doc/developer-notes.md:303 in 3df19b4e0a

 297 | @@ -298,6 +298,30 @@ Linux: `sudo apt install doxygen graphviz`
 298 |  
 299 |  MacOS: `brew install doxygen graphviz`
 300 |  
 301 | +## General Testing
 302 | +
 303 | +Tests should be added or updated for new features, bug fixes, and other behavior

maflcko commented at 3:55 PM on May 11, 2026:

Also, adding a test is not always the best choice. Sometimes it is better to manually document how to test something, instead of adding a brittle and confusing test for eternity.

ryanofsky approved

ryanofsky commented at 6:26 PM on May 14, 2026: contributor

Code review ACK 3df19b4e0aa90206197fcebbb27950133f46b91c

My vote for the concept would be +0.5. This seems like good advice to me, but I think it should only be added if there is broad agreement. The developer notes already contain some advice that is debatable, was merged without much discussion, and is hard to remove once added. I think the bar for adding any new advice should be high (higher than it has been historically).

I also think the guideline could be shorter without changing the meaning, and not relying as heavily on external sources. For example, I would just write:

When feasible, changes that modify externally observable behavior should be accompanied by functional or unit tests. Ideally, new tests should be added in separate commits before changing behavior, as characterization tests, and then updated atomically in later commits that add features or fix bugs. This makes it clear exactly how behavior is changing. But this is not always practical, and it is also OK to add new tests in the same commits that change behavior.

This could get the basic point across without being as granular. But your current version also seems good and readers may appreciate being told specifically what to do in different cases.

DrahtBot requested review from hodlinator on May 14, 2026

maflcko commented at 6:51 PM on May 14, 2026: member

The developer notes already contain some advice that is debatable, was merged without much discussion, and is hard to remove once added.

I think it would be fine to remove stuff, especially if it is stale, confusing, redundant, or controversial. See also the latest removal for reference: https://github.com/bitcoin/bitcoin/pull/32572

in doc/developer-notes.md:307 in 3df19b4e0a

 302 | +
 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.

sedited commented at 7:00 PM on May 14, 2026:

I'm not sure about adding this. We already seem to be doing a decent job at it, and as maflcko pointed out, sometimes the tests being added for an obvious one-line change can be brittle, or just don't warrant the additional complexity. Some tests do also require non-trivial maintenance. Maybe this can be formulated in a bit more aspirational tone? I think you make a good job of that in the other paragraph, with "rule of thumb", "consider", etc.

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>
Co-authored-by: ryanofsky <ryan@ofsky.org>
Co-authored-by: sedited <seb.kung@gmail.com>

658927c877

l0rinc force-pushed on May 14, 2026

l0rinc commented at 8:23 PM on May 14, 2026: contributor

Thanks for the comments, rewrote it 20 times, I think the new version is indeed easier to understand and contains the important parts - please let me know if it addressed your concerns.

maflcko commented at 7:26 AM on May 15, 2026: member

The developer notes already contain some advice that is debatable, was merged without much discussion, and is hard to remove once added.

I think it would be fine to remove stuff, especially if it is stale, confusing, redundant, or controversial. See also the latest removal for reference: #32572

I don't want to hijack this pull for removals, but I couldn't find any debatable sections, so if you want something removed, please leave a comment in #35296, and I may take it or ignore it :sweat_smile:

l0rinc commented at 10:16 AM on May 15, 2026: contributor

I don't want to hijack this pull for removals

What's your opinion about this PR?

maflcko commented at 11:29 AM on May 15, 2026: member

What's your opinion about this PR?

lgtm.

nit: I think the notes encompass three large sections: (1) style/whitespace (2) debug tricks and tools, and (3) guidelines. So I think this belongs in section (3), but just a nit.

ryanofsky approved

ryanofsky commented at 6:19 PM on May 18, 2026: contributor

Code review ACK 658927c877352e9486e8fa39b8af7545bce7dd22. Concept +0.5 as explained in previous review (seems like a positive change, but I'm wary of guidelines being added that are hard to remove later).

DrahtBot requested review from rkrux on May 18, 2026