Category Archives: Code Reviews

Filing Defects in Review Board

In my last post, I talked about an extension for Review Board that would allow users to register “defects”, “TODOs” or “problems” with code that’s up for review.

After chatting with the lead RB devs for a bit, we’ve decided to scrap the extension.

[audible gasp, booing, hissing]

Instead, we’re just going to put it in the core of Review Board.

[thundering applause]

Defects

Why is this useful?  I’ve got a few reasons for you:

  1. It’ll be easier for reviewees to keep track of things left to fix, and similarly, it’ll be harder for reviewees to accidentally skip over fixing a defect that a reviewer has found
  2. My statistics extension will be able to calculate useful things like defect detection rate, and defect density
  3. Maybe it’s just me, but checking things off as “fixed” or “completed” is really satisfying
  4. Who knows, down the line, I might code up an extension that lets you turn finding/closing defects into a game

However, since we’re adding this to the core of Review Board, we have to keep it simple.  One of Review Board’s biggest strengths is in its total lack of clutter.  No bells.  No whistles.  Just the things you need to get the job done.  Let the extensions bring the bells and whistles.

So that means creating a bare-bones defect-tracking mechanism and UI, and leaving it open for extension.  Because who knows, maybe there are some people out there who want to customize what kind of defects they’re filing.

I’ve come up with a design that I think is pretty simple and clean.  And it doesn’t rock the boat – if you’re not interested in filing defects, your Review Board experience stays the same.

Filing a Defect

I propose adding a simple checkbox to the comment dialog to indicate that this comment files a defect, like so:

Comment Defect Checkbox Screenshot

No bells. No whistles. Just a simple little checkbox.

While I’m in there, I’ll try to toss in some hooks so that extension developers can add more fields – for example, the classification or the priority of the defect.  By default, however, it’s just a bare-bones little checkbox.

So far, so good.  You’ve filed a defect.  Maybe this is how it’ll look like in the in-line comment viewer:

The inline comment viewer is showing that a defect report has been filed.

A defect has been reported!

Two Choices

A reviewer can file defects reports, and the reviewee is able to act on them.

Lets say I’m the reviewee.  I’ve just gotten a review, and I’ve got my editor / IDE with my patch waiting in the background.  I see a few defect reports have been filed.  For the ones I completely agree with, I fix them in my editor, and then go back to Review Board and mark them as Fixed.

The defect report has been marked as being fixed.

All fixed!

It’s also possible that I might not agree with one or more of the defect reports.  In this case, I’ll reply to the comment to argue my case.  I might also mark the defect report as Pass, which means, “I’ve seen it, but I think I’ll pass on that”.

The defect report has been marked as "pass".

I think I'll pass on that, thanks.

These comments and defect reports are also visible in the review request details page:

A defect report has been filed, and we're in the review request detail page.

A defect has been filed.

The defect is marked as fixed, and we're in the review request detail page.

All fixed up.

We're passing on the defect report, and we're in the review request detail page.

It's all good - just pass this defect report.

Thoughts?

What do you think?  Am I on the right track?  Am I missing a case?  Does “pass” make sense?  Will this be useful?  I’d love to hear your thoughts.

Review Board Statistics Extensions: Karma, Stopwatch, and FixIt

I just spent the long weekend in Ottawa and Québec City with my parents and my girlfriend Em.

During the long drive back to Toronto from Québec City, I had plenty of time to think about my GSoC project, and where I want to go with it once GSoC is done.

Here’s what I came up with.

Detach Reviewing Time from Statistics

I think it’s a safe assumption that my reviewing-time extension isn’t going to be the only one to generate useful statistical data.

So why not give extension developers an easy mechanism to display statistical data for their extension?

First, I’m going to extract the reviewing-time recording portion of the extension. Then, RB-Stats (or whatever I end up calling it), will introduce it’s own set of hooks for other extensions to register with.  This way, if users want some stats, there will be one place to go to get them.  And if an extension developer wants to make some statistics available, a lot of the hard work will already be done for them.

And if an extension has the capability of combining its data with another extensions data to create a new statistic, we’ll let RB-Stats manage all of that business.

Stopwatch

The reviewing-time feature of RB-Stats will become an extension on its own, and register its data with RB-Stats.  Once RB-Stats and Stopwatch are done, we should be feature equivalent with my demo.

Review Karma

I kind of breezed past this in my demo, but I’m interested in displaying “review karma”.  Review karma is the reviews/review-requests ratio.

But I’m not sure karma is the right word.  It suggests that a low ratio (many review requests, few reviews) is a bad thing.  I’m not so sure that’s true.

