<p>If <em>all</em> you did was run the automatic tests on a standard system, and found they succeeded, there is little point in commenting at all.</p>
<p>The continuous integration system already runs all tests (in a variety of platforms), and pull requests are generally not merged before all tests pass. If all you did is run the tests on a fairly standard system, commenting that those tests pass doesn't add any information.</p>
<p>Of course, if you did more, for example:</p>
<ul>
<li>You're using an unusual operating system, and the PR interacts with the OS in some way, commenting that the PR indeed doesn't break things for you is useful.</li>
<li>You did manual testing, by going through a scenario of operations (like RPC calls, ...) that aren't already covered by one of the PR's automatic tests, or you're testing on mainnet, or testing steps in the GUI (which aren't subject to automatic testing), that is useful too.</li>
<li>As you already pointed out in your answer, if a tests fails which normally doesn't fail for you before that PR, that is obviously useful as well.</li>
<li>If you did code review and are reporting the result of that (see Michael Folkson's answer), it won't hurt to add you also ran the tests.</li>
</ul>

CC BY-SA 4.0
2022-08-15T13:41:48.677Z
2022-08-15T13:41:48.677Z
Pieter Wuille
<p>As part of <a href="https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review" rel="nofollow noreferrer">ACKing a top commit hash</a> (post Concept ACK, Approach ACK) you are encouraged to explain what you did during your review that resulted in your ACK. As part of this comment it is perfectly fine (encouraged even) to say what tests you ran (and that they passed) on what operating system.</p>
<p>An example of such a review comment is this from Jon Atack's <a href="https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core" rel="nofollow noreferrer">doc</a> on how to review pull requests in Bitcoin Core:</p>
<blockquote>
<p>ACK fa2f991, I built, ran tests, tested manually by doing X/Y/Z and reviewed the code and it looks OK, I agree it can be merged.</p>
</blockquote>
<p>Some of the long term contributors may not do this and simply ACK the commit hash but maintainers have a better idea of what long term contributors are ACKing than a newer, less experienced contributor.</p>

CC BY-SA 4.0
2022-08-15T13:06:29.623Z
Thanks Michael, but consider that I'm very new to the Bitcoin Core project and in many cases I can't ACK or NACK the PR (due to my lack of understanding of many parts). So it is possible that I can't ACK the PR but I ran the tests and they're all passed. What is your opinion in this case?
Amir reza Riahi
CC BY-SA 4.0
2022-08-15T13:11:50.927Z
It is fine to ACK the commit hash, explain what you did/looked at but caveat it with a comment saying you are still learning about this part of the codebase etc. The maintainers will generally know who is experienced and who is inexperienced so don't worry if you ACK something and there turns out to be a problem with the PR. Alternatively you can just Concept ACK, Approach ACK a PR if you aren't comfortable with your understanding to give a code review ACK. But even with Concept ACK, Approach ACKs try to explain what you've done or what you've understood to give it a Concept ACK, Approach ACK
Michael Folkson
CC BY-SA 4.0
2022-08-15T13:15:16.603Z
Then maintainers can look at your review and go "Ok I think this person is inexperienced in this codebase but they clearly understand what the PR is doing and so I should factor in their review into the readiness for merge decision". With no rationale a Concept ACK or Approach ACK from an unknown pseudonym can't really be factored in. It could be a bot that hasn't even looked at the PR.
Michael Folkson
CC BY-SA 4.0
2022-08-15T13:00:52.910Z
2022-08-15T13:00:52.910Z
Michael Folkson
<p>I'm not sure is there a right place to ask this question or not, if it is off-topic please let me know.</p>
<p>When reviewing a pull request of the Bitcoin Core, is it common to comment and inform others that <strong>all</strong> tests for this PR are passed? Or we should left comment only to inform failing tests?</p>

<bitcoin-core-development><bitcoincore-review>
Should we inform others that all tests are passed in the course of reviewing a pull request?
Amir reza Riahi
# Should we inform others that all tests are passed in the course of reviewing a pull request?

(Asked by: username_0 on 2022-08-15)

I'm not sure is there a right place to ask this question or not, if it is off-topic please let me know.

When reviewing a pull request of the Bitcoin Core, is it common to comment and inform others that **all** tests for this PR are passed? Or we should left comment only to inform failing tests?

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


---

## 2 answers

### Answer by username_1 on 2022-08-15. Score: 3 | Accepted answer

If *all* you did was run the automatic tests on a standard system, and found they succeeded, there is little point in commenting at all.

The continuous integration system already runs all tests (in a variety of platforms), and pull requests are generally not merged before all tests pass. If all you did is run the tests on a fairly standard system, commenting that those tests pass doesn't add any information.

Of course, if you did more, for example:

* You're using an unusual operating system, and the PR interacts with the OS in some way, commenting that the PR indeed doesn't break things for you is useful.
* You did manual testing, by going through a scenario of operations (like RPC calls, ...) that aren't already covered by one of the PR's automatic tests, or you're testing on mainnet, or testing steps in the GUI (which aren't subject to automatic testing), that is useful too.
* As you already pointed out in your answer, if a tests fails which normally doesn't fail for you before that PR, that is obviously useful as well.
* If you did code review and are reporting the result of that (see username_2 username_2's answer), it won't hurt to add you also ran the tests.


---

### Answer by username_2 on 2022-08-15. Score: 2

As part of [ACKing a top commit hash](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) (post Concept ACK, Approach ACK) you are encouraged to explain what you did during your review that resulted in your ACK. As part of this comment it is perfectly fine (encouraged even) to say what tests you ran (and that they passed) on what operating system.

An example of such a review comment is this from Jon Atack's [doc](https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core) on how to review pull requests in Bitcoin Core:

> ACK fa2f991, I built, ran tests, tested manually by doing X/Y/Z and reviewed the code and it looks OK, I agree it can be merged.

Some of the long term contributors may not do this and simply ACK the commit hash but maintainers have a better idea of what long term contributors are ACKing than a newer, less experienced contributor.

> **Comments:**
> **username_0**: Thanks username_2, but consider that I'm very new to the Bitcoin Core project and in many cases I can't ACK or NACK the PR (due to my lack of understanding of many parts). So it is possible that I can't ACK the PR but I ran the tests and they're all passed. What is your opinion in this case?  
> **username_2**: It is fine to ACK the commit hash, explain what you did/looked at but caveat it with a comment saying you are still learning about this part of the codebase etc. The maintainers will generally know who is experienced and who is inexperienced so don't worry if you ACK something and there turns out to be a problem with the PR. Alternatively you can just Concept ACK, Approach ACK a PR if you aren't comfortable with your understanding to give a code review ACK. But even with Concept ACK, Approach ACKs try to explain what you've done or what you've understood to give it a Concept ACK, Approach ACK  
> **username_2**: Then maintainers can look at your review and go "Ok I think this person is inexperienced in this codebase but they clearly understand what the PR is doing and so I should factor in their review into the readiness for merge decision". With no rationale a Concept ACK or Approach ACK from an unknown pseudonym can't really be factored in. It could be a bot that hasn't even looked at the PR.  

---

