<p>The Bitcoin Core contributing <a href="https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review" rel="nofollow noreferrer">guidelines</a> recommend that post Concept ACK, Approach ACK:</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>As you suggest &quot;I ran the tests on typical hardware&quot; 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.</p>
<p>Depending on the nature of the PR you may want to perform a less trivial workflow such as sending and receiving transactions.</p>
<p>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 <a href="https://github.com/fjahr/debugging_bitcoin" rel="nofollow noreferrer">debugging tools</a> like gdb and lldb.</p>
<p>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.</p>
<p>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.</p>
<p>Another thing to consider is whether the additional tests added in the PR are sufficient.</p>
<p>[edit: An example of something you can test that the CI won't test is to change a line of code in the PR, rebuild (i.e. run <code>make</code> again) and run the test (or multiple tests) that you expect to fail as a result. Jon Atack suggested this is a good way of reviewing <a href="https://github.com/bitcoin/bitcoin/pull/19951" rel="nofollow noreferrer">PR #19951</a> which at the time of writing (September 2020) is open and seeking review.]</p>
<p>This answer was collected together from comments from sipa, jonatack, hebasto, jnewbery, robot-dreams, instagibbs on IRC.</p>

CC BY-SA 4.0
2020-09-08T14:21:28.727Z
2020-09-27T17:24:57.223Z
2020-09-27T17:24:57.223Z
Michael Folkson
<p>I am reviewing an open Bitcoin Core PR. Should I run the tests or can I rely on the CI (continuous integration) output to catch test failures?</p>
<p>If I am to ACK a commit should I seek to catch flaky tests, run tests on different hardware, try extra test cases manually?</p>
<p>This question was asked by robot-dreams on IRC.</p>

<bitcoin-core-development><testing><bitcoincore-review>
Should I run the tests every time I review an open Bitcoin Core PR?
Michael Folkson
# Should I run the tests every time I review an open Bitcoin Core PR?

(Asked by: username_0 on 2020-09-08)

I am reviewing an open Bitcoin Core PR. Should I run the tests or can I rely on the CI (continuous integration) output to catch test failures?

If I am to ACK a commit should I seek to catch flaky tests, run tests on different hardware, try extra test cases manually?

This question was asked by robot-dreams on IRC.

**Tags:** bitcoin-core-development, testing, bitcoincore-review


---

## 1 answers

### Answer by username_0 on 2020-09-08. Score: 2 | Accepted answer

The Bitcoin Core contributing [guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) 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](https://github.com/fjahr/debugging_bitcoin) 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.

[edit: An example of something you can test that the CI won't test is to change a line of code in the PR, rebuild (i.e. run `make` again) and run the test (or multiple tests) that you expect to fail as a result. Jon Atack suggested this is a good way of reviewing [PR #19951](https://github.com/bitcoin/bitcoin/pull/19951) which at the time of writing (September 2020) is open and seeking review.]

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


---

