Hi Sascha,
On 08/11/2012 10:53 PM, Sascha Silbe wrote:
Dear Sugar Labs members,
== Executive Summary ==
The current review process isn't a good fit for the current Sugar
contributors and causes a lot of frustration on both sides. There are
better ways to reach the same goals. The two major changes to the review
process I'll propose in this email are making in-depth reviews by
independent senior developers optional and non-blocking as well as
accepting "no ceiling" and standards compliance patches into
mainline. Other work to address some of the goals of the previous review
process has already been going one for quite some time.
== Why we should rethink our current approach ==
I think it's time to take a step back and reconsider our current
approach for reviews and QA. For the past few months to years, upstream
reviews were primarily a QA measure. While hopefully many contributors
learned to be better developers, the focus clearly was on making sure
that only "good" patches were merged into mainline. That's the way many
(maybe most) community-driven Open Source projects work: Downstream
needs to show their contribution is of value to upstream (including not
having a negative impact on other parts of the user base) and does not
impair maintainability.
Sugar, however, currently isn't a community project: Most of the work is
done by a small number of organisations with a clear commercial
focus. They have deadlines breathing down their neck and need to care
about what their costumers want (who are the ones keeping these
organisations alive), not what's in the interest of the general user
base. The paying customers are what keeps these organisations alive and
the organisations in turn are what keeps Sugar Labs alive. The small
number and similar focus of the organisations means there's not enough
diversity to address a more general user base.
Sugar Labs is a community project. There are two major player (OLPC and
Activity Central) with paid developers but there are as well a lot of
contributors from the outside. Just to name a few in the Sugar
development team: Daniel Narvaez, Gary C Martin, Benjamin Berg... Of
course the paid stuff does have more hours for Sugar work per day but
that should not have any impact on the review process.
Tight deadlines and focus on the needs of paying customers doesn't
really mix with the style of reviews done by community-driven
projects. This regularly results in frustration on both sides.
I disagree here. A review is a review. There are two parties involved, a
contributor and a reviewer. The reviewer is interested in getting quick
feedback about his approach. The reviewer (maintainer) is interested in
keeping the quality of his code base clean and is interested in getting
more contributions from the contributor. To handle that scenario there
are guidelines.
We should think about what we want to achieve and how to best achieve
it. Maybe we still want reviews to take place, but in a different
manner. Maybe we'll just do something else and do away with reviews. But
simply continuing the current way is not a good idea.
== Goals ==
So what do we want to achieve? Some options:
1. Few obvious mistakes: Making sure the code doesn't contain a lot of
mistakes that could have been easily spotted by some other developer,
resulting in bugs affecting the user.
2. Few bugs affecting the user: Making sure there are few regressions
and new features work as expected.
3. Few hard to fix bugs: Making sure that the risk of introducing bugs
that are hard to diagnose and fix (usually race conditions) is low.
4. Maintainability: Making sure that the cost of doing code changes
(including QA as necessary for 1. and 2.) doesn't grow in the long
term.
5. Better developers: Constantly increasing our own and others'
abilities and knowledge.
== Means ==
There are various ways to achieve one or more of the goals above:
A. Eating our own dog food
B. Private reviews done amongst colleagues
C. Public reviews done amongst colleagues
D. Public in-depth reviews done by senior developers
E. Public short reviews done by senior developers
F. Requiring public short reviews by senior developers, usually from
within the same organisation, before pushing changes
G. Requiring public in-depth reviews by independent, senior developers
(AKA upstream maintainers) before pushing changes
H. Manual UI/UX tests
J. Automated tests (UI tests, unit tests, etc.)
K. Automated code checks (e.g. pylint, pep8)
L. Leveraging the community of upstream components
== Proposal ==
All of the means listed above have different trade-offs, there's no
silver bullet. Review bandwidth of senior developers is limited, manual
tests take so much effort that only a subset can be tested regularly and
automated tests needs to be maintained alongside the code base. We need
a good balance to arrive at a great product.
My current proposal would be:
I. Do private (B) or public (C) review amongst colleagues to address
goal 1 (few obvious mistakes).
II. Do public in-depth reviews by senior developers (D), but make them
optional (i.e. _not_ G) and non-blocking. Addresses goals 3 (few
hard to fix bugs), 4 (maintainability) and 5 (better developers).
III. Require public short reviews by some senior developer
(E). Addresses goals 1 and to a limited amount 2 (few bugs
affecting the user), 3 and 4.
This is actually how I always have seen it, especially if code of long
term contributors is queued to get in. The reviewer should focus in
those cases on the approach that is taken and have a look at the meat of
the patch. No need to block for example because of the language of the
description. If there is time it is welcome to check that as well. But
it is secondary. The important thing is: make sure to know what you can
do as a maintainer. If you have the time to do in-depth reviews for each
patch in a timely manner, great, if not better to make sure you get the
short-review right.
IV. Manual UI/UX (H) testing to the extent feasible. Addresses goal 2.
V. Implement automated UI and system tests (J). Strongly encourage
contributors to run them (successfully) before posting a patch
upstream. Where necessary contributors should extend the test
suite. Routinely run the tests against mainline across a large
number of supported systems (different distros, different hardware
including VMs, XO-* and Olidata JumPC). Addresses goals 1, 2 and to
some degree 3.
Sure, Daniel Narvaez have been doing great work on that end with the
buildbot and the Ui tests he is building. Of course this will help to
increase the quality.
VI. Accept patches into mainline that are likely to increase the number
of contributors using Sugar themselves (A) or to increase their
usage of Sugar, even if the patch doesn't directly benefit the XO
target user base. It should not have a negative impact on the XO
target user base, of course. This addresses goals 2, 3, 4 and 5.
When we setup SugarLabs the goal was to make Sugar available on other
platforms like the XO. This goal is still the case.
VII. Work on making Sugar more modular, using upstream components and
standard protocols or APIs as much as possible (L), allowing users
to mix and match components or simply configure them differently
(A) and reducing the footprint of bugs. This addresses goals 2, 3,
4 and 5.
Using standards and upstream components has been a goal as well. So we
just have to continue that road.
AFAICT I. is already being done. With Manuel Quiñones' appointment as
Glucose maintainer and his subsequent actions, we're evidently also
doing III. now. OLPC-AU and OLPC are doing IV., no change there
either. I've been working on implementing automated tests that can be
used for V., both on system tests (sugar-datastore test suite) as well
as UI tests (Gnome a11y based, not ready yet). Similarly, I've been
working on moving to standard protocols (e.g. XDG desktop entries for
Activities, ICCCM compliance) and upstream components
(e.g. gnome-session).
The two major changes are making in-depth reviews by senior developers
optional and non-blocking (II.)
But require public short reviews by some senior developer, correct?
as well as accepting "no ceiling"
(VI.) and standards compliance (VII.) patches.
But modularity comes at a cost. This should be decided on a case per
case basis.
Regards,
Simon
_______________________________________________
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
