Patch review

[denise]

So, right now our major bottleneck in the dev process is code review! We're doing really well with patches being coded, and really well with bringing in new developers and getting them interested and working on things, but we've been having real problems finding people to do code review, and almost all the time it winds up being the people who are committing patches who do all the reviews.

This is a bad thing, for a few reasons: it means the committers burn out more quickly; everyone has different review styles, and notices different things, so having only one or two people doing the bulk of reviews leads to more things being missed; it slows down the turnaround time on commits; it means the committers are mostly working on reviewing and committing so they don't get as much time to code themselves; it means the committers feel rushed and pressured to get through the queue, because they know that nobody else is doing it, and that doesn't make for good work; and it means the rest of the dev team is losing out on the chance to learn from the process of reviewing someone else's code. I'd really like to see more peer review going on, with people reviewing each other, so that the committers have more of a chance to commit things that have already been reviewed by others instead of doing the bulk of the review themselves.

Ideally, I think we should be at around 60-70% peer review, with the remaining bits being committer review, and right now we're pretty much at 0%.

There are a few things I'd like to try to encourage people to do more peer reviewing, but before I do any of that, I figured I'd ask you guys some questions about doing reviews. Behind the cut is a poll, and I'd also love to hear any of your thoughts in comments. Poll answers are viewable in specific to nobody but maintainers of the comm, and if you have a comment that you'd like to leave, but don't feel comfortable doing so logged-in, anon commenting is enabled.

Poll #3965

Open to: Registered Users, detailed results viewable to: Just the Poll Creator, participants: 23

I don't do patch reviews because:

I don't like doing it
2 (9.5%)

I don't feel qualified
18 (85.7%)

I'm scared I might miss something and cause problems
12 (57.1%)

The instructions aren't clear enough
0 (0.0%)

I didn't think I was allowed to
8 (38.1%)

I don't have the time
6 (28.6%)

I'd rather be coding
4 (19.0%)

It's too hard to find things that need review
2 (9.5%)

I just generally feel intimidated by the thought
12 (57.1%)

I don't know what to look for
14 (66.7%)

I feel bad about being critical
3 (14.3%)

When I spot flaws in a patch, I want to fix them myself
3 (14.3%)

I find it too difficult to explain what's wrong with a patch
1 (4.8%)

I just keep forgetting
5 (23.8%)

I don't feel like there's enough social/emotional reward
1 (4.8%)

(Something else I will explain in comments)
1 (4.8%)

I would do more code review if:

I knew I wasn't going to be the only person to review the patch before commit
15 (65.2%)

There were more regular reminders to do reviews
6 (26.1%)

There were more clear instructions about what to look for when reviewing
11 (47.8%)

I could do partial reviews (review for style vs functionality vs efficiency/security)
13 (56.5%)

There were more public thanks/praise for reviewers
2 (8.7%)

There were tangible reward for reviewing (ie, DW points)
1 (4.3%)

There were classes/tutorials on how to do a review
10 (43.5%)

It were easier to apply and test patches
5 (21.7%)

It were easier to find patches that need review
4 (17.4%)

There were regularly scheduled group/peer "review queue whacks"
9 (39.1%)

I had a "review partner" to go over patches together
12 (52.2%)

Patches in the queue were more easy to identify as major vs minor
5 (21.7%)

I got better feedback on the quality of my reviews
3 (13.0%)

(Something else I will explain in comments)
3 (13.0%)

Comments

[deborah]

I think my other biggest problem, besides not feeling qualified, is that in some cases I disagree enough with the coding style that we use (I tend more Conway PBP and less terse) that I don't think I would be a good reviewer. I suspect that most of my comments would be COMMENT NEEDED HERE as a way of fighting back my urge to tell people not to use postfix conditionals.

But the biggest problem is that I don't feel qualified. I'm not particularly a hotshot developer in the first place, and I'm not saying that out of impostor syndrome. I'm saying that out of a very well-developed sense of reality.

[dancing_serpent]

