Skip to content

Commit f0069c6

Browse files
authored
Merge pull request element-hq#16980 from vector-im/jryans/code-quality
Add code quality review policy
2 parents 947c913 + 15fe709 commit f0069c6

File tree

1 file changed

+39
-2
lines changed

1 file changed

+39
-2
lines changed

docs/review.md

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,43 @@ When reviewing code, here are some things we look for and also things we avoid:
5858
* Assign issues only when in progress to indicate to others what can be picked
5959
up
6060

61+
## Code Quality
62+
63+
In the past, we have occasionally written different kinds of tests for
64+
Element and the SDKs, but it hasn't been a consistent focus. Going forward, we'd
65+
like to change that.
66+
67+
* For new features, code reviewers will expect some form of automated testing to
68+
be included by default
69+
* For bug fixes, regression tests are of course great to have, but we don't want
70+
to block fixes on this, so we won't require them at this time
71+
72+
The above policy is not a strict rule, but instead it's meant to be a
73+
conversation between the author and reviewer. As an author, try to think about
74+
writing a test when making your next change. As a reviewer, try to think about
75+
how you might test the area of code you are reviewing. If the reviewer agrees
76+
it would be quite difficult to test some new feature, then it's okay for them to
77+
accept the change without tests for now, but we'd eventually like to be more
78+
strict about this further down the road.
79+
80+
If you do spot areas that are quite hard to test today, please let us know in
81+
[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org). We can
82+
work on improving the app architecture and testing helpers so that future tests
83+
are easier for everyone to write, but we won't know which parts are difficult
84+
unless people shout when stumbling through them.
85+
86+
We recognise that this testing policy will slow things down a bit, but overall
87+
it should encourage better long-term health of the app and give everyone more
88+
confidence when making changes as coverage increases over time.
89+
90+
For changes guarded by a feature flag, we currently lean towards prioritising
91+
our ability to evolve quickly using such flags and thus we will not currently
92+
require tests to appear at the same time as the initial landing of features
93+
guarded by flags, as long as (for new flagged features going forward) the
94+
feature author understands that they are effectively deferring part of their
95+
work (adding tests) until later and tests are expected to appear before the
96+
feature can be enabled by default.
97+
6198
## Design and Product Review
6299

63100
We want to ensure that all changes to Element fit with our design and product
@@ -79,5 +116,5 @@ easily.
79116

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

0 commit comments

Comments
 (0)