docs: improve development guidelines for PR merging

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

Reword development guideline of PR merging to be more general meaning, expanding the meaning beyond CI

Based on the comment below

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
Stale ACK	rkrux

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

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.

docs: improve guidelines for PR merging 9a9725ecc5

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

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

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

docs: improve development guidelines for PR merging #32006

Code Coverage & Benchmarks

Reviews