bitcoincore development – Should I run the tests every time I review an open Bitcoin Core PR?


The Bitcoin Core contributing guidelines recommend that post Concept ACK, Approach ACK:

A review begins with ACK BRANCH_COMMIT, where BRANCH_COMMIT is the top
of the PR branch, followed by a description of how the reviewer did
the review.

As you suggest “I ran the tests on typical hardware” generally isn’t particularly useful as Bitcoin Core has solidly improving CI tools but there are exceptions e.g. a GUI change is not covered by tests and there would be value to running tests for certain IBD, validation changes, nontrivial changes too.

Depending on the nature of the PR you may want to perform a less trivial workflow such as sending and receiving transactions.

To get additional assurance that you are comfortable with the code change you could add debug prints, asserts, custom logging and sanity checks. You could change the patch or use debugging tools like gdb and lldb.

You can break many things without the CI or test suite picking it up. Manual testing can catch things that might be missed in code review. You might see warnings or errors when debug building PRs that you might not see otherwise, either because it’s buried in one of the CI job logs or because your compiler or configuration or system is different.

If the PR is implementing a particular BIP you could find a particular rule from the BIP in the code, mutate (break) the code and check that test(s) fail as a result.

Another thing to consider is whether the additional tests added in the PR are sufficient.

This answer was collected together from comments from sipa, jonatack, hebasto, jnewbery, robot-dreams, instagibbs on IRC.