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

[Tomeu Vizoso]

On Wed, Aug 18, 2010 at 11:54, Sascha Silbe
<sascha-ml-ui-sugar-devel at 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 at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel
>
>