On Wed, Aug 18, 2010 at 11:54, Sascha Silbe
sascha-ml-ui-sugar-de...@silbe.org wrote:
Excerpts from Tomeu Vizoso's message of Wed Aug 11 12:22:00 +0200 2010:
any code contributor is expecting that their patches will be tested by
the reviewer?
A contributor should test their code to the best of their ability.
A reviewer can, but doesn't have to test the code. A tester can, but
doesn't have to review the code. With the mailing list based review model,
this is expressed as the separate Reviewed-By and Tested-By tags.
Because of this specific commit, file transfers have been broken since
early this year and it's obvious that this code wasn't tested at all:
http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/11828796
Me being the original author of the patch, it's obvious that the code was
tested rather well before submitting it for review. But
a) I'm not perfect, so some code paths were not exercised before the
initial submission for review
b) it took many months before the patch was accepted, so it bitrotted.
Simon probably had to solve a number of conflicts; also some changes
in the surroundings of the patch might have been ignored by the VCS.
All this is an error-prone process that requires any test to be
re-run / re-done.
This is a very nice example of
a) how the long time patches wait in the review queue and the cumbersome
process of submitting them to Trac is negatively affecting the project
(I remember fixing an issue in this very patch related to file sharing,
but probably never submitted an updated patch to Trac because it was
rejected anyway back then).
b) why automated testing is crucial. (I don't think I need to elaborate
that point, it should be obvious from the above if it wasn't obvious
before already.)
Given the current poor state of our testing efforts,
The best way to get Sugar tested well (besides automatic tests which only
catch specific cases) is by eating our own dog food. Of course, this
requires us to accept some high ceiling patches (that no deployment
will call for) instead of just low floor ones. This might increase the
risk of breaking something (or raising the floor), but IMO is more than
offset by the better testing and increased number of motivated
contributors we get.
You make good points, but the bit about dogfooding doesn't hold as
well because I, for example, don't ever use file transfer in GNOME.
In any case, I don't think you are saying that if I propose a patch
that changes code in the file transfer path I shouldn't have tested
file transfers?
Btw, I've been looking at AT-SPI with eyes on accessibility and testing today.
Regards,
Tomeu
Please consider this mail food for thought, not a direct retort to your
complaint about the breakage. That I was the one who authored this
original patch is merely a detail showing that even careful and
experienced contributors (as I consider myself to be) can't be relied upon
to not make mistakes. We need to accept the fact that humans are imperfect
and cater for it by tuning our processes and goals.
Sascha
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
