1

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?

Improve this question

2 Answers 2

Reset to default
3

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 Michael Folkson's answer), it won't hurt to add you also ran the tests.
Improve this answer
2

As part of ACKing a top commit hash (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 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.

Improve this answer
3
  • 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
    Aug 15, 2022 at 13:06
  • 1
    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
    Aug 15, 2022 at 13:11
  • 1
    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
    Aug 15, 2022 at 13:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.