[Owncloud] Pull request testing

[Jan-Christoph Borchardt]

Yeah, Jörn makes the right point here. Instead of only thumbs upping –
which doesn’t indicate what you did (test in Firefox&Chrome and maybe IE,
or you just like it, or you just looked at the code). So yes, just add a
comment next to the thumbs, clarifying why you give the thumbs up.

Of course there are exceptions where there is only one reviewer able to do
it, but then it just needs to be clarified. We won’t go to hell if only one
person tested it and another one only looked at the code, but we should
make sure that, well, actually at least one person tested it.

Thanks folks!


On Wed, Mar 20, 2013 at 11:58 AM, Arthur Schiwon <blizzz at owncloud.com>wrote:

> On 03/20/2013 10:57 AM, Klaas Freitag wrote:
>
>> On 19.03.2013 22:28, Frank Karlitschek wrote:
>>
>>> I completely agree
>>>
>>
>> I agree partly. Of course all code reviews should be done as accurate as
>> possible, and that involves patch applying and testing.
>>
>
> I agree with Klaas
>
>
>
>> But:
>>
>>  A problem is that I often see: »thumbs up, code looks good but I
>>>> didn’t test«
>>>>
>>>> Now I’m sure you’re all code wizards who can parse and render code in
>>>> their head … but actually checking out the merge request, open
>>>> ownCloud, in an actual browser (or two), and click around a bit if it
>>>> really *works* is the crucial part here.
>>>>
>>> I don't think that testing of the code changes is always so easy that it
>> could be verified by "clicking around a bit". I also don't think that
>> code reviews can and should replace a qa step which is an important part
>> of bug fixing for which a dev is responsibility.
>> Code review here is more like a smoke test where people help to identify
>> obvious problems, code errors or design pitfalls.
>>
>
> Also:
>
> Some PRs touch setups or configurations that are not used very often. In
> terms of LDAP there is not a single core developer but me who do run a
> server. While oftenly even two, OpenLDAP and AD, are necessary.
>
> I really benefit from people looking at the code, what works pretty well
> for smaller PRs, but not so much for PRs with big changes.
>
> And it is not only LDAP, but also files_external or things related to IIS
> for instance.
>
> Cheers
> Arthur
>
>
>
>> We should pay attention to not end up with the result that people stand
>> away from reviews because we put too much responsibility on the reviewer.
>>
>> Klaas
>>
>> ______________________________**_________________
>> Owncloud mailing list
>> Owncloud at kde.org
>> https://mail.kde.org/mailman/**listinfo/owncloud<https://mail.kde.org/mailman/listinfo/owncloud>
>>
> ______________________________**_________________
> Owncloud mailing list
> Owncloud at kde.org
> https://mail.kde.org/mailman/**listinfo/owncloud<https://mail.kde.org/mailman/listinfo/owncloud>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/owncloud/attachments/20130320/06350535/attachment.html>