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
[email protected]
https://mail.kde.org/mailman/listinfo/owncloud
_______________________________________________
Owncloud mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/owncloud