Still, I wonder what the impact will be to display review karma?  Not just in the RB-Stats statistics view, but next to user names?  Will there be an impact on review activity when we display this “reputation” value?

FixIt

This is a big one.

Most code review tools allow reviewers to register “defects”, “todos” or “problems” with the code up for review.  This makes it easier for reviewees to keep track of things to fix, and things that have already been taken care of.  It’s also useful in that it helps generate interesting statistics like defect density and defect detection rate (assuming Stopwatch is installed and enabled).

I’m going to tackle this extension as soon as RB-Stats, Stopwatch and Karma are done.  At this point, I’m quite confident that the current extension framework can more or less handle this.

Got any more ideas for me?  Or maybe an extension wish-list?  Let  me know.

Review Board Statistics Extension – Demo Time

If I’ve learned anything from my supervisor, it’s to demo. Demo often. Step out of the lab and introduce what you’ve been working on to the world. Hit the pavement and show, rather than tell.

So here’s a video of me demoing my statistics extension for Review Board.  It’s still in the early phases, but a lot of the groundwork has been taken care of.

And sorry for the video quality.  Desktop capture on Ubuntu turned out to be surprisingly difficult for my laptop, and that’s the best I could do.

So, without further ado, here’s my demo (click here if you can’t see it):

Not bad!  And I haven’t even reached the midterm of GSoC yet.  Still plenty of time to enhance, document, test, and polish.

If you have any questions or comments, I’d love to hear them.

GSoC Update: My Review Board Statistics Extension

The Primary Goal

From the very beginning, my GSoC project has been mainly focused towards one primary goal:  I want to build an extension for Review Board that will allow me to collect information about how long reviewers actually spend reviewing code.

That’s easier said than done.  When I started, the Review Board extension framework wasn’t really in a state to allow such an extension to exist.

So I’ve been tooling around in the Review Board code for the past 2 months, preparing the framework, and getting it ready to handle my extension.

And last night, it started to work.  I can now give rough estimates on how long a reviewer has spent reviewing code.

How It Works

My extension adds a new table to the database which stores “reviewing sessions”.  Each reviewing session is associated to a particular review request and user, and also has a field to store the number of seconds that a user has spent in review.

I’ve created a TemplateHook that allows me to inject Javascript into key areas of Review Board (in particular, the diff viewer, and the screenshot viewer).  The Javascript does the following:  every 10 seconds, we check to see if the mouse has moved on the body of the HTML document.  If it has, we send an “activity” notification to the server.

The server receives this activity notification through the Web API, and checks to see if the time lapsed since the last session update was greater than 10 seconds.  If it is, we increment the working session by 10 seconds and return a 200 HTTP code.  If it isn’t, we don’t change anything and return a 304 HTTP code.

Next, my extension waits for a user to publish a review.  When it notices that a review is being published, it finds the working session for that user and review request, and then attaches it to the published review.  If the user then starts looking at the diff or screenshots again, a new working session is created.

The result?  A pretty decent estimate of how long a user has spent reviewing the code.  No time gets recorded if the user gets up and has a sandwich.  No time gets recorded if the user is on another tab reading Reddit.

An image showing how reviewing time is displayed to the user

Not bad.  For a first draft, anyhow.

I think I’m going to try to chart the data somehow, so that users can track their inspection rates.  I’ll let you know how that goes.

Review Board Tests: SCMTool Segmentation Fault

I can’t honestly say I’ve been doing much test-driven development on Review Board.  Django is a really cool web-framework, but I miss all of the nice testing tools from the Rails ecosystem.

Anyhow, today, I decided to run the tests, and BAM – Segmentation Fault:

Testing Perforce binary diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_changeset … SKIP: perforce/p4python is not installed
Testing Perforce empty and normal diff parsing … SKIP: perforce/p4python is not installed
Testing Perforce empty diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_file … SKIP: perforce/p4python is not installed
Testing parsing SVN diff with binary file … ok
Testing SVNTool.get_file … Segmentation Fault

Yikes.  Getting a segfault in Python is unusual, and a little jarring.  However, it did let me narrow down the problem a bit:  it must have to do with the pysvn bindings that Review Board uses to talk to Subversion, because those are written in C++ (which can certainly segfault).

The machine I was using had Ubuntu Hardy on it, and the Hardy packages have pysvn 1.5.2-1.  Taking a look at the pysvn homepage, the most recent version is actually 1.7.2.

So, I uninstalled pysvn, downloaded the source for 1.7.2, compiled it, and installed it manually.

Segfault gone.  Tests pass.  Awesome sauce.