42

We are trying to switch to a continuous integration environment but are not sure when to do code reviews. From what I've read of continuous integration, we should be attempting to check in code as often as multiple times a day. I assume, this even means for features that are not yet complete.

So the question is, when do we do the code reviews?

We can't do it before we check in the code, because that would slow down the process where we will not be able to do daily checkins, let alone multiple checkins per day.

Also, if the code we are checking in merely compiles but is not feature complete, doing a code review then is pointless, as most code reviews are best done as the feature is finalized. Does this mean we should do code reviews when a feature is completed, but that unreviewed code will get into the repository?

Improve this question
3
  • When it comes to checkins/pushes, most places have one major rule: Don't break the build! I.e. don't check in something that won't build. Other than that most places I been to want small and confined checkins, but never said anything about the amount.
    – Some programmer dude
    Nov 8, 2011 at 3:01
  • but when does the code review happen, before you checkin, or when your done with the feature? Does that mean you have code that hasn't been reviewed checked in, and that you fix any issues found by the review after?
    – SpecialEd
    Nov 8, 2011 at 3:22
  • It varies, but most places want to do code review on private branches before being merged into one of the main branches,
    – Some programmer dude
    Nov 8, 2011 at 3:28

11 Answers 11

Reset to default
17

IMO, you should review code before it gets published to mainline so that mainline only ever has the highest quality code.

OTOH, a good point could be made that 'why bother reviewing if the CI test automation hasn't run on it?', so perhaps the best would be to give developers each a private branch that the CI server will build and test for them. That way they first commit and push there, then once it passes get it reviewed, then merge to mainline (where it will get another run through the CI server).

You should definitely review non feature-complete code to make sure that scaffolding for future features is in place, or at least that there's nothing being put in that would prevent said future features from being implemented.

Also, note that code reviews don't have to be slow, or synchronous - a tool like gerrit or reviewboard or the like can make them asynchronous and fairly painless.

(Full disclosure: I used to work for SmartBear, makers of Code Collaborator, a code review tool)

Improve this answer
8
  • 5
    Codereview-by-email is poor practice (though better than nothing, admittedly) because it's hard to tell when the review is 'done'. Get a real code-review tool like gerrit or reviewboard and use it and stop emailing patches around :)
    – pjz
    May 25, 2012 at 21:30
  • 1
    Still, I don't think that's an ideal process, regardless of DVCS or not. One of the necessities of code review is not just to look at the code, but to actually run the code or automatically test the code and see what happens. You can't do that with just a series of diffs.
    – Jordan
    May 26, 2012 at 2:41
  • 2
    +1 for the suggestion that reviews should be carried out after automated tests have been run.
    – William Payne
    May 30, 2012 at 15:40
  • 1
    Jordan: real codereview tools (gerrit, etc) provide more than just diffs - the let you read all the context so you can figure out what the code change is actually affecting. If need be you can, yes, download the patch and build it, but since it's all going through CI anyway it's presumed that errors that can be caught by automation will be, so the concentration is more on maintainability and edge cases that the automation or casual testing might not catch.
    – pjz
    Jun 20, 2012 at 3:47
  • 1
    Isn't one of the points of CI to sync early and often with mainline? Your approach would delay syncing which has numerous drawbacks.
    – Jacob R
    Sep 11, 2012 at 7:34
12

Here is the extract from continuous delivery author:

Jez Humble Writes as:

I am currently writing a blog post on this topic. The short answer is this:

  • The best way to review code is through pair programming
  • It's a bad idea to gate merge to mainline - by creating a separate branch, for example - on a formal review process. This inhibits continuous integration (the best way of reducing the risk of bad changes, which is what you are really aiming to achieve).
  • I think Gerrit is a nice tool, but it should be used after check- in (that's how it's designed, in fact). Part of the job of the senior developers is to review all check-ins. They could, for example, subscribe to a feed.

To summarize: code review is good. So good, we should be doing it continuously, through pair programming and reviewing commits. If a senior dev finds a bad commit, she should pair with the person who committed it to help them fix the problem.

Gating merge to mainline on a formal review is bad, and creating branches to do so is extra bad, for the same reason that feature branches are bad.

Thanks,

Jez.

original link is : https://groups.google.com/forum/#!msg/continuousdelivery/LIJ1nva9Oas/y3sAaMtibGAJ

Improve this answer
11

Set up pair programming?

All of the code is reviewed as it is being typed without extending the process or introducing another step.

Improve this answer
5
+50

I don't know if it is the best way to do it... but I'll explain how we do it. One or more developers work on a given branch and commit their code as often as they can in order to avoid wasting time on a merge that would not have happened otherwise. Only when the code is ready is it committed into the head. Now that's for the commits and branch/head thing.

As for the code review, we use Sonar as our continuous integration tool (and Maven/Jenkins for interacting with Sonar) for providing us with fresh test results, code coverage, and automatic code review every morning (builds are done nightly) so that we developers can spend a maximum of one hour every morning to fix their problems/code smells. Each developer takes responsibility (being proud too!) for the feature he is writing. Now, that's automatic code review, which is great to find potential technical/architectural problems, but what is more important is to test if these new implemented features are doing what the business wants them to do, properly.

And for that, there are two things: integration tests and peer code review. Integration tests help in being reasonably confident that the new code does not break the existing code. As for the peer code review, we do it on Friday afternoons, which is a bit more relaxed time to do that :-) Each developer is assigned to a branch he does not work on, takes some time to read the requirements of the new feature first, and then checks what has been done. His most important job is to make sure that the new code works as expected given the requirements, does not break our own "rules" (use this object for that, and not that one), is easy to read, and that it allows for easy extension.