I've only just started patching. Give me a bit of time - when I'm more experienced with that I'll naturally come to the point where I'll want to learn and do more.

[pauamma]

One of the "I don't do reviews" items is kinda applicable to me - I don't do many reviews because I don't have much time, and what time I have for reviews is pretty much eaten up by anarres and occasional reviewing of complex, scary patches on low-numbered tickets.

[allen]

I marked "It's too hard to find things that need review" not because I can't find things that need review, but because it's too much of a pain to do for something that I don't really like doing in the first place. I mean, I'll go out of my way to find interesting bugs to fix, but if after spending five minutes looking at the review queue I don't find anything to review, I'll use that as an excuse to move on to other things.

The biggest problem from my perspective is that I can't tell which bugs need a first review and which ones are on their fifth round of feedback. I'm much more likely to take a new patch where nobody else has read and understood the bug, set up test cases, etc., and where I'm not going to be stepping on another reviewer's toes. If I could just do a search for bugs that need review and whose QA Contacts are Unassigned (or some something similar, showing that nobody else has reviewed them yet), then I'd be much more likely to look at that list and take something from there.

It might also be nice to separate out QA (does this actually fix the bug? did it break anything else?) from code review (does the code work efficiently? does it follow our coding standards?). It's possible that we could find more people who are comfortable doing one but not the other.

[shadowspar]

Good point! I was wondering about this very same thing -- how much of the code review is meant to be, well, code review, and how much is meant to be full-blown, all-scenarios-on-the-table testing.

[suncat]

Thank you. I've just started learning the whole patching process here. In my experience, there is a separation between QA/test activities and code review activities. I haven't understood how the two activities are merged or separate here at DW.

[denise]

We don't really have a formal QA environment, although we've often talked about what the best way to build a user-facing 'beta' site that people could opt into and do testing on, mostly because it's hard to simulate production conditions in development environments and a lot of bugs only turn up in production. Instead, we have automated tests and both the developer who's working on the bug and the reviewers/committers are expected to include regression testing as part of their dev work.

(Also, welcome! Have you been given the rundown of all the resources available? Also, have you been pointed at irc? It's irc.freenode.net, channel #dreamwidth.)

[suncat]

I understand about the issues of setting up a formal QA environment. I guess I've been more concerned/confused about the separation of "duties". As in, if I'm writing a patch I do a certain type of testing. Then if I'm a tester of someone else's code, I do other types of testing. In my experience, a code reviewer only looked at and commented on code but doesn't do actual running of or testing of the code. Hence, my confusion here but I think I'm coming to understand the organization here.

Thanks for the welcome. 900degrees was giving me a ton of help last month and I made a lot of progress getting started. Then real life intruded again. I will get back on track. I will! Thanks for the IRC reference. Hadn't seen that yet, will check it out.

[thorfinn]

[X] I'm lazy and haven't gone looking for DW development process documentation yet, so I don't know enough about the process and what does or doesn't exist to contribute. :-)

If I *were* to do code reviews, I would mainly need a clear and detailed style guide. Especially so if the style guide isn't:

1. Use Perl::Critic and Perl::Tidy as per Damian Conway's Perl Best Practices book.

Judging by comments, that's not the style guide, presumably because the DW style is inherited from the LJ Style and that was created well before PBP (and Perl::Critic and Perl::Tidy) existed.

I suspect that considering whether or not to adopt that as the future style guide is something that should be discussed.

The usual practice with large existing codebases is to not go through and modify existing code - that's obviously too much pointless work. What is typically done is that if you need to edit a code file, the first step is to run Perl::Tidy and Perl::Critic, fix everything up, then get on with editing.

That automation alone catches a very large amount of "style" issues without requiring extra human review, which is a huge win. It leaves more time for humans to review things that humans are better at than computers, such as whether you should be using a different algorithm, or different approach to the problem, or whether the fix actually fixes the problem, etc.

[thorfinn]

I forgot to say, first pass is to tidy/critic the code and commit that as a separate "just tidying" commit.