docs: remove passing CI section in guidelines for PR merging as it's common sense

suiyuan1314 commented at 2:22 am on March 6, 2025: none

Removing passing CI section in guidelines for PR merging as it’s common sens

DrahtBot commented at 2:22 am on March 6, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32006.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, rkrux, maflcko
Concept NACK	pinheadmz, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

LLM Linter

Make sure pull request has verified build and test compatibility across environments → Make sure the pull request has verified build and test compatibility across environments

DrahtBot added the label Docs on Mar 6, 2025

in doc/developer-notes.md:733 in 18d3ea0193 outdated

729@@ -730,7 +730,7 @@ General Bitcoin Core
730 
731 - Make sure pull requests pass CI before merging.
732 
733-  - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing
734+  - *Rationale*: Make sure that they pass through testing, and that the tester will keep passing

pinheadmz commented at 2:40 am on March 6, 2025:

I think “thorough” is correct here

suiyuan1314 commented at 3:06 am on March 6, 2025:

yes. i guess it means pass all the testing cases. sorry for the inconvenience. i have reverted this change.

suiyuan1314 force-pushed on Mar 6, 2025

in doc/developer-notes.md:1418 in a695c9f517 outdated

1414@@ -1415,7 +1415,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1415     convert a plaintext command line to JSON. If the types don't match, the method can be unusable
1416     from there.
1417 
1418-- A RPC method must either be a wallet method or a non-wallet method. Do not
1419+- An RPC method must either be a wallet method or a non-wallet method. Do not

Sjors commented at 10:28 am on March 6, 2025:

Makes sense, although both “a” and “an” sound acceptable to me: https://www.utwente.nl/en/language-centre/translation-editing-services/english-styleguide/abbreviations/ (see “INDEFINITE ARTICLE ‘A’ OR ‘AN’ BEFORE ACRONYMS”)

suiyuan1314 commented at 11:15 am on March 6, 2025:

thanks for sharing this！

pinheadmz commented at 11:18 am on March 6, 2025:

I looked this up also but have no strong opinion. It could also be read “a remote procedure call” which would make the original correct

Sjors commented at 11:42 am on March 6, 2025:

@pinheadmz no, a/an always refers to the “RPC” acronym, not to its meaning. It only depends on how you pronounce the letters R P C, and specifically on how you pronounce “R” as a standalone letter, rather than as part of a word. This varies by accent. A pirate would say “an Ahrrr PC”.

rkrux commented at 1:32 pm on March 7, 2025: contributor

crACK a695c9f517d5b1c8a5b8b33abaa59c38b19c012b

in doc/developer-notes.md:735 in a695c9f517 outdated

729@@ -730,7 +730,7 @@ General Bitcoin Core
730 
731 - Make sure pull requests pass CI before merging.
732 
733-  - *Rationale*: Makes sure that they pass thorough testing, and that the tester will keep passing
734+  - *Rationale*: Make sure that they pass thorough testing, and that the tester will keep passing
735      on the master branch. Otherwise, all new pull requests will start failing the tests, resulting in
736      confusion and mayhem.
737

maflcko commented at 7:49 am on March 17, 2025:

this can probably be dropped completely. I’d guess it was written back when CI on open source was an fresh and possibly unknown concept. However, today, it seems common sense to require passing CI for all CI tasks, just like the code is required to compile on all supported build configs (even if they are not tested by CI). But the requirement that code must compile on all supported build configs isn’t mentioned here either.

So, I’d say to either remove this (as it is common sense), or reword it to a more general meaning, expanding the meaning beyond CI.

suiyuan1314 commented at 10:36 am on March 17, 2025:

Thanks for your reply, I have removed this.

maflcko commented at 10:47 am on March 17, 2025:

You’ll have to adjust the pull title, description and commit message as well to include the rationale. Removing a section is a bit different from just a typo fix.

suiyuan1314 commented at 11:53 am on March 17, 2025:

I have changed the commit message, the PR title and PR description, are they okay now?

Thanks for your review ^_^

rkrux commented at 1:24 pm on March 17, 2025:

IMO, this section should be kept by rewording it a bit as suggested in this comment #32006 (review). Though it is common sense now to ensure a passing CI, there could be new contributors who might not be aware of it and this section is something that can be shared with them if needed. Also, I’ve seen few instances of CI (beyond this repo) where the CI checks are not required to be passing for the PR to be merged, having this section would set clear expectations of a PR.
Having said this, my comment is not blocking if other reviewers are on-board with removing this section.

Also, the author would need to remove the tag to maflcko in the PR description because this causes the other person to be pinged repeatedly later but I can’t seem to find right now where this is stated.

suiyuan1314 commented at 2:07 pm on March 17, 2025:

I have reworded the guidelines, is it okay?

suiyuan1314 force-pushed on Mar 17, 2025

suiyuan1314 requested review from Sjors on Mar 17, 2025

suiyuan1314 requested review from pinheadmz on Mar 17, 2025

suiyuan1314 requested review from rkrux on Mar 17, 2025

