[Sugar-devel] Why automated testing and "dogfooding" is crucial (was: Re: code submitted for review should have been tested)

[Sascha Silbe]

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.


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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100818/a15d54dc/attachment.pgp