Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: update contributing guidelines #10223

Merged
merged 16 commits into from
Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update the notes
  • Loading branch information
robert-zaremba committed Sep 29, 2021
commit 22232ea227f015b257df746eb1d2092dfe13c310
32 changes: 26 additions & 6 deletions CONTRIBUTING-GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ This document is an extension to [CONTRIBUTING](./CONTRIBUTING.md) and provides
Performance:
+ Avoid unnecessary operations or memory allocations.


Security:
+ Pay proper attention to exploits involving:
+ gas usage
Expand All @@ -40,27 +39,48 @@ Make sure your code is well tested:
+ Use both test cases and property / fuzzy testing. We use the [rapid](pgregory.net/rapid) Go library for property-based and fuzzy testing.
+ Do not decrease code test coverage. Explain in a PR if test coverage is decreased.


We expect tests to use `require` or `assert` rather than `t.Skip` or `t.Fail`,
unless there is a reason to do otherwise.
When testing a function under a variety of different inputs, we prefer to use
[table driven tests](https://github.com/golang/go/wiki/TableDrivenTests).
Table driven test error messages should follow the following format
`<desc>, tc #<index>, i #<index>`.
`<desc>` is an optional short description of whats failing, `tc` is the
index within the test case table that is failing, and `i` is when there
is a loop, exactly which iteration of the loop failed.
The idea is you should be able to see the
error message and figure out exactly what failed.
Here is an example check:

```go
<some table>
for tcIndex, tc := range cases {
<some code>
resp, err := doSomething()
require.NoError(err)
require.Equal(t, tc.expected, resp, "should correctly perform X")
```

## Quality Assurance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great to outline and document in the Repo, but I'm not sure it belongs in the CONTRIBUTING-GUIDELINES.md doc. Maybe its better in its own doc around QA practices and scope & team members & outcomes?

Or maybe a pinned issue similar to what we have for working groups?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a pinned issue? Do you think it will have a better visibility? For me issues are something that will have it's own life and will be closed eventually.


We are forming a QA team that will support the core Cosmos SDK team and collaborators by:
- Improving the Cosmos SDK and Regen Network QA Processes
- Improving the Cosmos SDK QA Processes
- Improving automation in QA and testing
- Defining high-quality metrics
- Maintaining and improving testing frameworks (unit tests, integration tests, and functional tests)
- Defining test scenarios.
- Verifying user experience and defining a high quality.
- We want to have **acceptance tests**! Document and list acceptance lists that are implemented and identify acceptance tests that are still missing.
- Acceptance tests should be specified in `acceptance-tests` directory as Markdown files.
- Supporting other teams with testing frameworks, automation, and User Experience testing.
- Testing chain upgrades for every new breaking change.
- Defining automated tests that assure data integrity after an update.

Desired outcomes:

- QA team works with Development Team
- QA is happening in parallel with Core Cosmos SDK development
- Releases are more predictable (0.40 and 0.43 were not predictable at all).
- QA team works with Development Team.
- QA is happening in parallel with Core Cosmos SDK development.
- Releases are more predictable.
- QA reports. Goal is to guide with new tasks and be one of the QA measures.


Expand Down
50 changes: 11 additions & 39 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ discussion or proposing code changes. To ensure a smooth workflow for all
contributors, the general procedure for contributing has been established:

1. Start by browsing [new issue](https://github.com/cosmos/cosmos-sdk/issues) and [discussions](https://github.com/cosmos/cosmos-sdk/discussions). If you are looking for something interesting or if you have something in your mind, there is a chance it was has been discussed.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
- Looking for a good place to start contributing? How about checking out some [good first issues](https://github.com/cosmos/cosmos-sdk/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22)?
2. Determine whether a GitHub issue or discussion is more appropriate for your needs:
1. If want to propose something new that requires specification or an additional design, or you would like to change a process, start with a [new discussion](https://github.com/cosmos/cosmos-sdk/discussions/new). With discussions, we can better handle the design process using discussion threads. A discussion usually leads to one or more issues.
2. If the issue you want addressed is a specific proposal or a bug, then open a [new issue](https://github.com/cosmos/cosmos-sdk/issues/new/choose).
Expand All @@ -39,23 +40,11 @@ contributors, the general procedure for contributing has been established:
to begin work.
5. To submit your work as a contribution to the repository follow standard GitHub best practices. See [pull request guideline](#pull-requests) below.


**Note: ** For very small or blatantly obvious problems such as typos, you are
not required to an open issue to submit a PR, but be aware that for more complex
problems/features, if a PR is opened before an adequate design discussion has
taken place in a GitHub issue, that PR runs a high likelihood of being rejected.

Other notes:

- Looking for a good place to start contributing? How about checking out some
[good first issues](https://github.com/cosmos/cosmos-sdk/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22)
- Please make sure to run `make format` before every commit - the easiest way
to do this is have your editor run it for you upon saving a file (most of the editors
will do it anyway using a pre-configured setup of the programming language mode).
Additionally please ensure that your code is lint compliant by running `make lint-fix`.
A convenience git `pre-commit` hook that runs the formatters automatically
before each commit is available in the `contrib/githooks/` directory.

## Architecture Decision Records (ADR)

When proposing an architecture decision for the SDK, please start by opening an [issue](https://github.com/cosmos/cosmos-sdk/issues/new/choose) or a [discussion](https://github.com/cosmos/cosmos-sdk/discussions/new) with a summary of the proposal. Once the proposal has been discussed and there is rough alignment on a high-level approach to the design, the [ADR creation process](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/PROCESS.md) can begin. We are following this process to ensure all involved parties are in agreement before any party begins coding the proposed implementation. If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/cosmos-sdk/tree/master/docs/architecture).
Expand All @@ -71,36 +60,20 @@ When proposing an architecture decision for the SDK, please start by opening an
- For core developers working within the `cosmos-sdk` repo, follow branch name conventions to ensure a clear
ownership of branches: `{moniker}/{issue#}-branch-name`.
- See [Branching Model](#branching-model-and-release) for more details.
- Please make sure to run `make format` before every commit - the easiest way
to do this is have your editor run it for you upon saving a file (most of the editors
will do it anyway using a pre-configured setup of the programming language mode).
Additionally please ensure that your code is lint compliant by running `make lint-fix`.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
A convenience git `pre-commit` hook that runs the formatters automatically
before each commit is available in the `contrib/githooks/` directory.
- Follow the [CONTRIBUTING GUIDELINES](CONTRIBUTING-GUIDELINES.md).

Code is merged into master through pull request procedure.

### Testing

Tests can be ran by running `make test` at the top level of the SDK repository.

We expect tests to use `require` or `assert` rather than `t.Skip` or `t.Fail`,
unless there is a reason to do otherwise.
When testing a function under a variety of different inputs, we prefer to use
[table driven tests](https://github.com/golang/go/wiki/TableDrivenTests).
Table driven test error messages should follow the following format
`<desc>, tc #<index>, i #<index>`.
`<desc>` is an optional short description of whats failing, `tc` is the
index within the table of the testcase that is failing, and `i` is when there
is a loop, exactly which iteration of the loop failed.
The idea is you should be able to see the
error message and figure out exactly what failed.
Here is an example check:

```go
<some table>
for tcIndex, tc := range cases {
<some code>
resp, err := doSomething()
require.NoError(err)
require.Equal(t, tc.expected, resp, "should correctly perform X")
```


### Pull Requests

Before submitting a pull request:
Expand All @@ -111,10 +84,9 @@ Then:

1. If you have something to show, **start with a `Draft` PR**. It's good to have early validation of your work and we highly recommend this practice. A Draft PR also indicates to the community that the work is in progress.
Draft PRs also helps the core team provide early feedback and ensure the work is in the right direction.
2. Follow the [CONTRIBUTING GUIDELINES](CONTRIBUTING-GUIDELINES.md).
3. When the code is complete, change your PR from `Draft` to `Ready for Review`.
4. Go through the actions for each checkbox present in the PR template description. The PR actions are automatically provided for each new PR.
5. Be sure to include a relevant changelog entry in the `Unreleased` section of `CHANGELOG.md` (see file for log format).
2. When the code is complete, change your PR from `Draft` to `Ready for Review`.
3. Go through the actions for each checkbox present in the PR template description. The PR actions are automatically provided for each new PR.
4. Be sure to include a relevant changelog entry in the `Unreleased` section of `CHANGELOG.md` (see file for log format).

PRs must have a category prefix that is based on the type of changes being made (for example, `fix`, `feat`,
`refactor`, `docs`, and so on). The *type* must be included in the PR title as a prefix (for example,
Expand Down