From 001b15b856d41ecdc3b9b66e9d05b751e6844d77 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 16 Apr 2021 16:02:46 +0100 Subject: [PATCH 1/4] Rewrap review guide --- docs/review.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/review.md b/docs/review.md index c08ab10818..8e6dd7584f 100644 --- a/docs/review.md +++ b/docs/review.md @@ -79,5 +79,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. From c2e87dde85e0929bc5e01180e26074a5b70b3a9e Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 16 Apr 2021 18:53:18 +0100 Subject: [PATCH 2/4] Add code quality review policy This adds a new review policy which encourages tests for new features. As with everything, we'll continue to tune this based on feedback. --- docs/review.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/review.md b/docs/review.md index 8e6dd7584f..36ca1ffcca 100644 --- a/docs/review.md +++ b/docs/review.md @@ -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 +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 +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 From b0c43890af364db2b0a0c2ef6ff85900d984f023 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Mon, 19 Apr 2021 17:02:11 +0100 Subject: [PATCH 3/4] Change `#element-dev` reference to a link --- docs/review.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/review.md b/docs/review.md index 36ca1ffcca..2aab77ddb7 100644 --- a/docs/review.md +++ b/docs/review.md @@ -78,10 +78,10 @@ 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 -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. +[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org). We can +work on improving the app architecture and 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 From 15fe7091ff0adb7416968ed720c49b89cf9f510a Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 23 Apr 2021 17:46:08 +0100 Subject: [PATCH 4/4] Add section on quality when guarded by a feature flag --- docs/review.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/review.md b/docs/review.md index 2aab77ddb7..b84f422dca 100644 --- a/docs/review.md +++ b/docs/review.md @@ -87,6 +87,14 @@ 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. +For changes guarded by a feature flag, we currently lean towards prioritising +our ability to evolve quickly using such flags and thus we will not currently +require tests to appear at the same time as the initial landing of features +guarded by flags, as long as (for new flagged features going forward) the +feature author understands that they are effectively deferring part of their +work (adding tests) until later and tests are expected to appear before the +feature can be enabled by default. + ## Design and Product Review We want to ensure that all changes to Element fit with our design and product