14

At work I've been temporary lent to another team this quarter because they have too much work on their plate. I'm here to work on subject X for this team, that is kinda stand-alone but would benefit the team overall and was required by them.

The way we work with code in my company is that we have a repository by team (roughly) and that each commit has to go through a code review phase in gerrit first before in can be merged in the team's repo. The only people with the power to accept merging the pending commit in the repo are the people from the team. We also have the informal rule that no commit should be left un-reviewed more than 24 hours or so.

Now, in the past 10 days or so I have pushed 15+ commits, and got almost no feedback on any of them. At best, my initial commit was merged but I still have 14 commit unmerged that have been requiring code review for over a week.

What can I do to address this? The team I work with is composed of 3 people, one is senior enough to do the reviews but would prefer if the other juniors would ramp up and do the reviews. The other 2 juniors are really reluctant to do the review as they feel they can't understand the code enough due to lack of real knowledge in the language and as a result they won't take the responsibility to approve my code.

For the last ~5 days my status at the stand-up has been "I'm stuck due to no code reviews", and I've even booked a 30 min meeting to walk the team through the code and explain what I was doing in the reviews. Now I'm at a point where I won't do any further work as it's too complex to work on a giant stack of accumulated commits whose code could change.

Any idea on how to solve this? thanks!

  • 2
    What is the response from the team when you say you are stuck? Have you asked them to look at your code NOW? – Erik Oct 23 '18 at 20:22
  • 3
    Have you discussed this with your boss? – Mawg says reinstate Monica Oct 24 '18 at 6:19
  • "They feel they can't understand the code enough due to lack of real knowledge in the language" - Has the senior developer ever expressed concerns over this fact with the juniors? – user34587 Oct 24 '18 at 7:16
  • How this relates to your performance? If the commits are not merged/reviewed and are hanging there does it matter? If someone asks about your progress, is saying that you send your code for review a legitimate answer that gets you off the hook? – Konrad Oct 24 '18 at 13:23
  • @Konrad When your changes are accumulating into multiple pull requests, branching and merging become a heavy maintance, you start to have branches of branches because of some dependencies and also, the merge conflict is increased. Those elements affect your performance because it is an overhead from people who do not review your code. I agree with OP that at some point, it is better doing other tasks like training, spiking for futures features and helping co-worker that are overloaded so they can approve your PR :) – Sebastien DErrico Oct 24 '18 at 15:48
8

There are a couple of things to do in these scenarios, and I recommend doing both:

  1. Be proactive. Explain the benefit of what you're trying to convince them to do. In your case, explain that code reviews find silly mistakes and are a great way to share knowledge among co-workers. Edit: additionally it can provide training as @Upper_Case points out. Here's a longer list of code review benefits.
  2. Clearly explain the problem to a manager or someone who has the power to mandate a change. It should be really easy to explain to your manager (who loaned you to this team) and the manager of this team that you cannot do anything because no one will review your code. Don't get overly emotional, but make sure they understand that you cannot productively continue to work because future code is built on what you've already done (people familiar with software should easily understand this).

The last thing - which is more specific to your situation - is for you to write tests for your pending code in the meantime as a way to attempt to still contribute something while you are blocked.

  • 2
    +1 Tests describing the requirement is an excellent suggestion – Sebastien DErrico Oct 23 '18 at 20:45
  • 1
    Great answer! Another benefit of the code reviews would be that the coworkers that don't feel confident in their skills in that language would be able to develop them somewhat – Upper_Case-Stop Harming Monica Oct 23 '18 at 21:25
  • out of interest, what are 'tests' in this scenario? – Kilisi Oct 24 '18 at 6:00
  • 1
    @Kilisi tests are code that would make sure the pending commits are working properly. – dbeer Oct 24 '18 at 18:52
  • @dbeer tests should be commited alongside for review, not after. they also exemplify how the code performs and what not – André Werlang Mar 8 at 1:37
2

You're not stuck because of lack of reviews, you are stuck because no one in that team has the time or the knowledge to review your work. Reviews for the sake of following procedure are the complete opposite of what Agile should teach. Please don't fetishise the tools.

Two possible approaches:

First: Ask the senior dev (assuming he has the necessary knowledge) to review your code in a time-boxed manner (5 mins/commit or whatever s/he pleases). If that gets denied goto 2:

Second: Merge your code and wait for defect reports.

The whole point of a review is not to tick a checkbox but to spot possible defects before they end up in the target branch.

If no one has the time or knowledge to do it for you and it's critical that your fixes make it to the customer, batten down the hatches as much as you can (A.K.A unit test the sh*t out of it), force push it and wait for bugs.

  • 2
    I .-1 that because it seems to be surreal wrong. If noone is qualified to do them, this still is not HIM fetishising the reviews, it is a fundamental problem created by bad management, which is not HIS ti fix. Been there too - unless the manager removes the fetisch, this is a requirement. Happens. – TomTom Oct 24 '18 at 14:10
  • 4
    +1 for putting pragmatism over dogma. – Jim Clay Oct 24 '18 at 14:54
  • No, don't break procedure to merge code (and doubly so not as a new and temporary team member), that's just going to make an even bigger mess for everyone, especially at the most likely reason the senior dev isn't reviewing the code is that they're either in the midst of something thorny and higher priority, or there are fairly deep seated disagreements over the proposed changes they don't have time to dig into yet. – Chris Stratton Mar 8 at 17:17
  • @ChrisStratton If they don't want to do code reviews it's probably because they don't understand the value of it. To them, you're like a Lumbergh who want them to put cover sheets on TPS reports. This is why some companies ask about these things in the interview process. Good companies value experience and understanding of methodologies rather than just X years of experience with language Y and framework Z. – dan-klasson Mar 8 at 22:55
  • 1
    @dan-klasson - I think it's more likely that they don't see a current benefit from the code to be reviewed, ie, the OP's project is a bit tangential to what this busy team is currently worried about, so while it's needed eventually and they probably care a lot what it looks like, it's not something the senior folks want to deal with right now. Dumping an unauthorized and unwelcome merge smack dab in the middle of what they are worried about is great way to make enemies. OP can always ask "mind if I just merge this?" but should definitely not just do it. – Chris Stratton Mar 8 at 22:57
0
  1. Explain the importance of code reviews to the juniors.
  2. Train the juniors on how to perform code reviews.
  3. Schedule in-person meetings for code reviews with your reviewers. I usually send Google calendar invites.
0

Others responses here seem to be focusing on the team. This sounds like a management problem. The team has 'agreed' to the 24 commitment on code reviews, yet they are not doing them.

It is time to take this to management, if you have one, and indicate the team is blocking your work because they refuse to do code reviews, detail your efforts to get this done and ask for help to push this along.

If this is a true agile team with little to no management oversight, it's time for you to take charge. At standup you need to say:

I am blocked on this. We CANNOT move forward on ANY OTHER WORK until this is resolved. We are not leaving this room until the code reviews are done, this is all hands on deck, everyone is going to participate.

The team has agreed this is part of the working agreement so you are fair to demand this.

  • One can certainly ask, but it's necessary - especially as a new and temporary team member to be open to being told that other work has higher priority. While that may be a process and resource utilization failure, it can also be a real reality. – Chris Stratton Mar 8 at 17:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.