suiyuan1314 requested review from maflcko on Mar 17, 2025

suiyuan1314 force-pushed on Mar 17, 2025

suiyuan1314 renamed this:
~~docs: fix typos~~
docs: remove CI passing requirement for PR merging
on Mar 17, 2025

suiyuan1314 force-pushed on Mar 17, 2025

suiyuan1314 renamed this:
~~docs: remove CI passing requirement for PR merging~~
docs: improve development guidelines for PR merging
on Mar 17, 2025

DrahtBot added the label CI failed on Mar 17, 2025

DrahtBot commented at 4:19 pm on March 17, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38893801821

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

suiyuan1314 force-pushed on Mar 18, 2025

DrahtBot removed the label CI failed on Mar 18, 2025

in doc/developer-notes.md:742 in 9a9725ecc5 outdated

743+    1. Must compile successfully under all supported build configurations
744+    2. Must pass automated test suites (unit, functional, fuzz)
745+    3. Must include new test coverage for added functionality
746+
747+  - *Implementation*:
748+    - For modifying existing behavior, update affected test cases first

maflcko commented at 11:57 am on March 27, 2025:

this should be self-explanatory, but “first” should be “at the same time”

in doc/developer-notes.md:745 in 9a9725ecc5 outdated

746+
747+  - *Implementation*:
748+    - For modifying existing behavior, update affected test cases first
749+    - For platform-specific changes, verify against multiple target environments
750+    - For new features, create test cases covering success/failure scenarios, edge case validation,
751+    and fuzz testing targets for security-critical components

maflcko commented at 11:58 am on March 27, 2025:

fuzz tests aren’t limited to security-critical components. Any test framework can be used for any component.

maflcko commented at 11:59 am on March 27, 2025: member

not sure if it makes sense to extend this further, at the risk of being inaccurate or redundant with other docs.

My preference would be to either remove it or keep it short

fanquake marked this as a draft on Apr 17, 2025

fanquake commented at 12:08 pm on April 17, 2025: member

@suiyuan1314 what’s the status of this? Concept ~0.

suiyuan1314 force-pushed on Apr 17, 2025

suiyuan1314 marked this as ready for review on Apr 17, 2025

suiyuan1314 commented at 1:20 pm on April 17, 2025: none

@suiyuan1314 what’s the status of this? Concept ~0.

Hi sir, I have updated the suggestion changes. Thanks for your review

jonatack commented at 5:59 pm on April 17, 2025: member

My preference would be to either remove it or keep it short

Same, suggest removing the section completely. It’s very handhold-y, and the few people who might need to read this information are probably the least likely to do so (edit: and maintainers know this already).

docs: remove passing CI section in guidelines for PR merging as it's common sense 17a1630f22

in doc/developer-notes.md:731 in 23476cec0e outdated

727@@ -728,14 +728,20 @@ General Bitcoin Core
728   - *Rationale*: RPC allows for better automatic testing. The test suite for
729     the GUI is very limited.
730 
731-- Make sure pull requests pass CI before merging.
732+- Make sure pull request has verified build and test compatibility across environments

jonatack commented at 6:01 pm on April 17, 2025:

Prefer the current version of this line to the proposed change. It is clearer, shorter, and more grammatically correct.

suiyuan1314 force-pushed on Apr 18, 2025

suiyuan1314 commented at 3:38 pm on April 18, 2025: none

My preference would be to either remove it or keep it short

Same, suggest removing the section completely. It’s very handhold-y, and the few people who might need to read this information are probably the least likely to do so.

Removed this section now :)

suiyuan1314 renamed this:
~~docs: improve development guidelines for PR merging~~
docs: remove passing CI section in guidelines for PR merging as it's common sense
on Apr 18, 2025

jonatack commented at 4:05 pm on April 18, 2025: member

ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

“Make sure pull requests pass CI before merging.” -> Indeed, this line seems pointless (maintainers are aware), ACK on removing it.

rkrux approved

rkrux commented at 8:43 am on April 19, 2025: contributor

ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

pinheadmz commented at 1:18 pm on April 21, 2025: member

NACK, it may not be common sense for new contributors. Also the tip about adding passing tests before code changes in commit history is useful. I’m not gonna push back if others really want to merge this change, it just doesn’t seem useful.

maflcko commented at 2:31 pm on April 21, 2025: member

lgtm ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

Makes sense to remove this, because it is common sense, and also redundant with the readme sections https://github.com/bitcoin/bitcoin/blob/master/README.md#automated-testing and https://github.com/bitcoin/bitcoin/blob/master/README.md#manual-quality-assurance-qa-testing.

maflcko closed this on Apr 21, 2025

maflcko reopened this on Apr 21, 2025

achow101 commented at 5:54 pm on April 22, 2025: member

NACK, agree with @pinheadmz

maflcko commented at 6:10 pm on April 22, 2025: member

Closing for now, due to controversy.

maflcko closed this on Apr 22, 2025

docs: remove passing CI section in guidelines for PR merging as it’s common sense #32006

Code Coverage & Benchmarks

Reviews

LLM Linter