diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c0ac8800df2431..63e76a27fab182 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -13,7 +13,7 @@ Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md - [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes - [ ] tests and/or benchmarks are included - [ ] documentation is changed or added -- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines) +- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines) ##### Affected core subsystem(s) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index f96c9a2a0dd4d5..b88ab2a7843b73 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -437,7 +437,7 @@ The TSC should serve as the final arbiter where required. author when squashing. Review the commit message to ensure that it adheres to the guidelines outlined -in the [contributing](./CONTRIBUTING.md#commit-message-guidelines) guide. +in the [contributing](./doc/guides/contributing/pull-requests.md#commit-message-guidelines) guide. Add all necessary [metadata](#metadata) to commit messages before landing. @@ -467,7 +467,7 @@ $ git checkout master ``` Update the tree (assumes your repo is set up as detailed in -[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)): +[CONTRIBUTING.md](./doc/guides/contributing/pull-requests.md#step-1-fork)): ```text $ git fetch upstream @@ -562,7 +562,7 @@ commit logs, ensure that they are properly formatted, and add `Reviewed-By` lines. * The commit message text must conform to the -[commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines). +[commit message guidelines](./doc/guides/contributing/pull-requests.md#commit-message-guidelines). * Modify the original commit message to include additional metadata regarding diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b1cc67ada553ff..27f9dad928c651 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,821 +11,37 @@ small and all contributions are valued. This guide explains the process for contributing to the Node.js project's core `nodejs/node` GitHub Repository and describes what to expect at each step. -* [Code of Conduct](#code-of-conduct) - * [Bad Actors](#bad-actors) -* [Issues](#issues) - * [Asking for General Help](#asking-for-general-help) - * [Discussing non-technical topics](#discussing-non-technical-topics) - * [Submitting a Bug Report](#submitting-a-bug-report) - * [Triaging a Bug Report](#triaging-a-bug-report) - * [Resolving a Bug Report](#resolving-a-bug-report) -* [Pull Requests](#pull-requests) - * [Dependencies](#dependencies) - * [Setting up your local environment](#setting-up-your-local-environment) - * [Step 1: Fork](#step-1-fork) - * [Step 2: Branch](#step-2-branch) - * [The Process of Making Changes](#the-process-of-making-changes) - * [Step 3: Code](#step-3-code) - * [Step 4: Commit](#step-4-commit) - * [Commit message guidelines](#commit-message-guidelines) - * [Step 5: Rebase](#step-5-rebase) - * [Step 6: Test](#step-6-test) - * [Test Coverage](#test-coverage) - * [Step 7: Push](#step-7-push) - * [Step 8: Opening the Pull Request](#step-8-opening-the-pull-request) - * [Step 9: Discuss and Update](#step-9-discuss-and-update) - * [Approval and Request Changes Workflow](#approval-and-request-changes-workflow) - * [Step 10: Landing](#step-10-landing) - * [Reviewing Pull Requests](#reviewing-pull-requests) - * [Review a bit at a time](#review-a-bit-at-a-time) - * [Be aware of the person behind the code](#be-aware-of-the-person-behind-the-code) - * [Respect the minimum wait time for comments](#respect-the-minimum-wait-time-for-comments) - * [Abandoned or Stalled Pull Requests](#abandoned-or-stalled-pull-requests) - * [Approving a change](#approving-a-change) - * [Accept that there are different opinions about what belongs in Node.js](#accept-that-there-are-different-opinions-about-what-belongs-in-nodejs) - * [Performance is not everything](#performance-is-not-everything) - * [Continuous Integration Testing](#continuous-integration-testing) -* [Additional Notes](#additional-notes) - * [Commit Squashing](#commit-squashing) - * [Getting Approvals for your Pull Request](#getting-approvals-for-your-pull-request) - * [CI Testing](#ci-testing) - * [Waiting Until the Pull Request Gets Landed](#waiting-until-the-pull-request-gets-landed) - * [Check Out the Collaborator's Guide](#check-out-the-collaborators-guide) - * [Helpful Resources](#helpful-resources) -* [Developer's Certificate of Origin 1.1](#developers-certificate-of-origin-11) +## [Code of Conduct](./doc/guides/contributing/coc.md) -## Code of Conduct +The Node.js project has a +[Code of Conduct](https://github.com/nodejs/admin/blob/master/CODE_OF_CONDUCT.md) +that *all* contributors are expected to follow. This code describes the +*minimum* behavior expectations for all contributors. -The Node.js project has a [Code of Conduct][] that *all* contributors are -expected to follow. This code describes the *minimum* behavior expectations -for all contributors. +See [details on our policy on Code of Conduct](./doc/guides/contributing/coc.md). -As a contributor to Node.js, how you choose to act and interact towards your -fellow contributors, as well as to the community, will reflect back not only -on yourself but on the project as a whole. The Code of Conduct is designed and -intended, above all else, to help establish a culture within the project that -allows anyone and everyone who wants to contribute to feel safe doing so. - -Should any individual act in any way that is considered in violation of the -[Code of Conduct][], corrective actions will be taken. It is possible, however, -for any individual to *act* in such a manner that is not in violation of the -strict letter of the Code of Conduct guidelines while still going completely -against the spirit of what that Code is intended to accomplish. - -Open, diverse, and inclusive communities live and die on the basis of trust. -Contributors can disagree with one another so long as they trust that those -disagreements are in good faith and everyone is working towards a common goal. - -### Bad actors - -All contributors to Node.js tacitly agree to abide by both the letter and -spirit of the [Code of Conduct][]. Failure, or unwillingness, to do so will -result in contributions being respectfully declined. - -A *bad actor* is someone who repeatedly violates the *spirit* of the Code of -Conduct through consistent failure to self-regulate the way in which they -interact with other contributors in the project. In doing so, bad actors -alienate other contributors, discourage collaboration, and generally reflect -poorly on the project as a whole. - -Being a bad actor may be intentional or unintentional. Typically, unintentional -bad behavior can be easily corrected by being quick to apologize and correct -course *even if you are not entirely convinced you need to*. Giving other -contributors the benefit of the doubt and having a sincere willingness to admit -that you *might* be wrong is critical for any successful open collaboration. - -Don't be a bad actor. - -## Issues +## [Issues](./doc/guides/contributing/issues.md) Issues in `nodejs/node` are the primary means by which bug reports and -general discussions are made. For any issue, there are fundamentally three -ways an individual can contribute: - -1. By opening the issue for discussion: For instance, if you believe that you - have uncovered a bug in Node.js, creating a new issue in the `nodejs/node` - issue tracker is the way to report it. -2. By helping to triage the issue: This can be done either by providing - supporting details (a test case that demonstrates a bug), or providing - suggestions on how to address the issue. -3. By helping to resolve the issue: Typically this is done either in the form - of demonstrating that the issue reported is not a problem after all, or more - often, by opening a Pull Request that changes some bit of something in - `nodejs/node` in a concrete and reviewable manner. - -### Asking for General Help - -Because the level of activity in the `nodejs/node` repository is so high, -questions or requests for general help using Node.js should be directed at -the [Node.js help repository][]. - -### Discussing non-technical topics - -Discussion of non-technical topics (such as intellectual property and trademark) -should be directed to the [Technical Steering Committee (TSC) repository][]. - -### Submitting a Bug Report - -When opening a new issue in the `nodejs/node` issue tracker, users will be -presented with a basic template that should be filled in. - -```markdown - - -* **Version**: -* **Platform**: -* **Subsystem**: - - -``` - -If you believe that you have uncovered a bug in Node.js, please fill out this -form, following the template to the best of your ability. Do not worry if you -cannot answer every detail, just fill in what you can. - -The two most important pieces of information we need in order to properly -evaluate the report is a description of the behavior you are seeing and a simple -test case we can use to recreate the problem on our own. If we cannot recreate -the issue, it becomes impossible for us to fix. +general discussions are made. -In order to rule out the possibility of bugs introduced by userland code, test -cases should be limited, as much as possible, to using *only* Node.js APIs. -If the bug occurs only when you're using a specific userland module, there is -a very good chance that either (a) the module has a bug or (b) something in -Node.js changed that broke the module. +* [How to Contribute in Issues](./doc/guides/contributing/issues.md#how-to-contribute-in-issues) +* [Asking for General Help](./doc/guides/contributing/issues.md#asking-for-general-help) +* [Discussing non-technical topics](./doc/guides/contributing/issues.md#discussing-non-technical-topics) +* [Submitting a Bug Report](./doc/guides/contributing/issues.md#submitting-a-bug-report) +* [Triaging a Bug Report](./doc/guides/contributing/issues.md#triaging-a-bug-report) +* [Resolving a Bug Report](./doc/guides/contributing/issues.md#resolving-a-bug-report) -### Triaging a Bug Report - -Once an issue has been opened, it is not uncommon for there to be discussion -around it. Some contributors may have differing opinions about the issue, -including whether the behavior being seen is a bug or a feature. This discussion -is part of the process and should be kept focused, helpful, and professional. - -Short, clipped responses—that provide neither additional context nor supporting -detail—are not helpful or professional. To many, such responses are simply -annoying and unfriendly. - -Contributors are encouraged to help one another make forward progress as much -as possible, empowering one another to solve issues collaboratively. If you -choose to comment on an issue that you feel either is not a problem that needs -to be fixed, or if you encounter information in an issue that you feel is -incorrect, explain *why* you feel that way with additional supporting context, -and be willing to be convinced that you may be wrong. By doing so, we can often -reach the correct outcome much faster. - -### Resolving a Bug Report - -In the vast majority of cases, issues are resolved by opening a Pull Request. -The process for opening and reviewing a Pull Request is similar to that of -opening and triaging issues, but carries with it a necessary review and approval -workflow that ensures that the proposed changes meet the minimal quality and -functional guidelines of the Node.js project. - -## Pull Requests +## [Pull Requests](./doc/guides/contributing/pull-requests.md) Pull Requests are the way concrete changes are made to the code, documentation, dependencies, and tools contained in the `nodejs/node` repository. -There are two fundamental components of the Pull Request process: one concrete -and technical, and one more process oriented. The concrete and technical -component involves the specific details of setting up your local environment -so that you can make the actual changes. This is where we will start. - -### Dependencies - -Node.js has several bundled dependencies in the *deps/* and the *tools/* -directories that are not part of the project proper. Changes to files in those -directories should be sent to their respective projects. Do not send a patch to -Node.js. We cannot accept such patches. - -In case of doubt, open an issue in the -[issue tracker](https://github.com/nodejs/node/issues/) or contact one of the -[project Collaborators](https://github.com/nodejs/node/#current-project-team-members). -Node.js has two IRC channels: -[#Node.js](https://webchat.freenode.net/?channels=node.js) for general help and -questions, and -[#Node-dev](https://webchat.freenode.net/?channels=node-dev) for development of -Node.js core specifically. - -### Setting up your local environment - -To get started, you will need to have `git` installed locally. Depending on -your operating system, there are also a number of other dependencies required. -These are detailed in the [Building guide][]. - -Once you have `git` and are sure you have all of the necessary dependencies, -it's time to create a fork. - -Before getting started, it is recommended to configure `git` so that it knows -who you are: - -```text -$ git config --global user.name "J. Random User" -$ git config --global user.email "j.random.user@example.com" -``` -Please make sure this local email is also added to your -[GitHub email list](https://github.com/settings/emails) so that your commits -will be properly associated with your account and you will be promoted -to Contributor once your first commit is landed. - -#### Step 1: Fork - -Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork -locally. - -```text -$ git clone git@github.com:username/node.git -$ cd node -$ git remote add upstream https://github.com/nodejs/node.git -$ git fetch upstream -``` - -#### Step 2: Branch - -As a best practice to keep your development environment as organized as -possible, create local branches to work within. These should also be created -directly off of the `master` branch. - -```text -$ git checkout -b my-branch -t upstream/master -``` - -### The Process of Making Changes - -#### Step 3: Code - -The vast majority of Pull Requests opened against the `nodejs/node` -repository includes changes to either the C/C++ code contained in the `src` -directory, the JavaScript code contained in the `lib` directory, the -documentation in `docs/api` or tests within the `test` directory. - -If you are modifying code, please be sure to run `make lint` from time to -time to ensure that the changes follow the Node.js code style guide. - -Any documentation you write (including code comments and API documentation) -should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included -in the API docs will also be checked when running `make lint` (or -`vcbuild.bat lint` on Windows). - -For contributing C++ code, you may want to look at the -[C++ Style Guide](CPP_STYLE_GUIDE.md). - -#### Step 4: Commit - -It is a recommended best practice to keep your changes as logically grouped -as possible within individual commits. There is no limit to the number of -commits any single Pull Request may have, and many contributors find it easier -to review changes that are split across multiple commits. - -```text -$ git add my/changed/files -$ git commit -``` - -Note that multiple commits often get squashed when they are landed (see the -notes about [commit squashing](#commit-squashing)). - -##### Commit message guidelines - -A good commit message should describe what changed and why. - -1. The first line should: - - contain a short description of the change (preferably 50 characters or less, - and no more than 72 characters) - - be entirely in lowercase with the exception of proper nouns, acronyms, and - the words that refer to code, like function/variable names - - be prefixed with the name of the changed subsystem and start with an - imperative verb. Check the output of `git log --oneline files/you/changed` to - find out what subsystems your changes touch. - - Examples: - - `net: add localAddress and localPort to Socket` - - `src: fix typos in node_lttng_provider.h` - - -2. Keep the second line blank. -3. Wrap all other lines at 72 columns. - -4. If your patch fixes an open issue, you can add a reference to it at the end -of the log. Use the `Fixes:` prefix and the full issue URL. For other references -use `Refs:`. - - Examples: - - `Fixes: https://github.com/nodejs/node/issues/1337` - - `Refs: http://eslint.org/docs/rules/space-in-parens.html` - - `Refs: https://github.com/nodejs/node/pull/3615` - -5. If your commit introduces a breaking change (`semver-major`), it should -contain an explanation about the reason of the breaking change, which -situation would trigger the breaking change and what is the exact change. - -Breaking changes will be listed in the wiki with the aim to make upgrading -easier. Please have a look at [Breaking Changes](https://github.com/nodejs/node/wiki/Breaking-changes-between-v4-LTS-and-v6-LTS) -for the level of detail that's suitable. - -Sample complete commit message: - -```txt -subsystem: explain the commit in one line - -Body of commit message is a few lines of text, explaining things -in more detail, possibly giving some background about the issue -being fixed, etc. - -The body of the commit message can be several paragraphs, and -please do proper word-wrap and keep columns shorter than about -72 characters or so. That way, `git log` will show things -nicely even when it is indented. - -Fixes: https://github.com/nodejs/node/issues/1337 -Refs: http://eslint.org/docs/rules/space-in-parens.html -``` - -If you are new to contributing to Node.js, please try to do your best at -conforming to these guidelines, but do not worry if you get something wrong. -One of the existing contributors will help get things situated and the -contributor landing the Pull Request will ensure that everything follows -the project guidelines. - -#### Step 5: Rebase - -As a best practice, once you have committed your changes, it is a good idea -to use `git rebase` (not `git merge`) to synchronize your work with the main -repository. - -```text -$ git fetch upstream -$ git rebase upstream/master -``` - -This ensures that your working branch has the latest changes from `nodejs/node` -master. - -#### Step 6: Test - -Bug fixes and features should always come with tests. A -[guide for writing tests in Node.js](./doc/guides/writing-tests.md) has been -provided to make the process easier. Looking at other tests to see how they -should be structured can also help. - -The `test` directory within the `nodejs/node` repository is complex and it is -often not clear where a new test file should go. When in doubt, add new tests -to the `test/parallel/` directory and the right location will be sorted out -later. - -Before submitting your changes in a Pull Request, always run the full Node.js -test suite. To run the tests (including code linting) on Unix / macOS: - -```text -$ ./configure && make -j4 test -``` - -And on Windows: - -```text -> vcbuild test -``` - -(See the [BUILDING.md](./BUILDING.md) for more details.) - -Make sure the linter does not report any issues and that all tests pass. Please -do not submit patches that fail either check. - -If you want to run the linter without running tests, use -`make lint`/`vcbuild lint`. It will run both JavaScript linting and -C++ linting. - -If you are updating tests and just want to run a single test to check it: - -```text -$ python tools/test.py -J --mode=release parallel/test-stream2-transform -``` - -You can execute the entire suite of tests for a given subsystem -by providing the name of a subsystem: - -```text -$ python tools/test.py -J --mode=release child-process -``` - -If you want to check the other options, please refer to the help by using -the `--help` option - -```text -$ python tools/test.py --help -``` - -You can usually run tests directly with node: - -```text -$ ./node ./test/parallel/test-stream2-transform.js -``` - -Remember to recompile with `make -j4` in between test runs if you change code in -the `lib` or `src` directories. - -##### Test Coverage - -It's good practice to ensure any code you add or change is covered by tests. -You can do so by running the test suite with coverage enabled: - -```text -$ ./configure --coverage && make coverage -``` - -A detailed coverage report will be written to `coverage/index.html` for -JavaScript coverage and to `coverage/cxxcoverage.html` for C++ coverage. - -_Note that generating a test coverage report can take several minutes._ - -To collect coverage for a subset of tests you can set the `CI_JS_SUITES` and -`CI_NATIVE_SUITES` variables: - -```text -$ CI_JS_SUITES=child-process CI_NATIVE_SUITES= make coverage -``` - -The above command executes tests for the `child-process` subsystem and -outputs the resulting coverage report. - -Running tests with coverage will create and modify several directories -and files. To clean up afterwards, run: - -```text -make coverage-clean -./configure && make -j4. -``` - -#### Step 7: Push - -Once you are sure your commits are ready to go, with passing tests and linting, -begin the process of opening a Pull Request by pushing your working branch to -your fork on GitHub. - -```text -$ git push origin my-branch -``` - -#### Step 8: Opening the Pull Request - -From within GitHub, opening a new Pull Request will present you with a template -that should be filled out: - -```markdown - - -##### Checklist - - -- [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes -- [ ] tests and/or benchmarks are included -- [ ] documentation is changed or added -- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines) - -##### Affected core subsystem(s) - -``` - -Please try to do your best at filling out the details, but feel free to skip -parts if you're not sure what to put. - -Once opened, Pull Requests are usually reviewed within a few days. - -#### Step 9: Discuss and update - -You will probably get feedback or requests for changes to your Pull Request. -This is a big part of the submission process so don't be discouraged! Some -contributors may sign off on the Pull Request right away, others may have -more detailed comments or feedback. This is a necessary part of the process -in order to evaluate whether the changes are correct and necessary. - -To make changes to an existing Pull Request, make the changes to your local -branch, add a new commit with those changes, and push those to your fork. -GitHub will automatically update the Pull Request. - -```text -$ git add my/changed/files -$ git commit -$ git push origin my-branch -``` - -It is also frequently necessary to synchronize your Pull Request with other -changes that have landed in `master` by using `git rebase`: - -```text -$ git fetch --all -$ git rebase origin/master -$ git push --force-with-lease origin my-branch -``` - -**Important:** The `git push --force-with-lease` command is one of the few ways -to delete history in `git`. Before you use it, make sure you understand the -risks. If in doubt, you can always ask for guidance in the Pull Request or on -[IRC in the #node-dev channel][]. - -If you happen to make a mistake in any of your commits, do not worry. You can -amend the last commit (for example if you want to change the commit log). - -```text -$ git add any/changed/files -$ git commit --amend -$ git push --force-with-lease origin my-branch -``` - -There are a number of more advanced mechanisms for managing commits using -`git rebase` that can be used, but are beyond the scope of this guide. - -Feel free to post a comment in the Pull Request to ping reviewers if you are -awaiting an answer on something. If you encounter words or acronyms that -seem unfamiliar, refer to this -[glossary](https://sites.google.com/a/chromium.org/dev/glossary). - -##### Approval and Request Changes Workflow - -All Pull Requests require "sign off" in order to land. Whenever a contributor -reviews a Pull Request they may find specific details that they would like to -see changed or fixed. These may be as simple as fixing a typo, or may involve -substantive changes to the code you have written. In general, such requests -are intended to be helpful, but at times may come across as abrupt or unhelpful, -especially requests to change things that do not include concrete suggestions -on *how* to change them. - -Try not to be discouraged. If you feel that a particular review is unfair, -say so, or contact one of the other contributors in the project and seek their -input. Often such comments are the result of the reviewer having only taken a -short amount of time to review and are not ill-intended. Such issues can often -be resolved with a bit of patience. That said, reviewers should be expected to -be helpful in their feedback, and feedback that is simply vague, dismissive and -unhelpful is likely safe to ignore. - -#### Step 10: Landing - -In order to land, a Pull Request needs to be reviewed and [approved][] by -at least one Node.js Collaborator and pass a -[CI (Continuous Integration) test run][]. After that, as long as there are no -objections from other contributors, the Pull Request can be merged. If you find -your Pull Request waiting longer than you expect, see the -[notes about the waiting time](#waiting-until-the-pull-request-gets-landed). - -When a collaborator lands your Pull Request, they will post -a comment to the Pull Request page mentioning the commit(s) it -landed as. GitHub often shows the Pull Request as `Closed` at this -point, but don't worry. If you look at the branch you raised your -Pull Request against (probably `master`), you should see a commit with -your name on it. Congratulations and thanks for your contribution! - -### Reviewing Pull Requests - -All Node.js contributors who choose to review and provide feedback on Pull -Requests have a responsibility to both the project and the individual making the -contribution. Reviews and feedback must be helpful, insightful, and geared -towards improving the contribution as opposed to simply blocking it. If there -are reasons why you feel the PR should not land, explain what those are. Do not -expect to be able to block a Pull Request from advancing simply because you say -"No" without giving an explanation. Be open to having your mind changed. Be open -to working with the contributor to make the Pull Request better. - -Reviews that are dismissive or disrespectful of the contributor or any other -reviewers are strictly counter to the [Code of Conduct][]. - -When reviewing a Pull Request, the primary goals are for the codebase to improve -and for the person submitting the request to succeed. Even if a Pull Request -does not land, the submitters should come away from the experience feeling like -their effort was not wasted or unappreciated. Every Pull Request from a new -contributor is an opportunity to grow the community. - -#### Review a bit at a time. - -Do not overwhelm new contributors. - -It is tempting to micro-optimize and make everything about relative performance, -perfect grammar, or exact style matches. Do not succumb to that temptation. - -Focus first on the most significant aspects of the change: - -1. Does this change make sense for Node.js? -2. Does this change make Node.js better, even if only incrementally? -3. Are there clear bugs or larger scale issues that need attending to? -4. Is the commit message readable and correct? If it contains a breaking change is it clear enough? - -When changes are necessary, *request* them, do not *demand* them, and do not -assume that the submitter already knows how to add a test or run a benchmark. - -Specific performance optimization techniques, coding styles and conventions -change over time. The first impression you give to a new contributor never does. - -Nits (requests for small changes that are not essential) are fine, but try to -avoid stalling the Pull Request. Most nits can typically be fixed by the -Node.js Collaborator landing the Pull Request but they can also be an -opportunity for the contributor to learn a bit more about the project. - -It is always good to clearly indicate nits when you comment: e.g. -`Nit: change foo() to bar(). But this is not blocking.` - -#### Be aware of the person behind the code - -Be aware that *how* you communicate requests and reviews in your feedback can -have a significant impact on the success of the Pull Request. Yes, we may land -a particular change that makes Node.js better, but the individual might just -not want to have anything to do with Node.js ever again. The goal is not just -having good code. - -#### Respect the minimum wait time for comments - -There is a minimum waiting time which we try to respect for non-trivial -changes, so that people who may have important input in such a distributed -project are able to respond. - -For non-trivial changes, Pull Requests must be left open for *at least* 48 -hours during the week, and 72 hours on a weekend. In most cases, when the -PR is relatively small and focused on a narrow set of changes, these periods -provide more than enough time to adequately review. Sometimes changes take far -longer to review, or need more specialized review from subject matter experts. -When in doubt, do not rush. - -Trivial changes, typically limited to small formatting changes or fixes to -documentation, may be landed within the minimum 48 hour window. - -#### Abandoned or Stalled Pull Requests - -If a Pull Request appears to be abandoned or stalled, it is polite to first -check with the contributor to see if they intend to continue the work before -checking if they would mind if you took it over (especially if it just has -nits left). When doing so, it is courteous to give the original contributor -credit for the work they started (either by preserving their name and email -address in the commit log, or by using an `Author: ` meta-data tag in the -commit. - -#### Approving a change - -Any Node.js core Collaborator (any GitHub user with commit rights in the -`nodejs/node` repository) is authorized to approve any other contributor's -work. Collaborators are not permitted to approve their own Pull Requests. - -Collaborators indicate that they have reviewed and approve of the changes in -a Pull Request either by using GitHub's Approval Workflow, which is preferred, -or by leaving an `LGTM` ("Looks Good To Me") comment. - -When explicitly using the "Changes requested" component of the GitHub Approval -Workflow, show empathy. That is, do not be rude or abrupt with your feedback -and offer concrete suggestions for improvement, if possible. If you're not -sure *how* a particular change can be improved, say so. - -Most importantly, after leaving such requests, it is courteous to make yourself -available later to check whether your comments have been addressed. - -If you see that requested changes have been made, you can clear another -collaborator's `Changes requested` review. - -Change requests that are vague, dismissive, or unconstructive may also be -dismissed if requests for greater clarification go unanswered within a -reasonable period of time. - -If you do not believe that the Pull Request should land at all, use -`Changes requested` to indicate that you are considering some of your comments -to block the PR from landing. When doing so, explain *why* you believe the -Pull Request should not land along with an explanation of what may be an -acceptable alternative course, if any. - -#### Accept that there are different opinions about what belongs in Node.js - -Opinions on this vary, even among the members of the Technical Steering -Committee. - -One general rule of thumb is that if Node.js itself needs it (due to historic -or functional reasons), then it belongs in Node.js. For instance, `url` -parsing is in Node.js because of HTTP protocol support. - -Also, functionality that either cannot be implemented outside of core in any -reasonable way, or only with significant pain. - -It is not uncommon for contributors to suggest new features they feel would -make Node.js better. These may or may not make sense to add, but as with all -changes, be courteous in how you communicate your stance on these. Comments -that make the contributor feel like they should have "known better" or -ridiculed for even trying run counter to the [Code of Conduct][]. - -#### Performance is not everything - -Node.js has always optimized for speed of execution. If a particular change -can be shown to make some part of Node.js faster, it's quite likely to be -accepted. Claims that a particular Pull Request will make things faster will -almost always be met by requests for performance [benchmark results][] that -demonstrate the improvement. - -That said, performance is not the only factor to consider. Node.js also -optimizes in favor of not breaking existing code in the ecosystem, and not -changing working functional code just for the sake of changing. - -If a particular Pull Request introduces a performance or functional -regression, rather than simply rejecting the Pull Request, take the time to -work *with* the contributor on improving the change. Offer feedback and -advice on what would make the Pull Request acceptable, and do not assume that -the contributor should already know how to do that. Be explicit in your -feedback. - -#### Continuous Integration Testing - -All Pull Requests that contain changes to code must be run through -continuous integration (CI) testing at [https://ci.nodejs.org/][]. - -Only Node.js core Collaborators with commit rights to the `nodejs/node` -repository may start a CI testing run. The specific details of how to do -this are included in the new Collaborator [Onboarding guide][]. - -Ideally, the code change will pass ("be green") on all platform configurations -supported by Node.js (there are over 30 platform configurations currently). -This means that all tests pass and there are no linting errors. In reality, -however, it is not uncommon for the CI infrastructure itself to fail on -specific platforms or for so-called "flaky" tests to fail ("be red"). It is -vital to visually inspect the results of all failed ("red") tests to determine -whether the failure was caused by the changes in the Pull Request. - -## Additional Notes - -### Commit Squashing - -When the commits in your Pull Request land, they may be squashed -into one commit per logical change. Metadata will be added to the commit -message (including links to the Pull Request, links to relevant issues, -and the names of the reviewers). The commit history of your Pull Request, -however, will stay intact on the Pull Request page. - -For the size of "one logical change", -[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8) -can be a good example. It touches the implementation, the documentation, -and the tests, but is still one logical change. In general, the tests should -always pass when each individual commit lands on the master branch. - -### Getting Approvals for Your Pull Request - -A Pull Request is approved either by saying LGTM, which stands for -"Looks Good To Me", or by using GitHub's Approve button. -GitHub's Pull Request review feature can be used during the process. -For more information, check out -[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g) -or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/). - -After you push new changes to your branch, you need to get -approval for these new changes again, even if GitHub shows "Approved" -because the reviewers have hit the buttons before. - -### CI Testing - -Every Pull Request needs to be tested -to make sure that it works on the platforms that Node.js -supports. This is done by running the code through the CI system. - -Only a Collaborator can start a CI run. Usually one of them will do it -for you as approvals for the Pull Request come in. -If not, you can ask a Collaborator to start a CI run. - -### Waiting Until the Pull Request Gets Landed - -A Pull Request needs to stay open for at least 48 hours (72 hours on a -weekend) from when it is submitted, even after it gets approved and -passes the CI. This is to make sure that everyone has a chance to -weigh in. If the changes are trivial, collaborators may decide it -doesn't need to wait. A Pull Request may well take longer to be -merged in. All these precautions are important because Node.js is -widely used, so don't be discouraged! - -### Check Out the Collaborator's Guide - -If you want to know more about the code review and the landing process, -you can take a look at the -[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md). - -### Helpful Resources - -The following additional resources may be of assistance: - -* [How to create a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve) -* [core-validate-commit](https://github.com/evanlucas/core-validate-commit) - - A utility that ensures commits follow the commit formatting guidelines. +* [Dependencies](./doc/guides/contributing/pull-requests.md#dependencies) +* [Setting up your local environment](./doc/guides/contributing/pull-requests.md#setting-up-your-local-environment) +* [The Process of Making Changes](./doc/guides/contributing/pull-requests.md#the-process-of-making-changes) +* [Reviewing Pull Requests](./doc/guides/contributing/pull-requests.md#reviewing-pull-requests) +* [Additional Notes](./doc/guides/contributing/pull-requests.md#additional-notes) ## Developer's Certificate of Origin 1.1 @@ -853,14 +69,3 @@ By making a contribution to this project, I certify that: personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. - -[approved]: #getting-approvals-for-your-pull-request -[benchmark results]: ./doc/guides/writing-and-running-benchmarks.md -[Building guide]: ./BUILDING.md -[CI (Continuous Integration) test run]: #ci-testing -[Code of Conduct]: https://github.com/nodejs/admin/blob/master/CODE_OF_CONDUCT.md -[https://ci.nodejs.org/]: https://ci.nodejs.org/ -[IRC in the #node-dev channel]: https://webchat.freenode.net?channels=node-dev&uio=d4 -[Node.js help repository]: https://github.com/nodejs/help/issues -[Onboarding guide]: ./doc/onboarding.md -[Technical Steering Committee (TSC) repository]: https://github.com/nodejs/TSC/issues diff --git a/doc/guides/contributing/coc.md b/doc/guides/contributing/coc.md new file mode 100644 index 00000000000000..f2c337cf5099b3 --- /dev/null +++ b/doc/guides/contributing/coc.md @@ -0,0 +1,43 @@ +# Code of Conduct + +The Node.js project has a [Code of Conduct][] that *all* contributors are +expected to follow. This code describes the *minimum* behavior expectations +for all contributors. + +As a contributor to Node.js, how you choose to act and interact towards your +fellow contributors, as well as to the community, will reflect back not only +on yourself but on the project as a whole. The Code of Conduct is designed and +intended, above all else, to help establish a culture within the project that +allows anyone and everyone who wants to contribute to feel safe doing so. + +Should any individual act in any way that is considered in violation of the +[Code of Conduct][], corrective actions will be taken. It is possible, however, +for any individual to *act* in such a manner that is not in violation of the +strict letter of the Code of Conduct guidelines while still going completely +against the spirit of what that Code is intended to accomplish. + +Open, diverse, and inclusive communities live and die on the basis of trust. +Contributors can disagree with one another so long as they trust that those +disagreements are in good faith and everyone is working towards a common goal. + +## Bad actors + +All contributors to Node.js tacitly agree to abide by both the letter and +spirit of the [Code of Conduct][]. Failure, or unwillingness, to do so will +result in contributions being respectfully declined. + +A *bad actor* is someone who repeatedly violates the *spirit* of the Code of +Conduct through consistent failure to self-regulate the way in which they +interact with other contributors in the project. In doing so, bad actors +alienate other contributors, discourage collaboration, and generally reflect +poorly on the project as a whole. + +Being a bad actor may be intentional or unintentional. Typically, unintentional +bad behavior can be easily corrected by being quick to apologize and correct +course *even if you are not entirely convinced you need to*. Giving other +contributors the benefit of the doubt and having a sincere willingness to admit +that you *might* be wrong is critical for any successful open collaboration. + +Don't be a bad actor. + +[Code of Conduct]: https://github.com/nodejs/admin/blob/master/CODE_OF_CONDUCT.md diff --git a/doc/guides/contributing/issues.md b/doc/guides/contributing/issues.md new file mode 100644 index 00000000000000..054bbd7b2775f9 --- /dev/null +++ b/doc/guides/contributing/issues.md @@ -0,0 +1,113 @@ +# Issues + +* [How to Contribute in Issues](#how-to-contribute-in-issues) +* [Asking for General Help](#asking-for-general-help) +* [Discussing non-technical topics](#discussing-non-technical-topics) +* [Submitting a Bug Report](#submitting-a-bug-report) +* [Triaging a Bug Report](#triaging-a-bug-report) +* [Resolving a Bug Report](#resolving-a-bug-report) + +## How to Contribute in Issues + +For any issue, there are fundamentally three ways an individual can +contribute: + +1. By opening the issue for discussion: For instance, if you believe that you + have uncovered a bug in Node.js, creating a new issue in the `nodejs/node` + issue tracker is the way to report it. +2. By helping to triage the issue: This can be done either by providing + supporting details (a test case that demonstrates a bug), or providing + suggestions on how to address the issue. +3. By helping to resolve the issue: Typically this is done either in the form + of demonstrating that the issue reported is not a problem after all, or more + often, by opening a Pull Request that changes some bit of something in + `nodejs/node` in a concrete and reviewable manner. + +## Asking for General Help + +Because the level of activity in the `nodejs/node` repository is so high, +questions or requests for general help using Node.js should be directed at +the [Node.js help repository][]. + +## Discussing non-technical topics + +Discussion of non-technical topics (such as intellectual property and trademark) +should be directed to the [Technical Steering Committee (TSC) repository][]. + +## Submitting a Bug Report + +When opening a new issue in the `nodejs/node` issue tracker, users will be +presented with a basic template that should be filled in. + +```markdown + + +* **Version**: +* **Platform**: +* **Subsystem**: + + +``` + +If you believe that you have uncovered a bug in Node.js, please fill out this +form, following the template to the best of your ability. Do not worry if you +cannot answer every detail, just fill in what you can. + +The two most important pieces of information we need in order to properly +evaluate the report is a description of the behavior you are seeing and a simple +test case we can use to recreate the problem on our own. If we cannot recreate +the issue, it becomes impossible for us to fix. + +In order to rule out the possibility of bugs introduced by userland code, test +cases should be limited, as much as possible, to using *only* Node.js APIs. +If the bug occurs only when you're using a specific userland module, there is +a very good chance that either (a) the module has a bug or (b) something in +Node.js changed that broke the module. + +See [How to create a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve). + +## Triaging a Bug Report + +Once an issue has been opened, it is not uncommon for there to be discussion +around it. Some contributors may have differing opinions about the issue, +including whether the behavior being seen is a bug or a feature. This discussion +is part of the process and should be kept focused, helpful, and professional. + +Short, clipped responses—that provide neither additional context nor supporting +detail—are not helpful or professional. To many, such responses are simply +annoying and unfriendly. + +Contributors are encouraged to help one another make forward progress as much +as possible, empowering one another to solve issues collaboratively. If you +choose to comment on an issue that you feel either is not a problem that needs +to be fixed, or if you encounter information in an issue that you feel is +incorrect, explain *why* you feel that way with additional supporting context, +and be willing to be convinced that you may be wrong. By doing so, we can often +reach the correct outcome much faster. + +## Resolving a Bug Report + +In the vast majority of cases, issues are resolved by opening a Pull Request. +The process for opening and reviewing a Pull Request is similar to that of +opening and triaging issues, but carries with it a necessary review and approval +workflow that ensures that the proposed changes meet the minimal quality and +functional guidelines of the Node.js project. + +[Node.js help repository]: https://github.com/nodejs/help/issues +[Technical Steering Committee (TSC) repository]: https://github.com/nodejs/TSC/issues diff --git a/doc/guides/contributing/pull-requests.md b/doc/guides/contributing/pull-requests.md new file mode 100644 index 00000000000000..5812c8c54645e2 --- /dev/null +++ b/doc/guides/contributing/pull-requests.md @@ -0,0 +1,663 @@ +# Pull Requests + +There are two fundamental components of the Pull Request process: one concrete +and technical, and one more process oriented. The concrete and technical +component involves the specific details of setting up your local environment +so that you can make the actual changes. This is where we will start. + +* [Dependencies](#dependencies) +* [Setting up your local environment](#setting-up-your-local-environment) + * [Step 1: Fork](#step-1-fork) + * [Step 2: Branch](#step-2-branch) +* [The Process of Making Changes](#the-process-of-making-changes) + * [Step 3: Code](#step-3-code) + * [Step 4: Commit](#step-4-commit) + * [Commit message guidelines](#commit-message-guidelines) + * [Step 5: Rebase](#step-5-rebase) + * [Step 6: Test](#step-6-test) + * [Test Coverage](#test-coverage) + * [Step 7: Push](#step-7-push) + * [Step 8: Opening the Pull Request](#step-8-opening-the-pull-request) + * [Step 9: Discuss and Update](#step-9-discuss-and-update) + * [Approval and Request Changes Workflow](#approval-and-request-changes-workflow) + * [Step 10: Landing](#step-10-landing) +* [Reviewing Pull Requests](#reviewing-pull-requests) + * [Review a bit at a time](#review-a-bit-at-a-time) + * [Be aware of the person behind the code](#be-aware-of-the-person-behind-the-code) + * [Respect the minimum wait time for comments](#respect-the-minimum-wait-time-for-comments) + * [Abandoned or Stalled Pull Requests](#abandoned-or-stalled-pull-requests) + * [Approving a change](#approving-a-change) + * [Accept that there are different opinions about what belongs in Node.js](#accept-that-there-are-different-opinions-about-what-belongs-in-nodejs) + * [Performance is not everything](#performance-is-not-everything) + * [Continuous Integration Testing](#continuous-integration-testing) +* [Additional Notes](#additional-notes) + * [Commit Squashing](#commit-squashing) + * [Getting Approvals for your Pull Request](#getting-approvals-for-your-pull-request) + * [CI Testing](#ci-testing) + * [Waiting Until the Pull Request Gets Landed](#waiting-until-the-pull-request-gets-landed) + * [Check Out the Collaborator's Guide](#check-out-the-collaborators-guide) + +## Dependencies + +Node.js has several bundled dependencies in the *deps/* and the *tools/* +directories that are not part of the project proper. Changes to files in those +directories should be sent to their respective projects. Do not send a patch to +Node.js. We cannot accept such patches. + +In case of doubt, open an issue in the +[issue tracker](https://github.com/nodejs/node/issues/) or contact one of the +[project Collaborators](https://github.com/nodejs/node/#current-project-team-members). +Node.js has two IRC channels: +[#Node.js](https://webchat.freenode.net/?channels=node.js) for general help and +questions, and +[#Node-dev](https://webchat.freenode.net/?channels=node-dev) for development of +Node.js core specifically. + +## Setting up your local environment + +To get started, you will need to have `git` installed locally. Depending on +your operating system, there are also a number of other dependencies required. +These are detailed in the [Building guide][]. + +Once you have `git` and are sure you have all of the necessary dependencies, +it's time to create a fork. + +Before getting started, it is recommended to configure `git` so that it knows +who you are: + +```text +$ git config --global user.name "J. Random User" +$ git config --global user.email "j.random.user@example.com" +``` +Please make sure this local email is also added to your +[GitHub email list](https://github.com/settings/emails) so that your commits +will be properly associated with your account and you will be promoted +to Contributor once your first commit is landed. + +### Step 1: Fork + +Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork +locally. + +```text +$ git clone git@github.com:username/node.git +$ cd node +$ git remote add upstream https://github.com/nodejs/node.git +$ git fetch upstream +``` + +### Step 2: Branch + +As a best practice to keep your development environment as organized as +possible, create local branches to work within. These should also be created +directly off of the `master` branch. + +```text +$ git checkout -b my-branch -t upstream/master +``` + +## The Process of Making Changes + +### Step 3: Code + +The vast majority of Pull Requests opened against the `nodejs/node` +repository includes changes to either the C/C++ code contained in the `src` +directory, the JavaScript code contained in the `lib` directory, the +documentation in `docs/api` or tests within the `test` directory. + +If you are modifying code, please be sure to run `make lint` from time to +time to ensure that the changes follow the Node.js code style guide. + +Any documentation you write (including code comments and API documentation) +should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included +in the API docs will also be checked when running `make lint` (or +`vcbuild.bat lint` on Windows). + +For contributing C++ code, you may want to look at the +[C++ Style Guide](CPP_STYLE_GUIDE.md). + +### Step 4: Commit + +It is a recommended best practice to keep your changes as logically grouped +as possible within individual commits. There is no limit to the number of +commits any single Pull Request may have, and many contributors find it easier +to review changes that are split across multiple commits. + +```text +$ git add my/changed/files +$ git commit +``` + +Note that multiple commits often get squashed when they are landed (see the +notes about [commit squashing](#commit-squashing)). + +#### Commit message guidelines + +A good commit message should describe what changed and why. + +1. The first line should: + - contain a short description of the change (preferably 50 characters or less, + and no more than 72 characters) + - be entirely in lowercase with the exception of proper nouns, acronyms, and + the words that refer to code, like function/variable names + - be prefixed with the name of the changed subsystem and start with an + imperative verb. Check the output of `git log --oneline files/you/changed` to + find out what subsystems your changes touch. + + Examples: + - `net: add localAddress and localPort to Socket` + - `src: fix typos in node_lttng_provider.h` + + +2. Keep the second line blank. +3. Wrap all other lines at 72 columns. + +4. If your patch fixes an open issue, you can add a reference to it at the end +of the log. Use the `Fixes:` prefix and the full issue URL. For other references +use `Refs:`. + + Examples: + - `Fixes: https://github.com/nodejs/node/issues/1337` + - `Refs: http://eslint.org/docs/rules/space-in-parens.html` + - `Refs: https://github.com/nodejs/node/pull/3615` + +5. If your commit introduces a breaking change (`semver-major`), it should +contain an explanation about the reason of the breaking change, which +situation would trigger the breaking change and what is the exact change. + +Breaking changes will be listed in the wiki with the aim to make upgrading +easier. Please have a look at [Breaking Changes](https://github.com/nodejs/node/wiki/Breaking-changes-between-v4-LTS-and-v6-LTS) +for the level of detail that's suitable. + +Sample complete commit message: + +```txt +subsystem: explain the commit in one line + +Body of commit message is a few lines of text, explaining things +in more detail, possibly giving some background about the issue +being fixed, etc. + +The body of the commit message can be several paragraphs, and +please do proper word-wrap and keep columns shorter than about +72 characters or so. That way, `git log` will show things +nicely even when it is indented. + +Fixes: https://github.com/nodejs/node/issues/1337 +Refs: http://eslint.org/docs/rules/space-in-parens.html +``` + +If you are new to contributing to Node.js, please try to do your best at +conforming to these guidelines, but do not worry if you get something wrong. +One of the existing contributors will help get things situated and the +contributor landing the Pull Request will ensure that everything follows +the project guidelines. + +See [core-validate-commit](https://github.com/evanlucas/core-validate-commit) - +A utility that ensures commits follow the commit formatting guidelines. + +### Step 5: Rebase + +As a best practice, once you have committed your changes, it is a good idea +to use `git rebase` (not `git merge`) to synchronize your work with the main +repository. + +```text +$ git fetch upstream +$ git rebase upstream/master +``` + +This ensures that your working branch has the latest changes from `nodejs/node` +master. + +### Step 6: Test + +Bug fixes and features should always come with tests. A +[guide for writing tests in Node.js][] has been +provided to make the process easier. Looking at other tests to see how they +should be structured can also help. + +The `test` directory within the `nodejs/node` repository is complex and it is +often not clear where a new test file should go. When in doubt, add new tests +to the `test/parallel/` directory and the right location will be sorted out +later. + +Before submitting your changes in a Pull Request, always run the full Node.js +test suite. To run the tests (including code linting) on Unix / macOS: + +```text +$ ./configure && make -j4 test +``` + +And on Windows: + +```text +> vcbuild test +``` + +(See the [Building guide][] for more details.) + +Make sure the linter does not report any issues and that all tests pass. Please +do not submit patches that fail either check. + +If you want to run the linter without running tests, use +`make lint`/`vcbuild lint`. It will run both JavaScript linting and +C++ linting. + +If you are updating tests and just want to run a single test to check it: + +```text +$ python tools/test.py -J --mode=release parallel/test-stream2-transform +``` + +You can execute the entire suite of tests for a given subsystem +by providing the name of a subsystem: + +```text +$ python tools/test.py -J --mode=release child-process +``` + +If you want to check the other options, please refer to the help by using +the `--help` option + +```text +$ python tools/test.py --help +``` + +You can usually run tests directly with node: + +```text +$ ./node ./test/parallel/test-stream2-transform.js +``` + +Remember to recompile with `make -j4` in between test runs if you change code in +the `lib` or `src` directories. + +#### Test Coverage + +It's good practice to ensure any code you add or change is covered by tests. +You can do so by running the test suite with coverage enabled: + +```text +$ ./configure --coverage && make coverage +``` + +A detailed coverage report will be written to `coverage/index.html` for +JavaScript coverage and to `coverage/cxxcoverage.html` for C++ coverage. + +_Note that generating a test coverage report can take several minutes._ + +To collect coverage for a subset of tests you can set the `CI_JS_SUITES` and +`CI_NATIVE_SUITES` variables: + +```text +$ CI_JS_SUITES=child-process CI_NATIVE_SUITES= make coverage +``` + +The above command executes tests for the `child-process` subsystem and +outputs the resulting coverage report. + +Running tests with coverage will create and modify several directories +and files. To clean up afterwards, run: + +```text +make coverage-clean +./configure && make -j4. +``` + +### Step 7: Push + +Once you are sure your commits are ready to go, with passing tests and linting, +begin the process of opening a Pull Request by pushing your working branch to +your fork on GitHub. + +```text +$ git push origin my-branch +``` + +### Step 8: Opening the Pull Request + +From within GitHub, opening a new Pull Request will present you with a template +that should be filled out: + +```markdown + + +#### Checklist + + +- [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes +- [ ] tests and/or benchmarks are included +- [ ] documentation is changed or added +- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines) + +#### Affected core subsystem(s) + +``` + +Please try to do your best at filling out the details, but feel free to skip +parts if you're not sure what to put. + +Once opened, Pull Requests are usually reviewed within a few days. + +### Step 9: Discuss and update + +You will probably get feedback or requests for changes to your Pull Request. +This is a big part of the submission process so don't be discouraged! Some +contributors may sign off on the Pull Request right away, others may have +more detailed comments or feedback. This is a necessary part of the process +in order to evaluate whether the changes are correct and necessary. + +To make changes to an existing Pull Request, make the changes to your local +branch, add a new commit with those changes, and push those to your fork. +GitHub will automatically update the Pull Request. + +```text +$ git add my/changed/files +$ git commit +$ git push origin my-branch +``` + +It is also frequently necessary to synchronize your Pull Request with other +changes that have landed in `master` by using `git rebase`: + +```text +$ git fetch --all +$ git rebase origin/master +$ git push --force-with-lease origin my-branch +``` + +**Important:** The `git push --force-with-lease` command is one of the few ways +to delete history in `git`. Before you use it, make sure you understand the +risks. If in doubt, you can always ask for guidance in the Pull Request or on +[IRC in the #node-dev channel][]. + +If you happen to make a mistake in any of your commits, do not worry. You can +amend the last commit (for example if you want to change the commit log). + +```text +$ git add any/changed/files +$ git commit --amend +$ git push --force-with-lease origin my-branch +``` + +There are a number of more advanced mechanisms for managing commits using +`git rebase` that can be used, but are beyond the scope of this guide. + +Feel free to post a comment in the Pull Request to ping reviewers if you are +awaiting an answer on something. If you encounter words or acronyms that +seem unfamiliar, refer to this +[glossary](https://sites.google.com/a/chromium.org/dev/glossary). + +#### Approval and Request Changes Workflow + +All Pull Requests require "sign off" in order to land. Whenever a contributor +reviews a Pull Request they may find specific details that they would like to +see changed or fixed. These may be as simple as fixing a typo, or may involve +substantive changes to the code you have written. In general, such requests +are intended to be helpful, but at times may come across as abrupt or unhelpful, +especially requests to change things that do not include concrete suggestions +on *how* to change them. + +Try not to be discouraged. If you feel that a particular review is unfair, +say so, or contact one of the other contributors in the project and seek their +input. Often such comments are the result of the reviewer having only taken a +short amount of time to review and are not ill-intended. Such issues can often +be resolved with a bit of patience. That said, reviewers should be expected to +be helpful in their feedback, and feedback that is simply vague, dismissive and +unhelpful is likely safe to ignore. + +### Step 10: Landing + +In order to land, a Pull Request needs to be reviewed and [approved][] by +at least one Node.js Collaborator and pass a +[CI (Continuous Integration) test run][]. After that, as long as there are no +objections from other contributors, the Pull Request can be merged. If you find +your Pull Request waiting longer than you expect, see the +[notes about the waiting time](#waiting-until-the-pull-request-gets-landed). + +When a collaborator lands your Pull Request, they will post +a comment to the Pull Request page mentioning the commit(s) it +landed as. GitHub often shows the Pull Request as `Closed` at this +point, but don't worry. If you look at the branch you raised your +Pull Request against (probably `master`), you should see a commit with +your name on it. Congratulations and thanks for your contribution! + +## Reviewing Pull Requests + +All Node.js contributors who choose to review and provide feedback on Pull +Requests have a responsibility to both the project and the individual making the +contribution. Reviews and feedback must be helpful, insightful, and geared +towards improving the contribution as opposed to simply blocking it. If there +are reasons why you feel the PR should not land, explain what those are. Do not +expect to be able to block a Pull Request from advancing simply because you say +"No" without giving an explanation. Be open to having your mind changed. Be open +to working with the contributor to make the Pull Request better. + +Reviews that are dismissive or disrespectful of the contributor or any other +reviewers are strictly counter to the [Code of Conduct][]. + +When reviewing a Pull Request, the primary goals are for the codebase to improve +and for the person submitting the request to succeed. Even if a Pull Request +does not land, the submitters should come away from the experience feeling like +their effort was not wasted or unappreciated. Every Pull Request from a new +contributor is an opportunity to grow the community. + +### Review a bit at a time. + +Do not overwhelm new contributors. + +It is tempting to micro-optimize and make everything about relative performance, +perfect grammar, or exact style matches. Do not succumb to that temptation. + +Focus first on the most significant aspects of the change: + +1. Does this change make sense for Node.js? +2. Does this change make Node.js better, even if only incrementally? +3. Are there clear bugs or larger scale issues that need attending to? +4. Is the commit message readable and correct? If it contains a breaking change is it clear enough? + +When changes are necessary, *request* them, do not *demand* them, and do not +assume that the submitter already knows how to add a test or run a benchmark. + +Specific performance optimization techniques, coding styles and conventions +change over time. The first impression you give to a new contributor never does. + +Nits (requests for small changes that are not essential) are fine, but try to +avoid stalling the Pull Request. Most nits can typically be fixed by the +Node.js Collaborator landing the Pull Request but they can also be an +opportunity for the contributor to learn a bit more about the project. + +It is always good to clearly indicate nits when you comment: e.g. +`Nit: change foo() to bar(). But this is not blocking.` + +### Be aware of the person behind the code + +Be aware that *how* you communicate requests and reviews in your feedback can +have a significant impact on the success of the Pull Request. Yes, we may land +a particular change that makes Node.js better, but the individual might just +not want to have anything to do with Node.js ever again. The goal is not just +having good code. + +### Respect the minimum wait time for comments + +There is a minimum waiting time which we try to respect for non-trivial +changes, so that people who may have important input in such a distributed +project are able to respond. + +For non-trivial changes, Pull Requests must be left open for *at least* 48 +hours during the week, and 72 hours on a weekend. In most cases, when the +PR is relatively small and focused on a narrow set of changes, these periods +provide more than enough time to adequately review. Sometimes changes take far +longer to review, or need more specialized review from subject matter experts. +When in doubt, do not rush. + +Trivial changes, typically limited to small formatting changes or fixes to +documentation, may be landed within the minimum 48 hour window. + +### Abandoned or Stalled Pull Requests + +If a Pull Request appears to be abandoned or stalled, it is polite to first +check with the contributor to see if they intend to continue the work before +checking if they would mind if you took it over (especially if it just has +nits left). When doing so, it is courteous to give the original contributor +credit for the work they started (either by preserving their name and email +address in the commit log, or by using an `Author: ` meta-data tag in the +commit. + +### Approving a change + +Any Node.js core Collaborator (any GitHub user with commit rights in the +`nodejs/node` repository) is authorized to approve any other contributor's +work. Collaborators are not permitted to approve their own Pull Requests. + +Collaborators indicate that they have reviewed and approve of the changes in +a Pull Request either by using GitHub's Approval Workflow, which is preferred, +or by leaving an `LGTM` ("Looks Good To Me") comment. + +When explicitly using the "Changes requested" component of the GitHub Approval +Workflow, show empathy. That is, do not be rude or abrupt with your feedback +and offer concrete suggestions for improvement, if possible. If you're not +sure *how* a particular change can be improved, say so. + +Most importantly, after leaving such requests, it is courteous to make yourself +available later to check whether your comments have been addressed. + +If you see that requested changes have been made, you can clear another +collaborator's `Changes requested` review. + +Change requests that are vague, dismissive, or unconstructive may also be +dismissed if requests for greater clarification go unanswered within a +reasonable period of time. + +If you do not believe that the Pull Request should land at all, use +`Changes requested` to indicate that you are considering some of your comments +to block the PR from landing. When doing so, explain *why* you believe the +Pull Request should not land along with an explanation of what may be an +acceptable alternative course, if any. + +### Accept that there are different opinions about what belongs in Node.js + +Opinions on this vary, even among the members of the Technical Steering +Committee. + +One general rule of thumb is that if Node.js itself needs it (due to historic +or functional reasons), then it belongs in Node.js. For instance, `url` +parsing is in Node.js because of HTTP protocol support. + +Also, functionality that either cannot be implemented outside of core in any +reasonable way, or only with significant pain. + +It is not uncommon for contributors to suggest new features they feel would +make Node.js better. These may or may not make sense to add, but as with all +changes, be courteous in how you communicate your stance on these. Comments +that make the contributor feel like they should have "known better" or +ridiculed for even trying run counter to the [Code of Conduct][]. + +### Performance is not everything + +Node.js has always optimized for speed of execution. If a particular change +can be shown to make some part of Node.js faster, it's quite likely to be +accepted. Claims that a particular Pull Request will make things faster will +almost always be met by requests for performance [benchmark results][] that +demonstrate the improvement. + +That said, performance is not the only factor to consider. Node.js also +optimizes in favor of not breaking existing code in the ecosystem, and not +changing working functional code just for the sake of changing. + +If a particular Pull Request introduces a performance or functional +regression, rather than simply rejecting the Pull Request, take the time to +work *with* the contributor on improving the change. Offer feedback and +advice on what would make the Pull Request acceptable, and do not assume that +the contributor should already know how to do that. Be explicit in your +feedback. + +### Continuous Integration Testing + +All Pull Requests that contain changes to code must be run through +continuous integration (CI) testing at [https://ci.nodejs.org/][]. + +Only Node.js core Collaborators with commit rights to the `nodejs/node` +repository may start a CI testing run. The specific details of how to do +this are included in the new Collaborator [Onboarding guide][]. + +Ideally, the code change will pass ("be green") on all platform configurations +supported by Node.js (there are over 30 platform configurations currently). +This means that all tests pass and there are no linting errors. In reality, +however, it is not uncommon for the CI infrastructure itself to fail on +specific platforms or for so-called "flaky" tests to fail ("be red"). It is +vital to visually inspect the results of all failed ("red") tests to determine +whether the failure was caused by the changes in the Pull Request. + +## Additional Notes + +### Commit Squashing + +When the commits in your Pull Request land, they may be squashed +into one commit per logical change. Metadata will be added to the commit +message (including links to the Pull Request, links to relevant issues, +and the names of the reviewers). The commit history of your Pull Request, +however, will stay intact on the Pull Request page. + +For the size of "one logical change", +[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8) +can be a good example. It touches the implementation, the documentation, +and the tests, but is still one logical change. In general, the tests should +always pass when each individual commit lands on the master branch. + +### Getting Approvals for Your Pull Request + +A Pull Request is approved either by saying LGTM, which stands for +"Looks Good To Me", or by using GitHub's Approve button. +GitHub's Pull Request review feature can be used during the process. +For more information, check out +[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g) +or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/). + +After you push new changes to your branch, you need to get +approval for these new changes again, even if GitHub shows "Approved" +because the reviewers have hit the buttons before. + +### CI Testing + +Every Pull Request needs to be tested +to make sure that it works on the platforms that Node.js +supports. This is done by running the code through the CI system. + +Only a Collaborator can start a CI run. Usually one of them will do it +for you as approvals for the Pull Request come in. +If not, you can ask a Collaborator to start a CI run. + +### Waiting Until the Pull Request Gets Landed + +A Pull Request needs to stay open for at least 48 hours (72 hours on a +weekend) from when it is submitted, even after it gets approved and +passes the CI. This is to make sure that everyone has a chance to +weigh in. If the changes are trivial, collaborators may decide it +doesn't need to wait. A Pull Request may well take longer to be +merged in. All these precautions are important because Node.js is +widely used, so don't be discouraged! + +### Check Out the Collaborator's Guide + +If you want to know more about the code review and the landing process, +you can take a look at the +[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md). + +[approved]: #getting-approvals-for-your-pull-request +[benchmark results]: ../writing-and-running-benchmarks.md +[Building guide]: ../../../BUILDING.md +[CI (Continuous Integration) test run]: #ci-testing +[Code of Conduct]: https://github.com/nodejs/admin/blob/master/CODE_OF_CONDUCT.md +[guide for writing tests in Node.js]: ../writing-tests.md +[https://ci.nodejs.org/]: https://ci.nodejs.org/ +[IRC in the #node-dev channel]: https://webchat.freenode.net?channels=node-dev&uio=d4 +[Onboarding guide]: ../onboarding.md diff --git a/test/README.md b/test/README.md index d214f2fc10c246..bc2464bcfaecdf 100644 --- a/test/README.md +++ b/test/README.md @@ -6,7 +6,7 @@ For a detailed guide on how to write tests in this directory, see [the guide on writing tests](../doc/guides/writing-tests.md). On how to run tests in this directory, see -[the contributing guide](../CONTRIBUTING.md#step-6-test). +[the contributing guide](../doc/guides/contributing/pull-requests.md#step-6-test). ## Test Directories