Bitcoin Stack Exchange is a question and answer site for Bitcoin crypto-currency enthusiasts. It only takes a minute to sign up.
Sign up to join this community
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.
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.
[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 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.