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.

Sign up for GitHub

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

Jump to bottom

Add code quality review policy #16980

Merged
merged 4 commits into from
Apr 23, 2021
Merged

Add code quality review policy #16980

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
001b15b
Rewrap review guide
jryans Apr 16, 2021
c2e87dd
Add code quality review policy
jryans Apr 16, 2021
b0c4389
Change `#element-dev` reference to a link
jryans Apr 19, 2021
15fe709
Add section on quality when guarded by a feature flag
jryans Apr 23, 2021
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
33 changes: 31 additions & 2 deletions docs/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,35 @@ When reviewing code, here are some things we look for and also things we avoid:
* Assign issues only when in progress to indicate to others what can be picked
up

## Code Quality

In the past, we have occasionally written different kinds of tests for
Element and the SDKs, but it hasn't been a consistent focus. Going forward, we'd
like to change that.

* For new features, code reviewers will expect some form of automated testing to
be included by default
* For bug fixes, regression tests are of course great to have, but we don't want
to block fixes on this, so we won't require them at this time

The above policy is not a strict rule, but instead it's meant to be a
conversation between the author and reviewer. As an author, try to think about
jryans marked this conversation as resolved.
Show resolved Hide resolved
writing a test when making your next change. As a reviewer, try to think about
how you might test the area of code you are reviewing. If the reviewer agrees
it would be quite difficult to test some new feature, then it's okay for them to
accept the change without tests for now, but we'd eventually like to be more
strict about this further down the road.

If you do spot areas that are quite hard to test today, please let us know in
#element-dev:matrix.org. We can work on improving the app architecture and
jryans marked this conversation as resolved.
Show resolved Hide resolved
testing helpers so that future tests are easier for everyone to write, but we
won't know which parts are difficult unless people shout when stumbling through
them.

We recognise that this testing policy will slow things down a bit, but overall
it should encourage better long-term health of the app and give everyone more
confidence when making changes as coverage increases over time.

## Design and Product Review

We want to ensure that all changes to Element fit with our design and product
Expand All @@ -79,5 +108,5 @@ easily.

Before starting work on a feature, it's best to ensure your plan aligns well
with our vision for Element. Please chat with the team in
[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org) before you
start so we can ensure it's something we'd be willing to merge.
[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org) before
you start so we can ensure it's something we'd be willing to merge.