So we have two code reviews, one automatic and one "human" and we try to avoid committing unreviewed code into the HEAD branch. Now... It does happen sometimes for various reasons, we're far from perfect, but we try to keep a fair balance between quality and cost (time!)

@pjz provides a good answer too, and he mentions code review tools. I've never used any, so I can't say anything about that... although I have been tempted in the past to work with Crucible since we're already using JIRA.

Improve this answer
3
  • Interesting idea that reviews should be scheduled for a particular time/day...
    – William Payne
    May 30, 2012 at 15:42
  • @WilliamPayne thank you. Another thing that works for us is to schedule code review meetings, so it's clearly visible on the calendar that we are "busy". It helps in warning people that although we're here ... we're in fact not :-)
    – Jalayn
    May 30, 2012 at 19:58
  • > Only when the code is ready is it committed into the head. If this means that it routinely takes more than a day for the code to be committed into the head / main / master branch then it's not continuous integration.
    – bdsl
    Sep 5, 2022 at 15:46
4

I think the main concept that will help is that of a "Staging" area.

Yes, you don't want to check in code that's broken. But you also should be checking in code frequently. Does this imply perfection? ;) No. Just use multiple areas and a DVCS like git.
This way you make changes (locally) and commit them frequently as you test and develop until the tests pass. Then you push to a staging area for code review.

You should then be pushing from Staging to other QA efforts such as browser tests and user tests. Finally you may go to a volume testing area, then finally production.

There are also workflows within this such as everyone working on the main branch or using individual branches for all efforts.

Continuous integration itself can also happen at multiple levels. It can be local to a developers machine 'until the tests pass' and it can also be in staging and qa areas for when code goes to them.

Improve this answer
3

Decouple code review and continuous integration!

Why you combined them?

Improve this answer
2

I really-really-really think you would need a DVCS (e.g. mercurial, git) to do this naturally. With a CVCS you would need a branch and hope to whatever god you have there is no merging hell.

If you use a DVCS you can tier the integration process so that code already makes it reviewed before it arrives to the CI server. If you don't have a DVCS, well, the code will arrive your CI server before reviewed unless code-reviewers review the code on each developers' machine before they submit their changes.

A first way to do it, specially if you don't have repository management software that can help publish personal repositories (e.g. bitbucket, github, rhodecode), is to have hierarchical integration roles. In the following diagrams, you can have lieutenants review developers' work, and have the dictator as the main integrator reviewing how lieutenants merged the work.

enter image description here

Another way to do it if you have repository management software, is to use a workflow like the following:

enter image description here

Repository management software typically helps emit notifications when there is activity in repositories (e.g. email, rss) as well as allowing pull-requests. Code-review can organically happen during pull requests, as pull requests typically make people engage in conversations to have the code integrated. Take this public pull-request as example. The integration manager can actually not allow code to arrive to the blessed repository (aka "central repository") if the code needs to be corrected.

Most importantly, with a DVCS you can still support a centralized workflow, you don't need to have another fancy-awesome workflow if you don't want to... but with a DVCS you can separate a central development repository from the CI server and give someone the authority to push the changes from the dev repo to the CI repo once a code-review session has been done.

P.S.: Credit for the images go to git-scm.com

Improve this answer
1
  • 1
    The folks at github use pull requests to do code reviews and it seems to work well according to Scott Chacon, Zach Holman and others.
    – Spoike
    May 30, 2012 at 6:55
1

Why not have more than one repository? One for "daily" work, driving a continuous integration server, running all the unit tests & integration tests to get the nice tight feedback loop, and another for "stable" work, where commits are less frequent, but have to go through review.

Depending on the path that changes take as they move through the system, this can end up being a bit of a complex solution, and might work best when using tools like Git, or Mercurial Queues, (caveat: I have used neither in anger) but lots of organizations do something similar.

Improve this answer
1

We use git flow for our repositories, and we do our code reviews when it comes to merging into the develop branch.

Anything in develop is complete, deployable and code reviewed.

We also have CI set up for our develop and master branches.

Improve this answer
1
  • Git flow is not really compatible with the practice of continuous integration. CI is something you do, not something you set up.
    – bdsl
    Sep 5, 2022 at 15:48
1

Does this mean we should do code reviews when a feature is completed, but that unreviewed code will get into the repository?

Well above is the way I've seen it done in at least three projects that were intensively using continuous integration and per my recollection it worked like a charm. This practice is known as post-commit code reviews - search web for this term if you're interested in details.

  • On the other hand, the only case when I've been in project trying to "marry" pre-commit code reviews with CI turned out rather painful. Well when things went 100% smoothly, it was OK - but even infrequent interruptions (like when main and backup reviewers were both unavailable for say a few hours) made noticeable stress. I also noticed that team morale somewhat suffered - there were a bit too much conflicts.
Improve this answer
-1

First, we should clarify the concept of "continuous integration". In traditional developing methods, continuous integration means we can integrate and build our source code repository everyday, which will avoid the pitfalls of "integration hell". The code reviews are always between the period of Coding and Unit testing. We must guarantee the code merging to the branch can compile with no errors. There seldom has the situation that parts of the feature merges to the branch because it is difficult to handle the coherence of interface and compile errors.

Continuous Integration is popular in the Extreme Programming process. Test-driven development adds Pair programming, which is actual part of a code review process, making continuous integration easy to implement. Extreme Programming itself is a continuous code reviewing and integration process. The code reviews exist in everywhere.

In some open source communities, the code reviews are executed just before the code merging to the branch. It is always the most experienced people in this team to do the code reviews and decide whether the code can merge to the main branch. In this way, the period of Continuous Integration is a little bit longer but the code quality is a little bit better.

Return to the question. There has not standard answer for when to do code reviews, and it depends on your original development process and the actual implementation of your continuous integration.

Improve this answer